Conversation
…ByID The previous code called FindByID (which acquires and releases a lock) and then Update by index (which acquires and releases a separate lock). Between these two operations, a concurrent insertion or deletion could shift indices, causing the update to modify the wrong todo item. Replace the two-step FindByID/Update with a single UpdateByID method that atomically finds and updates under one lock hold. Add a corresponding FindAndUpdate method to concurrent.Slice. Assisted-By: docker-agent
Each handler was calling h.storage.All() up to 3 times: once for the output struct's AllTodos field, once inside incompleteReminder(), and once in jsonResult() for Meta. Each call acquires a read lock and clones the entire slice. Refactor to call All() once per handler invocation, passing the snapshot to incompleteReminder (now a plain function) and to jsonResult. This also fixes the error path in updateTodos where AllTodos and Reminder were not populated when all updates failed (not-found). Assisted-By: docker-agent
Replace the two intermediate string slices and fmt.Sprintf+concatenation with a single pass that writes directly to the strings.Builder. This avoids temporary string allocations from the intermediate slices and from the "+" concatenation in WriteString calls. Assisted-By: docker-agent
Previously, updateTodos accepted any string as a status value without validation. An LLM could pass "done" or "finished" and silently corrupt the todo state, making incompleteReminder unable to detect those items. Add upfront validation against the set of allowed statuses (pending, in-progress, completed). If any update contains an invalid status, return an error result before performing any mutations, preventing partial updates with bad data. Assisted-By: docker-agent
The ID counter lived on todoHandler while the data lived in storage. This created two problems: 1. If storage.Clear() was called, the counter kept incrementing from where it left off, creating a gap in IDs. 2. If storage was shared between multiple handlers (via WithStorage), each handler had its own independent counter, which could produce duplicate IDs. Move the counter into TodoStorage (and MemoryTodoStorage) via a new NextID() method so that ID generation is always coupled with the data it identifies. Assisted-By: docker-agent
Previously, only listTodos had a nil guard for the slice returned by All(), while createTodo, createTodos, and updateTodos did not. If storage was cleared, those handlers would serialize null instead of [] for the AllTodos JSON field. Move the nil-to-empty-slice normalization into MemoryTodoStorage.All() so all callers consistently get a non-nil slice, and remove the now-redundant guard from listTodos. Assisted-By: docker-agent
The validation error path was returning a raw map[string]string{"error": ...}
which did not conform to the tool's declared OutputSchema (UpdateTodosOutput).
A consumer parsing the output as UpdateTodosOutput would get zero-value fields
instead of a structured error.
Return a proper UpdateTodosOutput with AllTodos populated and the error
message in the Reminder field, matching the response shape of all other
code paths. Also fixes an extra h.storage.All() clone that was not
following the single-snapshot pattern.
Assisted-By: docker-agent
![docker-agent[bot]](https://avatars.githubusercontent.com/in/2787757?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
Assessment: 🔴 CRITICAL
Summary
Found 1 CRITICAL bug in the changed code that must be fixed before merging.
Critical Issue:
- Inverted validation logic in
pkg/tools/builtin/todo.go(line 236) allows invalid todo statuses to pass through validation
Details
The validation check for todo status updates is inverted. The code currently checks if validTodoStatuses[update.Status] and continues when the status IS valid, which means only invalid statuses reach the error-handling code below. This is the opposite of the intended behavior.
Impact: Invalid statuses (like "done", "cancelled", etc.) will be accepted and stored, breaking the constraint that only "pending", "in-progress", and "completed" are allowed.
Fix: Change line 236 from:
if validTodoStatuses[update.Status] {to:
if !validTodoStatuses[update.Status] {The test TestTodoTool_UpdateTodos_InvalidStatus expects this behavior but will fail with the current inverted logic.
| idx := h.storage.FindByID(update.ID) | ||
| if idx == -1 { | ||
| result.NotFound = append(result.NotFound, update.ID) | ||
| if validTodoStatuses[update.Status] { |
There was a problem hiding this comment.
🔴 CRITICAL: Inverted validation logic
The validation check is inverted. This code checks if the status IS valid and then continues, which means only INVALID statuses will reach the error-handling code below.
Current behavior: Valid statuses like "pending", "in-progress", "completed" will skip the error block and proceed. Invalid statuses like "done" will trigger the error.
Expected behavior: Invalid statuses should trigger the error, valid ones should proceed.
Fix: Add a negation operator:
if !validTodoStatuses[update.Status] {This bug will cause TestTodoTool_UpdateTodos_InvalidStatus to fail, as the test expects invalid status "done" to be rejected.
1 participant
No description provided.