Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 12 additions & 5 deletions strix/tools/todo/tools.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 JSON Scalar Quotes Become ID Characters

When a caller passes a single todo id as a JSON string scalar like "a3f9c2", json.loads still succeeds but the parsed scalar is now ignored, so the lookup uses the literal value including quotes. This is an old-vs-new regression for the single-id fallback path: the todo stored as a3f9c2 is reported as not found under "a3f9c2".

Suggested change
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(parsed, list):
return [str(item).strip() for item in parsed if str(item).strip()]
if isinstance(parsed, str):
return [parsed.strip()] if parsed.strip() else []
parts = stripped.split(",") if "," in stripped else [stripped]
return [part.strip() for part in parts if part.strip()]
Prompt To Fix With AI
This is a comment left during a code review.
Path: strix/tools/todo/tools.py
Line: 144-147

Comment:
**JSON Scalar Quotes Become ID Characters**

When a caller passes a single todo id as a JSON string scalar like `"a3f9c2"`, `json.loads` still succeeds but the parsed scalar is now ignored, so the lookup uses the literal value including quotes. This is an old-vs-new regression for the single-id fallback path: the todo stored as `a3f9c2` is reported as not found under `"a3f9c2"`.

```suggestion
        if isinstance(parsed, list):
            return [str(item).strip() for item in parsed if str(item).strip()]
        if isinstance(parsed, str):
            return [parsed.strip()] if parsed.strip() else []
        parts = stripped.split(",") if "," in stripped else [stripped]
        return [part.strip() for part in parts if part.strip()]
```

How can I resolve this? If you propose a fix, please make it concise.

if isinstance(raw_ids, list):
return [str(item).strip() for item in raw_ids if str(item).strip()]
return [str(raw_ids).strip()]
Expand Down
81 changes: 81 additions & 0 deletions tests/test_todo.py
Original file line number Diff line number Diff line change
@@ -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"