diff --git a/docs/mechanics.md b/docs/mechanics.md index 54efc6a..f8f7161 100644 --- a/docs/mechanics.md +++ b/docs/mechanics.md @@ -28,7 +28,7 @@ | **Subagent thinking stall detector** | Monitors subagent streaming; if only `thinking` output is emitted for 60 s or 12 000 chars without text/tool calls, aborts the subagent to prevent endless internal-token loops on local models. | Hard-coded `_SUBAGENT_THINKING_STALL_SECONDS=60.0`, `_SUBAGENT_THINKING_STALL_CHARS=12000` | `agent.py` | ❌ | | **Cooperative stop** | Checks `current_stop_event` (asyncio.Event) before each LLM call, during streaming, and after tool execution. Uses clean generator close — never `task.cancel()`. | None | `agent.py` | ✅ | | **Agent-invoked planning** | The `plan` tool runs the planning pipeline when the model itself decides a task needs it. PlanRunner is bound per tool-loop iteration via the `current_plan_runner` ContextVar; the tool result carries the follow-up instruction (complex → present and wait for confirmation, else proceed). PlanningStatus/PlanReady reach the UI mid-turn via `current_event_sink`. | None | `agent.py`, `plan.py` | ✅ | -| **Profile reload mid-session** | After each tool execution batch, checks DB for profile ID change (e.g. from `switch_profile`). If changed, reloads profile, tools, schemas, and backend for next iteration. | None | `agent.py` | ✅ | +| **Profile reload mid-session** | After each tool execution batch, compares the stored session's profile against the profile the run is **bound** to (`bound_profile_id`), not against `session.profile_id` — after a switch the two agree again, because `set_profile()` either rewrites the row (and the run's own `save()` used to write its stale value back) or mutates the very Session object the run holds. On a difference, reloads profile, tools, schemas, and backend for the next iteration. `save()` no longer writes `profile_id` on conflict: `set_profile()` owns that column after creation. | None | `agent.py`, `pg_session_store.py` | ✅ | | **Pre-turn context compression** | Before the assistant reply, estimates tokens via `real_baseline_estimate(session.context, session.context)` (real `prompt_tokens` bulk + heuristic delta; chars//3 heuristic only before the first LLM call) and compresses when over threshold. Guarded by `would_compress()` so `CompressionStarted` is only emitted when the partition can actually shrink the stored context. | `CONTEXT_COMPRESSION_ENABLED`, `OLLAMA_NUM_CTX`, `CONTEXT_COMPRESSION_THRESHOLD` | `agent.py` | ✅ | | **Mid-turn context compression** | On iterations > 0, estimates tokens via `real_baseline_estimate(session.context, preflight_ctx)` (real prompt_tokens bulk + heuristic delta) and compresses with `keep_recent_messages=max(12, CONTEXT_KEEP_RECENT*2)`. Guarded by `would_compress()`. For long autonomous loops where the entire conversation is one turn. | Same as above + `CONTEXT_KEEP_RECENT` | `agent.py` | ✅ | | **Forced `/compact`** | `compact_stream()` — bypasses the token threshold and compresses immediately on client demand (`{"type":"compact"}` WS control message). Emits `CompressionStarted` + `ContextCompressed`; raises `NothingToCompactError` when there is nothing to compress. Bound to `Ctrl+X C` in the TUI. | None | `agent.py`, `websocket.py` | ✅ | diff --git a/docs/profiles.md b/docs/profiles.md index 9fd4a90..25d9494 100644 --- a/docs/profiles.md +++ b/docs/profiles.md @@ -228,6 +228,6 @@ ## Profile switching -`switch_profile` tool updates `session.profile_id` in the DB. After each tool execution batch, `run_stream()` checks for a profile change and reloads profile + tools. Takes effect on the next LLM call. +`switch_profile` tool repoints the session with one narrow `UPDATE sessions SET profile_id` (`PgSessionStore.set_profile()` — never a full `save()`, which would hand out sequence numbers the running turn is still claiming). After each tool execution batch, `run_stream()` compares the stored profile against the profile the run is **bound** to — not against `session.profile_id`, since the store's write leaves those two equal again — and reloads profile + tools when they differ. Takes effect on the next LLM call. For the same reason `save()` leaves the `profile_id` column alone on conflict: a mid-run save carries the profile the run started with and must not write a switch back. Rules (in persona): don't switch for a single off-topic question; switch when the domain clearly changes; never switch back and forth repeatedly. diff --git a/navi/core/agent.py b/navi/core/agent.py index 76f3ad2..3c0c352 100644 --- a/navi/core/agent.py +++ b/navi/core/agent.py @@ -352,6 +352,11 @@ tools = self._tool_list(profile.get_agent_tools()) tool_schemas = [t.schema() for t in tools] llm = self._get_backend(profile.llm_backend) + # The profile these four are bound to. The post-turn reload must compare + # against this, not against session.profile_id: after a switch_profile the + # two can be equal again (the store rewrote the row, or mutated this very + # object) while the tool set above is still the old profile's. + bound_profile_id = profile.id mem = await self._ctx_builder._memory_msg(user_id=session.user_id) @@ -725,13 +730,18 @@ await anti_stall.post_turn(session_id, turn_tool_calls) # 7. If switch_profile was called this iteration, reload profile + tools. - # switch_profile updates the DB but run_stream() holds a local session - # object — without this check the final save would overwrite the change - # and the next LLM call would still use the old tool schemas. + # Compare against bound_profile_id, never against session.profile_id: + # set_profile() either rewrites the row (and this run's own save() + # then writes its stale value back over it) or mutates the very + # Session object the run holds — in both cases session.profile_id + # and the stored row agree again, the check stayed false and the + # turn kept calling the old profile's tools. The bindings at the + # top of the run are the only truth about what tools are live. fresh = await self._sessions.get(session_id) - if fresh and fresh.profile_id != session.profile_id: + if fresh and fresh.profile_id != bound_profile_id: session.profile_id = fresh.profile_id profile = self._profiles.get(session.profile_id) + bound_profile_id = profile.id self._set_active_profile(profile) # The rest of this run executes as the new profile — tools that # resolve a default profile must not keep reporting the old one. diff --git a/navi/core/pg_session_store.py b/navi/core/pg_session_store.py index 8c5fb6d..4a42240 100644 --- a/navi/core/pg_session_store.py +++ b/navi/core/pg_session_store.py @@ -474,13 +474,17 @@ # the session_messages FK is satisfied before messages are inserted. # created_at / pinned / name / next_sequence / archive_threshold are # set on insert and not overwritten on conflict (managed elsewhere). + # profile_id is on that list too, for the same reason: set_profile() + # owns the column after creation, while a run's Session can still + # carry the profile it started with — writing it here would silently + # undo a switch_profile made mid-run (agent.py then reloads the row + # it just clobbered and sees no change at all). await conn.execute( "INSERT INTO sessions " "(id, profile_id, user_id, pinned, special, created_at, last_active, " " context_token_count, name, planning_logs, next_sequence, archive_threshold, session_metadata) " "VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13) " "ON CONFLICT (id) DO UPDATE SET " - " profile_id = EXCLUDED.profile_id, " " user_id = EXCLUDED.user_id, " " last_active = EXCLUDED.last_active, " " context_token_count = EXCLUDED.context_token_count, " diff --git a/navi/core/tool_executor.py b/navi/core/tool_executor.py index 32b680b..45a077b 100644 --- a/navi/core/tool_executor.py +++ b/navi/core/tool_executor.py @@ -180,6 +180,16 @@ image_msg = None metadata: dict = {} if tool is None: + # The agent is calling something its live tool_map does not hold — + # usually a stale toolset (a profile switch that was not picked up). + # Without this line the only trace is the model's own error string. + log.warning( + "tool.not_found", + tool=tc.name, + profile=getattr(ctx, "profile_id", None), + session_id=current_session_id.get(), + live_tools=len(tool_map), + ) content = f"Error: tool '{tc.name}' not found." event = ToolEvent( tool_name=tc.name, diff --git a/tests/unit/core/test_agent.py b/tests/unit/core/test_agent.py index 30307cc..9491248 100644 --- a/tests/unit/core/test_agent.py +++ b/tests/unit/core/test_agent.py @@ -19,11 +19,16 @@ StreamEnd, SubagentComplete, ) -from navi.core.registry import BackendRegistry, ProfileRegistry +from navi.core.registry import BackendRegistry, ProfileRegistry, ToolRegistry from navi.core.session import InMemorySessionStore, Session from navi.exceptions import MaxIterationsReached, NothingToCompactError, SessionNotFound from navi.llm.base import LLMChunk, Message, ToolCallRequest -from tests.conftest_factory import FakeLLMBackend, make_profile, make_registry_with_tools +from tests.conftest_factory import ( + FakeLLMBackend, + FakeTool, + make_profile, + make_registry_with_tools, +) @pytest.fixture @@ -340,6 +345,99 @@ assert saved.messages[-1].token_count == 50 +# ─── switch_profile reload tests ───────────────────────────────────────────── + + +class SwitchProfileTool(FakeTool): + """The real switch_profile's shape: repoint the store, touch nothing else.""" + + def __init__(self, store, target): + super().__init__("switch_profile") + self._store = store + self._target = target + + async def execute(self, arguments, ctx=None): + from navi.tools._internal.base import ToolResult, current_session_id + + sid = (ctx.session_id if ctx else None) or current_session_id.get() + await self._store.set_profile(sid, self._target) + return ToolResult(success=True, output=f"Switched to '{self._target}'.") + + +class ToolRecordingBackend(FakeLLMBackend): + """Records which tools each LLM call was offered.""" + + def __init__(self, **kwargs): + super().__init__(**kwargs) + self.offered: list[set[str]] = [] + + async def stream_complete(self, messages, tools=None, **kwargs): + names = set() + for tool in tools or []: + names.add(tool["function"]["name"] if isinstance(tool, dict) else tool.name) + self.offered.append(names) + async for chunk in super().stream_complete(messages, tools, **kwargs): + yield chunk + + +class TestProfileSwitchReload: + """A mid-turn switch_profile must rebind the run's tools for the next LLM call. + + Both store paths leave session.profile_id equal to the stored row right after + a switch: set_profile() rewrites the row (and the run's own save() then put + its stale value back over it), or it mutates the very Session object the run + holds, as InMemorySessionStore does. The old guard compared those two after + the tool batch, so it was never true — the turn kept offering the old + profile's tools while switch_profile's own answer advertised the new ones. + """ + + def make_agent(self): + store = InMemorySessionStore() + profiles = ProfileRegistry() + profiles.register( + make_profile("test", enabled_tools=["test_tool", "switch_profile"]) + ) + profiles.register(make_profile("other", enabled_tools=["another_tool"])) + tools = ToolRegistry() + tools.register(FakeTool("test_tool"), builtin=True) + tools.register(FakeTool("another_tool"), builtin=True) + tools.register(SwitchProfileTool(store, "other"), builtin=True) + backends = BackendRegistry() + return Agent( + session_store=store, + profile_registry=profiles, + tool_registry=tools, + backend_registry=backends, + ) + + @pytest.mark.asyncio + async def test_the_next_llm_call_is_offered_the_new_profiles_tools(self): + agent = self.make_agent() + backend = ToolRecordingBackend( + responses=["", "final"], + tool_calls=[ + [ + ToolCallRequest( + id="1", + name="switch_profile", + arguments={"profile_id": "other"}, + ) + ], + None, + ], + ) + agent._backends.register("ollama", backend) + session = await agent._sessions.create(profile_id="test") + + async for _ in agent.run_stream(session.id, "switch please"): + pass + + assert "test_tool" in backend.offered[0] + assert backend.offered[1] == {"another_tool"} + # The switch is what the session ends on, not just what the tool said. + assert session.profile_id == "other" + + # ─── final-turn intercept tests ────────────────────────────────────────────── diff --git a/tests/unit/core/test_pg_session_store.py b/tests/unit/core/test_pg_session_store.py index e2025b4..8a3211b 100644 --- a/tests/unit/core/test_pg_session_store.py +++ b/tests/unit/core/test_pg_session_store.py @@ -128,6 +128,31 @@ @pytest.mark.asyncio +async def test_save_leaves_the_profile_column_to_set_profile(): + """save() carries the profile the run *started* with, and it runs right after + every tool batch — including the batch that held switch_profile. Writing + profile_id back here undid the narrow UPDATE in the same second, so the row + (and the post-turn reload that reads it) still showed the old profile. + """ + conn = FakeConnection() + conn.enqueue("OK") # upsert sessions row + conn.enqueue(0) # sequence range + store = PgSessionStore(FakePool(conn)) + store._initialized = True + + await store.save(Session(profile_id="developer")) + + upsert = next( + c for c in conn.calls if c[0] == "execute" and "INSERT INTO sessions" in c[1] + )[1] + insert, on_conflict = upsert.split("ON CONFLICT (id) DO UPDATE SET") + # A brand-new session still gets its profile from the INSERT... + assert "profile_id" in insert + # ...but an existing row keeps whatever set_profile() last wrote. + assert "profile_id" not in on_conflict + + +@pytest.mark.asyncio async def test_archive_old_messages_sends_correct_sql(): conn = FakeConnection() conn.enqueue("INSERT 0 3") # copy to archive diff --git a/tests/unit/core/test_tool_executor.py b/tests/unit/core/test_tool_executor.py index 1148ec8..080ac6e 100644 --- a/tests/unit/core/test_tool_executor.py +++ b/tests/unit/core/test_tool_executor.py @@ -279,3 +279,32 @@ assert job.ring.maxsize == 200 assert job.parent_tool_call_id == "tc1" await job.done.wait() + + +class TestUnresolvedTool: + """A tool the live tool_map does not hold must leave a trace. + + The agent gets an error string back and the user sees it, but the server + logged nothing at all — which is how a stale toolset after switch_profile + (all the new tools "not found") stayed invisible in the journal. + """ + + async def test_missing_tool_is_logged_with_the_live_profile(self, capfd): + from types import SimpleNamespace + + executor = ToolExecutor(ToolRegistry()) + ctx = SimpleNamespace(session_id="s1", profile_id="developer") + + event, msg, image = await executor._execute_one( + ToolCallRequest(id="1", name="ssh_exec", arguments={}), + {"todo": FakeTool("todo")}, + ctx=ctx, + ) + + assert event.success is False + assert msg.content == "Error: tool 'ssh_exec' not found." + assert image is None + logged = capfd.readouterr().out + assert "tool.not_found" in logged + assert "ssh_exec" in logged + assert "developer" in logged