core/tools: todo tools commit via session.data transactions

src/plyngent/agent/tools.py

@@ -412,7 +412,10 @@ class ToolRegistry:
                 return denied
             return await self._invoke(definition, args)
 
-        if self._auto_bind_state and (self._instance is not None or self._session is not None):
+        # Bind host-provided state for tools that read contextvars (todo, workspace).
+        # Tag enforcement remains gated by ``auto_bind_state``; binding is always useful
+        # when the registry holds instance/session handles.
+        if self._instance is not None or self._session is not None:
             from plyngent.tools.context import bind_tool_context
 
             with bind_tool_context(

src/plyngent/tools/todo.py

@@ -16,7 +16,12 @@ _VALID_STATUS = frozenset({"pending", "in_progress", "done", "cancelled"})
 
 
 def set_todo_stack(stack: TodoStack | None, *, on_change: Callable[[], None] | None = None) -> None:
-    """Bind the session todo stack for tool handlers (and optional persist hook)."""
+    """Bind the session todo stack for tool handlers (and optional persist hook).
+
+    Process-global bind remains for CLI/agent hosts that keep a live
+    :class:`~plyngent.agent.todo_stack.TodoStack` outside the view store.
+    Prefer session ``data["todo"]`` transactions when available.
+    """
     global _stack, _on_change  # noqa: PLW0603 — intentional process bind
     _stack = stack
     _on_change = on_change
@@ -26,37 +31,57 @@ def get_todo_stack() -> TodoStack | None:
     return _stack
 
 
-def _require_stack() -> TodoStack:
+def _notify() -> None:
-    """Prefer session-bound todo; fall back to process bind during migration.
+    if _on_change is not None:
+        _on_change()
 
-    Order: ``session.todo`` facet → ``session.data["todo"].typed(TodoStack)`` when a
-    view is open/bound → process ``set_todo_stack`` global.
-    """
-    from plyngent.agent.todo_stack import TodoStack
-    from plyngent.tools.context import get_session
 
-    session = get_session()
+def _process_stack() -> TodoStack:
-    if session is not None:
-        if session.todo is not None:
-            return session.todo
-        try:
-            # Prefer live domain object when a txn is open; otherwise skip.
-            todo = session.data["todo"].typed(TodoStack)
-        except RuntimeError:
-            todo = None
-        if isinstance(todo, TodoStack):
-            session.todo = todo
-            return todo
     if _stack is None:
         msg = "todo stack is not available in this session"
         raise RuntimeError(msg)
     return _stack
 
 
-def _notify() -> None:
+async def _with_todo_stack(mutator: Callable[[TodoStack], str]) -> str:
-    if _on_change is not None:
+    """Run *mutator* against the session todo stack and publish changes.
-        _on_change()
+
-    # Session-bound stacks may also persist via host on_change on the process bind.
+    Prefer::
+
+        async with session.data:
+            stack = session.data["todo"].typed(TodoStack)
+            ...
+
+    When a host already bound ``session.todo`` / process ``set_todo_stack``,
+    mutate that live object and still refresh the view buffer when a session
+    is bound so commits stay consistent.
+    """
+    from plyngent.agent.todo_stack import TodoStack
+    from plyngent.tools.context import get_session
+
+    session = get_session()
+    if session is None:
+        result = mutator(_process_stack())
+        _notify()
+        return result
+
+    result = ""
+    async with session.data as data:
+        # Prefer host-bound live stack (CLI keeps TodoStack for nags / memory).
+        if session.todo is not None:
+            stack = session.todo
+        elif _stack is not None:
+            stack = _stack
+            session.todo = stack
+        else:
+            stack = data["todo"].typed(TodoStack)
+            session.todo = stack
+        # Keep view domain + buffer in sync for durable commit.
+        data["todo"].store(stack)
+        result = mutator(stack)
+    # View commit serialized to_raw; process on_change may still persist CLI memory.
+    _notify()
+    return result
 
 
 @tool(name="todo_list", tags=ToolTag.LOCAL | ToolTag.PUBLIC | ToolTag.SESSION_STATE)
@@ -68,11 +93,13 @@ async def todo_list() -> str:
     group of siblings; pop removes the whole top group.
     Pattern: push [T1,T2] → push [T1.1,T1.2] → finish children → pop → push [T2.1]…
     """
-    stack = _require_stack()
+
+    def _run(stack: TodoStack) -> str:
         stack.mark_touched()
-    _notify()
         return stack.render()
 
+    return await _with_todo_stack(_run)
+
 
 @tool(name="todo_push", tags=ToolTag.LOCAL | ToolTag.PUBLIC | ToolTag.SESSION_STATE)
 async def todo_push(titles: list[str], notes: str = "") -> str:
@@ -82,7 +109,8 @@ async def todo_push(titles: list[str], notes: str = "") -> str:
     become members of a single new TOP group. Example: ``[\"T1\", \"T2\"]`` pushes
     one group {T1, T2}; a later ``[\"T1.1\", \"T1.2\"]`` pushes a child group above it.
     """
-    stack = _require_stack()
+
+    def _run(stack: TodoStack) -> str:
         parsed = [t.strip() for t in titles if t and t.strip()]
         if not parsed:
             return "error: titles must be a non-empty array of strings"
@@ -90,10 +118,11 @@ async def todo_push(titles: list[str], notes: str = "") -> str:
             group = stack.push_group(parsed, notes=notes)
         except ValueError as exc:
             return f"error: {exc}"
-    _notify()
         ids = ", ".join(i.id for i in group.items)
         return f"pushed group (depth={stack.depth}) items=[{ids}]\n{stack.render()}"
 
+    return await _with_todo_stack(_run)
+
 
 @tool(name="todo_pop", tags=ToolTag.LOCAL | ToolTag.PUBLIC | ToolTag.SESSION_STATE)
 async def todo_pop() -> str:
@@ -102,16 +131,18 @@ async def todo_pop() -> str:
     Prefer after TOP items are done/cancelled so the stack does not stay
     non-empty with only finished work.
     """
-    stack = _require_stack()
+
+    def _run(stack: TodoStack) -> str:
         group = stack.pop()
         if group is None:
             return "todo stack empty"
-    _notify()
         titles = ", ".join(f"{i.id}:{i.title}" for i in group.items) or "(empty)"
         top = stack.top_group
         top_s = "(empty)" if top is None else ", ".join(i.id for i in top.items)
         return f"popped TOP group ({titles}); new top group=[{top_s}]\n{stack.render()}"
 
+    return await _with_todo_stack(_run)
+
 
 @tool(name="todo_update", tags=ToolTag.LOCAL | ToolTag.PUBLIC | ToolTag.SESSION_STATE)
 async def todo_update(
@@ -126,7 +157,8 @@ async def todo_update(
     finished (not just deferred). Pop the TOP group when that breakdown level
     is complete so the stack does not linger as false open work.
     """
-    stack = _require_stack()
+
+    def _run(stack: TodoStack) -> str:
         status_arg: TodoStatus | None = None
         if status.strip():
             token = status.strip().lower()
@@ -142,17 +174,20 @@ async def todo_update(
             )
         except (KeyError, ValueError) as exc:
             return f"error: {exc}"
-    _notify()
         return f"updated {item.id} → {item.status}: {item.title}\n{stack.render()}"
 
+    return await _with_todo_stack(_run)
+
 
 @tool(name="todo_clear", tags=ToolTag.LOCAL | ToolTag.PUBLIC | ToolTag.SESSION_STATE)
 async def todo_clear() -> str:
     """Clear all groups on the stack."""
-    stack = _require_stack()
+
+    def _run(stack: TodoStack) -> str:
         n = stack.clear()
-    _notify()
         return f"cleared {n} item(s)"
 
+    return await _with_todo_stack(_run)
+
 
 TODO_TOOLS = [todo_list, todo_push, todo_pop, todo_update, todo_clear]

tests/test_tools/test_todo_session.py

@@ -0,0 +1,90 @@
+"""Todo tools via session.data PersistentDataView (no process global)."""
+
+from __future__ import annotations
+
+from plyngent.agent import ToolRegistry
+from plyngent.agent.todo_stack import TodoStack
+from plyngent.tools.context import SessionState, bind_tool_context
+from plyngent.tools.todo import TODO_TOOLS, get_todo_stack, set_todo_stack
+from plyngent.tools.view import MemoryViewStore, session_data_view
+
+
+async def test_todo_tools_session_data_without_process_bind() -> None:
+    set_todo_stack(None)
+    assert get_todo_stack() is None
+
+    store = MemoryViewStore({})
+    session = SessionState(session_id="s1", data=session_data_view(store=store))
+    registry = ToolRegistry(list(TODO_TOOLS), session_state=session)
+
+    out = await registry.execute("todo_push", '{"titles": ["A", "B"]}')
+    assert "pushed" in out
+    assert session.todo is not None
+    assert session.todo.depth == 1
+
+    loaded = await store.load()
+    assert isinstance(loaded, dict)
+    raw = loaded.get("todo")
+    assert isinstance(raw, dict)
+    assert "groups" in raw
+    restored = TodoStack.from_raw(raw)
+    assert [i.title for i in restored.groups[0].items] == ["A", "B"]
+
+    out2 = await registry.execute("todo_list", "{}")
+    assert "A" in out2 and "B" in out2
+
+
+async def test_todo_tools_prefer_session_todo_facet() -> None:
+    stack = TodoStack()
+    set_todo_stack(stack)  # process bind present
+    store = MemoryViewStore({})
+    session = SessionState(session_id="s2", data=session_data_view(store=store), todo=stack)
+    registry = ToolRegistry(list(TODO_TOOLS), session_state=session)
+
+    _ = await registry.execute("todo_push", '{"titles": ["X"]}')
+    assert stack.depth == 1
+    assert session.todo is stack
+
+    loaded = await store.load()
+    assert isinstance(loaded, dict)
+    assert isinstance(loaded.get("todo"), dict)
+    set_todo_stack(None)
+
+
+async def test_todo_tools_view_isolation_two_sessions() -> None:
+    set_todo_stack(None)
+    store_a = MemoryViewStore({})
+    store_b = MemoryViewStore({})
+    session_a = SessionState(session_id="a", data=session_data_view(store=store_a))
+    session_b = SessionState(session_id="b", data=session_data_view(store=store_b))
+
+    reg_a = ToolRegistry(list(TODO_TOOLS), session_state=session_a)
+    reg_b = ToolRegistry(list(TODO_TOOLS), session_state=session_b)
+
+    _ = await reg_a.execute("todo_push", '{"titles": ["only-a"]}')
+    _ = await reg_b.execute("todo_push", '{"titles": ["only-b"]}')
+
+    loaded_a = await store_a.load()
+    loaded_b = await store_b.load()
+    assert isinstance(loaded_a, dict)
+    assert isinstance(loaded_b, dict)
+    raw_a = TodoStack.from_raw(loaded_a.get("todo"))
+    raw_b = TodoStack.from_raw(loaded_b.get("todo"))
+    assert [i.title for i in raw_a.all_items()] == ["only-a"]
+    assert [i.title for i in raw_b.all_items()] == ["only-b"]
+    # Live facets must not alias across sessions.
+    assert session_a.todo is not session_b.todo
+
+
+async def test_with_bound_context_without_registry_session() -> None:
+    """Handlers honor contextvars when registry does not hold session_state."""
+    set_todo_stack(None)
+    store = MemoryViewStore({})
+    session = SessionState(session_id="ctx", data=session_data_view(store=store))
+    registry = ToolRegistry(list(TODO_TOOLS), auto_bind_state=False)
+    with bind_tool_context(session=session):
+        out = await registry.execute("todo_push", '{"titles": ["via-ctx"]}')
+    assert "pushed" in out
+    loaded = await store.load()
+    assert isinstance(loaded, dict)
+    assert isinstance(loaded.get("todo"), dict)