From 3d5380a3d4d4f508587cd45547afdcebd9575491 Mon Sep 17 00:00:00 2001 From: worldmozara Date: Fri, 24 Jul 2026 15:31:26 +0800 Subject: [PATCH] core/tools: todo tools commit via session.data transactions --- src/plyngent/agent/tools.py | 5 +- src/plyngent/tools/todo.py | 175 +++++++++++++++----------- tests/test_tools/test_todo_session.py | 90 +++++++++++++ 3 files changed, 199 insertions(+), 71 deletions(-) create mode 100644 tests/test_tools/test_todo_session.py diff --git a/src/plyngent/agent/tools.py b/src/plyngent/agent/tools.py index 87362a6..487e1c0 100644 --- a/src/plyngent/agent/tools.py +++ b/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( diff --git a/src/plyngent/tools/todo.py b/src/plyngent/tools/todo.py index 7bbd30d..bd9f4c2 100644 --- a/src/plyngent/tools/todo.py +++ b/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: - """Prefer session-bound todo; fall back to process bind during migration. +def _notify() -> None: + 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() - 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 +def _process_stack() -> TodoStack: if _stack is None: msg = "todo stack is not available in this session" raise RuntimeError(msg) return _stack -def _notify() -> None: - if _on_change is not None: - _on_change() - # Session-bound stacks may also persist via host on_change on the process bind. +async def _with_todo_stack(mutator: Callable[[TodoStack], str]) -> str: + """Run *mutator* against the session todo stack and publish changes. + + 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,10 +93,12 @@ 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() - stack.mark_touched() - _notify() - return stack.render() + + def _run(stack: TodoStack) -> str: + stack.mark_touched() + return stack.render() + + return await _with_todo_stack(_run) @tool(name="todo_push", tags=ToolTag.LOCAL | ToolTag.PUBLIC | ToolTag.SESSION_STATE) @@ -82,17 +109,19 @@ 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() - 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" - try: - 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()}" + + 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" + try: + group = stack.push_group(parsed, notes=notes) + except ValueError as exc: + return f"error: {exc}" + 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) @@ -102,15 +131,17 @@ 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() - 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()}" + + def _run(stack: TodoStack) -> str: + group = stack.pop() + if group is None: + return "todo stack empty" + 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) @@ -126,33 +157,37 @@ 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() - status_arg: TodoStatus | None = None - if status.strip(): - token = status.strip().lower() - if token not in _VALID_STATUS: - return "error: status must be pending, in_progress, done, or cancelled" - status_arg = cast("TodoStatus", token) - try: - item = stack.update( - item_id.strip(), - title=title if title.strip() else None, - status=status_arg, - notes=notes if notes != "" else None, - ) - except (KeyError, ValueError) as exc: - return f"error: {exc}" - _notify() - return f"updated {item.id} → {item.status}: {item.title}\n{stack.render()}" + + def _run(stack: TodoStack) -> str: + status_arg: TodoStatus | None = None + if status.strip(): + token = status.strip().lower() + if token not in _VALID_STATUS: + return "error: status must be pending, in_progress, done, or cancelled" + status_arg = cast("TodoStatus", token) + try: + item = stack.update( + item_id.strip(), + title=title if title.strip() else None, + status=status_arg, + notes=notes if notes != "" else None, + ) + except (KeyError, ValueError) as exc: + return f"error: {exc}" + 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() - n = stack.clear() - _notify() - return f"cleared {n} item(s)" + + def _run(stack: TodoStack) -> str: + n = stack.clear() + return f"cleared {n} item(s)" + + return await _with_todo_stack(_run) TODO_TOOLS = [todo_list, todo_push, todo_pop, todo_update, todo_clear] diff --git a/tests/test_tools/test_todo_session.py b/tests/test_tools/test_todo_session.py new file mode 100644 index 0000000..b50afac --- /dev/null +++ b/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)