From 946ae52411379df9e98cbd138236913bc109c3e3 Mon Sep 17 00:00:00 2001 From: Sean Kim Date: Fri, 3 Jul 2026 15:28:21 -0700 Subject: [PATCH 1/2] fix(todo): stop json.loads corrupting bare numeric-looking todo ids MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_normalize_todo_ids` runs a bare id string through `json.loads` and, when the result isn't a list, returns `str(parsed)`. Todo ids are `str(uuid.uuid4())[:6]` hex slugs, and ones like "1e5230" or "2363e0" are valid JSON numbers — so json.loads turns them into `inf` / `2363.0`. `mark_todo_done`, `mark_todo_pending`, and `delete_todo` then operate on a non-existent id and report "Todo with ID 'inf' not found", silently failing whenever such an id is passed as a bare string — a form the comma-split fallback explicitly supports. Use json only to detect the JSON *array* form; treat any non-array input as a literal id (optionally comma-separated). Adds tests/test_todo.py covering the normalization paths plus an end-to-end `_mark` resolution. --- strix/tools/todo/tools.py | 17 +++++--- tests/test_todo.py | 81 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+), 5 deletions(-) create mode 100644 tests/test_todo.py diff --git a/strix/tools/todo/tools.py b/strix/tools/todo/tools.py index 07761f5f8..c8f78454a 100644 --- a/strix/tools/todo/tools.py +++ b/strix/tools/todo/tools.py @@ -131,13 +131,20 @@ def _normalize_todo_ids(raw_ids: Any) -> list[str]: stripped = raw_ids.strip() if not stripped: return [] + # json.loads is used only to unpack the JSON *array* form, e.g. + # '["a", "b"]'. A bare string is a literal id (optionally + # comma-separated) and must NOT be routed through the parsed + # scalar value: ids are 6-char uuid slugs, and ones like "1e5230" + # or "2363e0" are valid JSON numbers that json.loads would mangle + # (-> inf / "2363.0"), silently targeting a non-existent todo. try: - data = json.loads(stripped) + parsed = json.loads(stripped) except json.JSONDecodeError: - data = stripped.split(",") if "," in stripped else [stripped] - if isinstance(data, list): - return [str(item).strip() for item in data if str(item).strip()] - return [str(data).strip()] + parsed = None + if isinstance(parsed, list): + return [str(item).strip() for item in parsed if str(item).strip()] + parts = stripped.split(",") if "," in stripped else [stripped] + return [part.strip() for part in parts if part.strip()] if isinstance(raw_ids, list): return [str(item).strip() for item in raw_ids if str(item).strip()] return [str(raw_ids).strip()] diff --git a/tests/test_todo.py b/tests/test_todo.py new file mode 100644 index 000000000..06a204eee --- /dev/null +++ b/tests/test_todo.py @@ -0,0 +1,81 @@ +"""Tests for per-agent todo id normalization and resolution.""" + +from __future__ import annotations + +import json +from typing import TYPE_CHECKING + +import pytest + +import strix.tools.todo.tools as todo_tools +from strix.tools.todo.tools import _normalize_todo_ids + + +if TYPE_CHECKING: + from collections.abc import Iterator + + +@pytest.fixture(autouse=True) +def _reset_todos_storage(monkeypatch: pytest.MonkeyPatch) -> Iterator[None]: + monkeypatch.setattr(todo_tools, "_todos_path", None) + with todo_tools._todos_io_lock: + todo_tools._todos_storage.clear() + yield + with todo_tools._todos_io_lock: + todo_tools._todos_storage.clear() + + +def test_bare_numeric_looking_id_is_preserved() -> None: + # ids are `str(uuid.uuid4())[:6]` hex slugs; slugs like these are valid + # JSON numbers, so json.loads would turn "1e5230" into inf and "2363e0" + # into 2363.0. They must be kept verbatim as literal ids. + assert _normalize_todo_ids("1e5230") == ["1e5230"] + assert _normalize_todo_ids("2363e0") == ["2363e0"] + assert _normalize_todo_ids("0e4440") == ["0e4440"] + + +def test_plain_hex_and_digit_ids_are_preserved() -> None: + assert _normalize_todo_ids("a3f9c2") == ["a3f9c2"] + assert _normalize_todo_ids("123456") == ["123456"] + + +def test_json_array_of_ids_is_unpacked() -> None: + assert _normalize_todo_ids('["1e5230", "a3f9c2"]') == ["1e5230", "a3f9c2"] + + +def test_comma_separated_ids_are_split() -> None: + assert _normalize_todo_ids("1e5230, a3f9c2") == ["1e5230", "a3f9c2"] + + +def test_list_input_is_stringified_and_stripped() -> None: + assert _normalize_todo_ids([" 1e5230 ", "a3f9c2"]) == ["1e5230", "a3f9c2"] + + +def test_empty_and_none_inputs_yield_no_ids() -> None: + assert _normalize_todo_ids("") == [] + assert _normalize_todo_ids(" ") == [] + assert _normalize_todo_ids(None) == [] + + +def test_mark_resolves_a_bare_numeric_looking_id() -> None: + # End-to-end: marking a bare id whose slug is a valid JSON number must + # find and update the real todo, not fail with "Todo with ID 'inf' not + # found". + agent_id = "agent-1" + todos = todo_tools._get_agent_todos(agent_id) + todos["1e5230"] = { + "title": "probe /admin", + "description": None, + "priority": "normal", + "status": "pending", + "created_at": "2026-07-03T00:00:00+00:00", + "updated_at": "2026-07-03T00:00:00+00:00", + "completed_at": None, + } + + result = json.loads(todo_tools._mark(agent_id=agent_id, todo_ids="1e5230", new_status="done")) + + assert result["success"] is True + assert result["marked"] == ["1e5230"] + assert "errors" not in result + assert todos["1e5230"]["status"] == "done" From 370e945da235dbc26ab77d5f30c65753c12b292f Mon Sep 17 00:00:00 2001 From: Sean Kim Date: Fri, 3 Jul 2026 20:16:17 -0700 Subject: [PATCH 2/2] fix(todo): unwrap JSON string-scalar ids (addresses review feedback) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first pass routed every non-array input through the literal-token path, which dropped the surrounding quotes of a JSON string scalar — so a caller passing '"a3f9c2"' looked the id up *with* quotes and missed the real todo (a regression from the prior json.loads behaviour, flagged by the Greptile review on #663). Handle a parsed `str` by unwrapping it, while still keeping numeric-looking bare ids literal (json.loads yields a float/inf for those, not a str). The string / comma / single-token cases resolve into one token list, which also keeps the function within the return-count lint limit. Adds a regression test for the quoted-scalar and empty-string inputs. --- strix/tools/todo/tools.py | 26 ++++++++++++++++++-------- tests/test_todo.py | 10 ++++++++++ 2 files changed, 28 insertions(+), 8 deletions(-) diff --git a/strix/tools/todo/tools.py b/strix/tools/todo/tools.py index c8f78454a..2014fdd1c 100644 --- a/strix/tools/todo/tools.py +++ b/strix/tools/todo/tools.py @@ -131,20 +131,30 @@ def _normalize_todo_ids(raw_ids: Any) -> list[str]: stripped = raw_ids.strip() if not stripped: return [] - # json.loads is used only to unpack the JSON *array* form, e.g. - # '["a", "b"]'. A bare string is a literal id (optionally - # comma-separated) and must NOT be routed through the parsed - # scalar value: ids are 6-char uuid slugs, and ones like "1e5230" - # or "2363e0" are valid JSON numbers that json.loads would mangle - # (-> inf / "2363.0"), silently targeting a non-existent todo. + # json.loads is used only to unpack a JSON *array* ('["a", "b"]') + # or a JSON *string* scalar ('"a3f9c2"', which unwraps to the id). + # Any other bare token is a literal id (optionally comma-separated) + # and must NOT be routed through a parsed *numeric* scalar: ids are + # 6-char uuid slugs, and ones like "1e5230" or "2363e0" are valid + # JSON numbers that json.loads would mangle (-> inf / "2363.0"), + # silently targeting a non-existent todo. try: parsed = json.loads(stripped) except json.JSONDecodeError: parsed = None if isinstance(parsed, list): return [str(item).strip() for item in parsed if str(item).strip()] - parts = stripped.split(",") if "," in stripped else [stripped] - return [part.strip() for part in parts if part.strip()] + # Otherwise resolve to literal tokens: a JSON string scalar unwraps + # to its value, a comma string splits, and anything else is a single + # token kept verbatim (so numeric-looking ids aren't mangled by the + # parse above). + if isinstance(parsed, str): + tokens = [parsed] + elif "," in stripped: + tokens = stripped.split(",") + else: + tokens = [stripped] + return [token.strip() for token in tokens if token.strip()] if isinstance(raw_ids, list): return [str(item).strip() for item in raw_ids if str(item).strip()] return [str(raw_ids).strip()] diff --git a/tests/test_todo.py b/tests/test_todo.py index 06a204eee..58fc24d47 100644 --- a/tests/test_todo.py +++ b/tests/test_todo.py @@ -43,6 +43,16 @@ def test_json_array_of_ids_is_unpacked() -> None: assert _normalize_todo_ids('["1e5230", "a3f9c2"]') == ["1e5230", "a3f9c2"] +def test_json_string_scalar_id_is_unwrapped() -> None: + # A caller may JSON-encode a single id ('"a3f9c2"'); the surrounding + # quotes must be stripped, not treated as part of the id. A quoted + # numeric-looking slug stays a literal string (not a parsed number), + # and an empty JSON string yields no ids. + assert _normalize_todo_ids('"a3f9c2"') == ["a3f9c2"] + assert _normalize_todo_ids('"1e5230"') == ["1e5230"] + assert _normalize_todo_ids('""') == [] + + def test_comma_separated_ids_are_split() -> None: assert _normalize_todo_ids("1e5230, a3f9c2") == ["1e5230", "a3f9c2"]