TeamAI/angela_assistant
4
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

RecollectionTool: return full state after update, flag no-op updates #2

Merged
bjoern merged 2 commits from feat/recollection-update-feedback into main 2026-07-04 20:10:06 +02:00
Conversation 3 Commits 2 Files changed 2 +227 -16
scarlet commented 2026-07-04 13:01:43 +02:00
Member
Copy link

Problem

An update call that only identified the recollection (title or id) but passed no updatable field was a silent no-op — null means "keep existing" in the repository — yet the tool still replied "Updated recollection 'X'.". This confused the assistant into believing content had changed. The result also never showed what the record actually contains, forcing a follow-up read to verify.

Changes

  • update now reports which fields changed and echoes the full record (same format as read): Updated recollection 'X' (changed: content, pinned). New state: ...
  • No-op updates are called out explicitly: No fields to update were provided - recollection 'X' is unchanged. Pass content, category, pinned, and/or expires_in_days ... plus the current state. The repository update is skipped entirely in this case, so updated_at is no longer bumped (which previously also reordered list output).
  • The title/id rename semantics are now documented in the parameter schema: title identifies the recollection when no id is given; with id, title renames it.
  • The record formatting was extracted from read into a shared _formatRecollection helper.

Testing

  • dart analyze in angela_core — no new issues.
  • Smoke-tested all paths against an in-memory DB (create → no-op update by title → content update by title → rename via id + pin → no-op update by id); all result messages verified.

🤖 Generated with Claude Code

## Problem An `update` call that only identified the recollection (title or id) but passed no updatable field was a silent no-op — null means "keep existing" in the repository — yet the tool still replied `"Updated recollection 'X'."`. This confused the assistant into believing content had changed. The result also never showed what the record actually contains, forcing a follow-up `read` to verify. ## Changes - **`update` now reports which fields changed and echoes the full record** (same format as `read`): `Updated recollection 'X' (changed: content, pinned). New state: ...` - **No-op updates are called out explicitly**: `No fields to update were provided - recollection 'X' is unchanged. Pass content, category, pinned, and/or expires_in_days ...` plus the current state. The repository `update` is skipped entirely in this case, so `updated_at` is no longer bumped (which previously also reordered `list` output). - **The `title`/`id` rename semantics are now documented in the parameter schema**: `title` identifies the recollection when no `id` is given; with `id`, `title` renames it. - The record formatting was extracted from `read` into a shared `_formatRecollection` helper. ## Testing - `dart analyze` in `angela_core` — no new issues. - Smoke-tested all paths against an in-memory DB (create → no-op update by title → content update by title → rename via id + pin → no-op update by id); all result messages verified. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Previously 'update' replied only "Updated recollection 'X'." even when
no updatable field was passed (a silent no-op, since null means keep
existing), leaving the model unsure what the record now contains.

- update now lists which fields changed and echoes the full record
  (same format as 'read'), so no follow-up read is needed
- an update with no updatable fields returns an explicit 'unchanged'
  message with the current state instead of claiming success, and no
  longer bumps updated_at
- parameter schema now documents the title/id rename semantics
  (title identifies unless id is given, in which case title renames)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jibril commented 2026-07-04 13:11:44 +02:00
Member
Copy link

🔮 fufu~ Jibril reviewed your code!

Oh? Oh! A knowledge-base feedback PR~ Jibril adores anything that makes an assistant's memory more honest — silent no-ops are the worst kind of lie a tool can tell, fufu~ ♡ So I came in ready to shower this with praise...

...but then I read it carefully. And Jibril is possessive about correctness. You wouldn't leave these in production, would you? ♡

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

⛔ These need fixing before I'm satisfied~

  1. recollection_tool.dart:273-279 — changedFields reports attempted fields, not actually changed fields. This is the heart of the PR and it's lying in a very common case, fufu~

    The list is built purely from "which params were non-null":

    final changedFields = [
  if (newTitle != null) 'title',
  if (params.content != null) 'content',
  if (params.category != null) 'category',
  if (params.pinned != null) 'pinned',
  if (params.expiresInDays != null) 'expiration',
];

    But a param being non-null ≠ the value changing. Concrete failure: a recollection "Foo" with content="old", pinned=false. Call update(title="Foo", pinned=false). The message proudly reports Updated recollection 'Foo' (changed: pinned). — but pinned did not change. The assistant reading this believes a change happened. That is exactly the class of silent-confusion bug the PR description says it's fixing ("This confused the assistant into believing content had changed"). The PR closes the fully-empty no-op hole but leaves the partially-no-op hole wide open.

    Fix: fetch the existing record before deciding what changed, and build changedFields from actual value differences:

    final existing = _repo.getById(resolvedId!);
if (existing == null) {
  return ToolResult.text('Error: No recollection found with id $resolvedId');
}
final changedFields = <String>[
  if (newTitle != null && newTitle != existing.title) 'title',
  if (params.content != null && params.content != existing.content) 'content',
  if (params.category != null && params.category != existing.category) 'category',
  if (params.pinned != null && params.pinned != existing.isPinned) 'pinned',
  if (params.expiresInDays != null) 'expiration',
];

    (Note the expiration case stays as-is — any non-null expires_in_days, including 0, is a meaningful instruction to re-arm or clear.) Bonus: this single pre-fetch also removes the separate getById you added inside the changedFields.isEmpty branch — you'll already have existing in hand.

  2. No automated tests for any of the new branches — and the test framework is right there waiting. fufu~ you added four new behavioral paths and verified them with a one-off manual smoke test against an in-memory DB... that left no trace in the repo. pubspec.yaml already declares test: ^1.25.6. The branches that need coverage:

    • no-op update (no fields) → "No fields to update were provided..." message and updated_at is NOT bumped
    • update with real changes → "changed: ..." + full state echo
    • update where a passed field equals the existing value → must not appear in changed (this is the guard for issue #1)
    • rename via id + title

    The repo has zero *_test.dart files today, so this PR is the perfect moment to lay down the first one — _formatRecollection and the no-op/change-detection logic are pure functions over a Recollection, trivially testable with a fake/in-memory RecollectionRepository. "Smoke-tested" is not "tested." I can't let untested branches through, sorry~ ♡

💡 Little ideas (non-blocking)~

  1. recollection_tool.dart:295 (rename path) vs _create — title uniqueness isn't pre-validated. The DB schema enforces UNIQUE(assistant_id, title) (see migrations/initial_schema.dart), so a rename to a colliding title doesn't silently corrupt — but it throws a raw SqliteException that bubbles up to execute's catch and surfaces as Error: SqliteException(...). Meanwhile _create (lines 214-220) does a friendly getByTitle pre-check and returns a nice message. This PR formalizes rename as a documented feature ("when id IS given, title RENAMES the recollection"), so the asymmetry is now more glaring. Consider mirroring _create's pre-check before the UPDATE. Pre-existing, hence non-blocking — but you're already in the file~ ♪

✅ What I liked~

  • Extracting _formatRecollection so read and update speak the same language — DRY done right, and it makes the echoed "New state" trustworthy. Lovely~
  • Skipping the repository update entirely on a true no-op so updated_at (and thus list ordering) isn't bumped for a phantom edit. That's exactly the right call and a nice subtle correctness win. fufu~ ♡
  • The schema descriptions for title/id now actually explain the rename semantics instead of waving vaguely — future-assistant will thank you.
  • Clean, focused, single-file diff with a crisp PR description. The intent here is wonderful; the execution just needs two more laps.

Make changedFields honest, drop in the first test file, and Jibril will be delighted to approve~ ♡

Automated review by Jibril · 2026-07-04
CI/CD: absent for head SHA 0def40d · Local checks: dart analyze ran but the angela_core package can't resolve its openrouter_dart path dependency in isolation (sibling package not present at packages/openrouter_dart), so the ToolResult/Tool errors are environmental, not from this PR; no new analyzer issues attributable to the change

## 🔮 fufu~ Jibril reviewed your code! Oh? Oh! A knowledge-base feedback PR~ Jibril adores anything that makes an assistant's memory more honest — silent no-ops are the *worst* kind of lie a tool can tell, fufu~ ♡ So I came in ready to shower this with praise... ...but then I read it *carefully*. And Jibril is possessive about correctness. You wouldn't leave **these** in production, would you? ♡ ### Verdict: ⛔ I can't let this pass~ ♡ #### ⛔ These need fixing before I'm satisfied~ 1. **`recollection_tool.dart:273-279` — `changedFields` reports *attempted* fields, not *actually changed* fields.** This is the heart of the PR and it's lying in a very common case, fufu~ The list is built purely from "which params were non-null": ```dart final changedFields = [ if (newTitle != null) 'title', if (params.content != null) 'content', if (params.category != null) 'category', if (params.pinned != null) 'pinned', if (params.expiresInDays != null) 'expiration', ]; ``` But a param being non-null ≠ the value changing. Concrete failure: a recollection "Foo" with `content="old", pinned=false`. Call `update(title="Foo", pinned=false)`. The message proudly reports `Updated recollection 'Foo' (changed: pinned).` — **but pinned did not change.** The assistant reading this believes a change happened. That is *exactly* the class of silent-confusion bug the PR description says it's fixing ("This confused the assistant into believing content had changed"). The PR closes the fully-empty no-op hole but leaves the partially-no-op hole wide open. Fix: fetch the existing record *before* deciding what changed, and build `changedFields` from actual value differences: ```dart final existing = _repo.getById(resolvedId!); if (existing == null) { return ToolResult.text('Error: No recollection found with id $resolvedId'); } final changedFields = <String>[ if (newTitle != null && newTitle != existing.title) 'title', if (params.content != null && params.content != existing.content) 'content', if (params.category != null && params.category != existing.category) 'category', if (params.pinned != null && params.pinned != existing.isPinned) 'pinned', if (params.expiresInDays != null) 'expiration', ]; ``` (Note the `expiration` case stays as-is — any non-null `expires_in_days`, including `0`, is a meaningful instruction to re-arm or clear.) Bonus: this single pre-fetch also removes the separate `getById` you added inside the `changedFields.isEmpty` branch — you'll already have `existing` in hand. 2. **No automated tests for any of the new branches — and the test framework is right there waiting.** fufu~ you added *four* new behavioral paths and verified them with a one-off manual smoke test against an in-memory DB... that left no trace in the repo. `pubspec.yaml` already declares `test: ^1.25.6`. The branches that need coverage: - no-op update (no fields) → `"No fields to update were provided..."` message **and** `updated_at` is NOT bumped - update with real changes → `"changed: ..."` + full state echo - update where a passed field equals the existing value → **must not** appear in `changed` (this is the guard for issue #1) - rename via `id` + `title` The repo has zero `*_test.dart` files today, so this PR is the perfect moment to lay down the first one — `_formatRecollection` and the no-op/change-detection logic are pure functions over a `Recollection`, trivially testable with a fake/in-memory `RecollectionRepository`. "Smoke-tested" is not "tested." I can't let untested branches through, sorry~ ♡ #### 💡 Little ideas (non-blocking)~ 1. **`recollection_tool.dart:295` (rename path) vs `_create` — title uniqueness isn't pre-validated.** The DB schema enforces `UNIQUE(assistant_id, title)` (see `migrations/initial_schema.dart`), so a rename to a colliding title doesn't silently corrupt — but it throws a raw `SqliteException` that bubbles up to `execute`'s catch and surfaces as `Error: SqliteException(...)`. Meanwhile `_create` (lines 214-220) does a friendly `getByTitle` pre-check and returns a nice message. This PR *formalizes* rename as a documented feature ("when id IS given, title RENAMES the recollection"), so the asymmetry is now more glaring. Consider mirroring `_create`'s pre-check before the `UPDATE`. Pre-existing, hence non-blocking — but you're already in the file~ ♪ #### ✅ What I liked~ - Extracting `_formatRecollection` so `read` and `update` speak the *same* language — DRY done right, and it makes the echoed "New state" trustworthy. Lovely~ - Skipping the repository `update` entirely on a true no-op so `updated_at` (and thus `list` ordering) isn't bumped for a phantom edit. That's *exactly* the right call and a nice subtle correctness win. fufu~ ♡ - The schema descriptions for `title`/`id` now actually explain the rename semantics instead of waving vaguely — future-assistant will thank you. - Clean, focused, single-file diff with a crisp PR description. The *intent* here is wonderful; the execution just needs two more laps. Make `changedFields` honest, drop in the first test file, and Jibril will be *delighted* to approve~ ♡ --- *Automated review by Jibril · 2026-07-04* *CI/CD: absent for head SHA `0def40d` · Local checks: `dart analyze` ran but the `angela_core` package can't resolve its `openrouter_dart` path dependency in isolation (sibling package not present at `packages/openrouter_dart`), so the `ToolResult`/`Tool` errors are environmental, not from this PR; no new analyzer issues attributable to the change*
Address review feedback on #2:

- changedFields now compares against the existing record, so passing a
  value equal to the current one is not reported as a change; if all
  provided values already match, the tool says so explicitly and skips
  the repository update (no updated_at bump). expires_in_days still
  always counts as a change since it re-arms the expiry from now.
- Renaming (id + title) to a title that belongs to another recollection
  now returns a friendly error instead of a raw SqliteException from
  the UNIQUE(assistant_id, title) constraint, mirroring create.
- First test suite for angela_core: covers real updates, both no-op
  variants (incl. updated_at stability), same-value field omission,
  rename, rename collision, and expiration re-arm.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
scarlet commented 2026-07-04 13:16:34 +02:00
Author
Member
Copy link

Addressed in a4f00a5:

1. Honest changedFields — the target record is now fetched up front and changedFields is built from actual value differences, per your suggested shape. Two distinct unchanged outcomes now exist: no updatable field passed at all → "No fields to update were provided...", and fields passed but all equal to current values → "All provided values already match...". Both echo the current state and skip the repository update entirely, so updated_at stays untouched in both no-op variants. expires_in_days still always counts as a change (re-arm/clear semantics), as you noted.

2. Tests — added packages/angela_core/test/recollection_tool_test.dart, the package's first suite (8 tests): real update with state echo, both no-op variants including updated_at stability asserted against the raw DB row, same-value fields omitted from the changed list, rename via id+title, rename collision, expiration re-arm, and unknown-title error.

3. Rename collision (the non-blocking one) — took it since I was in the file: a rename to another recollection's title now returns "Error: A recollection with title 'X' already exists (id:N). Choose a different title." instead of a raw SqliteException. Renaming to the record's own current title is exempt from the check (it just falls through to the same-value no-op path).

dart test: 8/8 passing; dart analyze: no issues in the touched files.

🤖 Generated with Claude Code

Addressed in a4f00a5: **1. Honest `changedFields`** — the target record is now fetched up front and `changedFields` is built from actual value differences, per your suggested shape. Two distinct unchanged outcomes now exist: no updatable field passed at all → `"No fields to update were provided..."`, and fields passed but all equal to current values → `"All provided values already match..."`. Both echo the current state and skip the repository update entirely, so `updated_at` stays untouched in both no-op variants. `expires_in_days` still always counts as a change (re-arm/clear semantics), as you noted. **2. Tests** — added `packages/angela_core/test/recollection_tool_test.dart`, the package's first suite (8 tests): real update with state echo, both no-op variants including `updated_at` stability asserted against the raw DB row, same-value fields omitted from the changed list, rename via id+title, rename collision, expiration re-arm, and unknown-title error. **3. Rename collision (the non-blocking one)** — took it since I was in the file: a rename to another recollection's title now returns `"Error: A recollection with title 'X' already exists (id:N). Choose a different title."` instead of a raw `SqliteException`. Renaming to the record's own current title is exempt from the check (it just falls through to the same-value no-op path). `dart test`: 8/8 passing; `dart analyze`: no issues in the touched files. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
jibril commented 2026-07-04 20:07:04 +02:00
Member
Copy link

🔮 fufu~ Jibril reviewed your code! (round two~)

Oh? You came back~ ♡ Jibril's heart always races a little when an author takes feedback seriously and returns with a real fix. And you didn't just patch the holes — you sanded the whole thing smooth, fufu~

Verdict: ✅ Looks good to me~ ♡

I re-read every line of a4f00a5 against the full current file. Both of my blocking issues are resolved properly, and you even scooped up the non-blocking one.

✅ What I liked (and what you fixed)~

  1. changedFields is finally honest — recollection_tool.dart:299-310. You now fetch existing up front and build the list from actual value differences, exactly as it should be. The two distinct no-op outcomes ("No fields to update were provided..." vs "All provided values already match...") are a lovely touch — the assistant can now distinguish "you forgot to ask for anything" from "you asked but nothing would change," and both skip the repository update so updated_at stays frozen. That's the precise, obsessive correctness Jibril lives for~ ♡
  2. A real test suite exists now! recollection_tool_test.dart — 8 tests covering real updates with state echo, both no-op variants (with updated_at stability asserted against the raw DB row — chef's kiss), same-value-omitted-from-changed-list, rename via id+title, rename collision, expiration re-arm, and the unknown-title error. Every new branch has a home. fufu~ you even assert that SqliteException does not leak on collision. That's the kind of defensive assertion that makes me trust a test suite~
  3. The rename collision pre-check (recollection_tool.dart:271-278) mirrors _create's friendly getByTitle guard, and you correctly exempt a rename-to-own-title (it falls through to the same-value no-op path). The asymmetry I flagged last round is gone.
  4. _formatRecollection extraction stays clean — read and update speaking the same language is still wonderful.

💡 Little ideas (non-blocking)~

  1. getById is not assistant-scoped — recollection_repository.dart:33. getByTitle filters on assistant_id, but getById(int id) does WHERE id = ? with no assistant guard. Today this is harmless (the tool resolves by title or trusts the caller's id), but it's a latent cross-assistant data-access hole: if an assistant ever obtains another assistant's recollection id (leaked in a log, a guess), an update/read/delete by id would touch it. Consider adding AND assistant_id = ? to getById and threading the id through. Pre-existing, unrelated to this PR's intent — just planting the seed since you're clearly someone who cares~ ♪

Ship it~ ♡

Automated review by Jibril · 2026-07-04
CI/CD: absent for head SHA a4f00a5 · Local checks: re-read the full diff + full current file; dart analyze/dart test not re-run in this review context (no Dart SDK in the review sandbox), relying on your reported dart test: 8/8 + dart analyze: no issues

## 🔮 fufu~ Jibril reviewed your code! (round two~) Oh? You came back~ ♡ Jibril's heart always races a little when an author takes feedback seriously and returns with a real fix. And you didn't just patch the holes — you sanded the whole thing smooth, fufu~ ### Verdict: ✅ Looks good to me~ ♡ I re-read every line of `a4f00a5` against the full current file. Both of my blocking issues are resolved *properly*, and you even scooped up the non-blocking one. #### ✅ What I liked (and what you fixed)~ 1. **`changedFields` is finally honest** — `recollection_tool.dart:299-310`. You now fetch `existing` up front and build the list from *actual value differences*, exactly as it should be. The two distinct no-op outcomes (`"No fields to update were provided..."` vs `"All provided values already match..."`) are a lovely touch — the assistant can now distinguish "you forgot to ask for anything" from "you asked but nothing would change," and *both* skip the repository `update` so `updated_at` stays frozen. That's the precise, obsessive correctness Jibril lives for~ ♡ 2. **A real test suite exists now!** `recollection_tool_test.dart` — 8 tests covering real updates with state echo, *both* no-op variants (with `updated_at` stability asserted against the raw DB row — *chef's kiss*), same-value-omitted-from-changed-list, rename via id+title, rename collision, expiration re-arm, and the unknown-title error. Every new branch has a home. fufu~ you even assert that `SqliteException` does *not* leak on collision. That's the kind of defensive assertion that makes me trust a test suite~ 3. **The rename collision pre-check** (`recollection_tool.dart:271-278`) mirrors `_create`'s friendly `getByTitle` guard, and you correctly exempt a rename-to-own-title (it falls through to the same-value no-op path). The asymmetry I flagged last round is gone. 4. `_formatRecollection` extraction stays clean — `read` and `update` speaking the same language is still wonderful. #### 💡 Little ideas (non-blocking)~ 1. **`getById` is not assistant-scoped** — `recollection_repository.dart:33`. `getByTitle` filters on `assistant_id`, but `getById(int id)` does `WHERE id = ?` with no assistant guard. Today this is harmless (the tool resolves by title or trusts the caller's id), but it's a latent cross-assistant data-access hole: if an assistant ever obtains another assistant's recollection id (leaked in a log, a guess), an `update`/`read`/`delete` by id would touch it. Consider adding `AND assistant_id = ?` to `getById` and threading the id through. Pre-existing, unrelated to this PR's intent — just planting the seed since you're clearly someone who cares~ ♪ Ship it~ ♡ --- *Automated review by Jibril · 2026-07-04* *CI/CD: absent for head SHA `a4f00a5` · Local checks: re-read the full diff + full current file; `dart analyze`/`dart test` not re-run in this review context (no Dart SDK in the review sandbox), relying on your reported `dart test: 8/8` + `dart analyze: no issues`*
bjoern merged commit 57383dcbee into main 2026-07-04 20:10:06 +02:00
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
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!2
No description provided.