diff --git a/strix/tools/notes/tools.py b/strix/tools/notes/tools.py index 065f8526c6..7a4522b7d8 100644 --- a/strix/tools/notes/tools.py +++ b/strix/tools/notes/tools.py @@ -297,7 +297,7 @@ def _update_note_impl( try: if note_id not in _notes_storage: return {"success": False, "error": f"Note with ID '{note_id}' not found"} - note = _notes_storage[note_id] + note = _notes_storage[note_id].copy() if title is not None: if not title.strip(): return {"success": False, "error": "Title cannot be empty"} @@ -312,6 +312,7 @@ def _update_note_impl( except (ValueError, TypeError) as e: return {"success": False, "error": f"Failed to update note: {e}"} else: + _notes_storage[note_id].update(note) _persist() return { "success": True, diff --git a/strix/tools/todo/tools.py b/strix/tools/todo/tools.py index 135f3bc026..e2c4c7fdfd 100644 --- a/strix/tools/todo/tools.py +++ b/strix/tools/todo/tools.py @@ -238,7 +238,7 @@ def _apply_single_update( ) -> dict[str, Any] | None: if todo_id not in agent_todos: return {"todo_id": todo_id, "error": f"Todo with ID '{todo_id}' not found"} - todo = agent_todos[todo_id] + todo = agent_todos[todo_id].copy() if title is not None: if not title.strip(): return {"todo_id": todo_id, "error": "Title cannot be empty"} @@ -260,6 +260,7 @@ def _apply_single_update( todo["status"] = status_candidate todo["completed_at"] = datetime.now(UTC).isoformat() if status_candidate == "done" else None todo["updated_at"] = datetime.now(UTC).isoformat() + agent_todos[todo_id].update(todo) return None diff --git a/tests/test_notes.py b/tests/test_notes.py index 3729dd7d2a..4d62f8cbaf 100644 --- a/tests/test_notes.py +++ b/tests/test_notes.py @@ -2,16 +2,19 @@ from __future__ import annotations +import json import uuid from typing import TYPE_CHECKING import pytest +from agents.tool_context import ToolContext import strix.tools.notes.tools as notes_tools if TYPE_CHECKING: from collections.abc import Iterator + from pathlib import Path @pytest.fixture(autouse=True) @@ -131,3 +134,51 @@ def test_list_notes_still_filters_on_real_values() -> None: result = notes_tools._list_notes_impl(category="findings") assert [n["title"] for n in result["notes"]] == ["recon"] + + +@pytest.mark.parametrize("content", ["", " "]) +async def test_rejected_update_keeps_note_unchanged_after_later_save( + tmp_path: Path, content: str +) -> None: + notes_tools.hydrate_notes_from_disk(tmp_path) + note_id = notes_tools._create_note_impl("Original title", "Original evidence")["note_id"] + original = notes_tools._get_note_impl(note_id)["note"] + ctx = ToolContext( + context={"agent_id": "root"}, + tool_name="update_note", + tool_call_id="call-update", + tool_arguments="{}", + ) + + raw = await notes_tools.update_note.on_invoke_tool( + ctx, json.dumps({"note_id": note_id, "title": "Rejected title", "content": content}) + ) + + assert json.loads(raw)["success"] is False + assert notes_tools._get_note_impl(note_id)["note"] == original + # A later successful tool call persists every note, including this one. + notes_tools._create_note_impl("Another note", "Unrelated evidence") + notes_tools.hydrate_notes_from_disk(tmp_path) + assert notes_tools._get_note_impl(note_id)["note"] == original + + +def test_valid_note_update_persists_all_supplied_fields(tmp_path: Path) -> None: + notes_tools.hydrate_notes_from_disk(tmp_path) + note_id = notes_tools._create_note_impl( + "Original title", "Original evidence", tags=["old"], agent_id="root" + )["note_id"] + original = notes_tools._get_note_impl(note_id)["note"] + + result = notes_tools._update_note_impl( + note_id, title=" Revised title ", content=" Revised evidence ", tags=[] + ) + + assert result["success"] is True + notes_tools.hydrate_notes_from_disk(tmp_path) + revised = notes_tools._get_note_impl(note_id)["note"] + assert revised["title"] == "Revised title" + assert revised["content"] == "Revised evidence" + assert revised["tags"] == [] + assert revised["agent_id"] == "root" + assert revised["created_at"] == original["created_at"] + assert revised["updated_at"] >= original["updated_at"] diff --git a/tests/test_todo.py b/tests/test_todo.py index 036bcacc31..b5fbdf4914 100644 --- a/tests/test_todo.py +++ b/tests/test_todo.py @@ -1,7 +1,7 @@ from __future__ import annotations import json -from typing import Any +from typing import TYPE_CHECKING, Any import pytest from agents.tool_context import ToolContext @@ -10,6 +10,10 @@ from strix.tools.todo.tools import _coerce_priority, create_todo +if TYPE_CHECKING: + from pathlib import Path + + @pytest.fixture(autouse=True) def _isolate_store() -> Any: tools._todos_storage.clear() @@ -103,3 +107,51 @@ async def test_non_string_priority_does_not_fail_the_batch() -> None: assert result["success"] is True assert result["created_count"] == 3 assert {c["priority"] for c in result["created"]} == {"normal"} + + +@pytest.mark.parametrize( + "invalid_field", [{"priority": "invalid"}, {"priority": "high", "status": "invalid"}] +) +async def test_rejected_todo_update_is_not_persisted_with_valid_sibling( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, invalid_field: dict[str, str] +) -> None: + monkeypatch.setattr(tools, "_todos_path", None) + tools.hydrate_todos_from_disk(tmp_path) + created = await _create([{"title": "Original task"}, {"title": "Other task"}]) + rejected_id, accepted_id = [item["todo_id"] for item in created["created"]] + original = tools._todos_storage["root"][rejected_id].copy() + ctx = ToolContext( + context={"agent_id": "root"}, + tool_name="update_todo", + tool_call_id="call-update", + tool_arguments="{}", + ) + + raw = await tools.update_todo.on_invoke_tool( + ctx, + json.dumps( + { + "updates": json.dumps( + [ + { + "todo_id": rejected_id, + "title": "Rejected title", + "description": "Rejected description", + **invalid_field, + }, + {"todo_id": accepted_id, "status": "done"}, + ] + ) + } + ), + ) + + result = json.loads(raw) + assert result["success"] is False + assert result["updated"] == [accepted_id] + assert result["errors"][0]["todo_id"] == rejected_id + assert tools._todos_storage["root"][rejected_id] == original + tools.hydrate_todos_from_disk(tmp_path) + assert tools._todos_storage["root"][rejected_id] == original + assert tools._todos_storage["root"][accepted_id]["status"] == "done" + assert tools._todos_storage["root"][accepted_id]["completed_at"] is not None