diff --git a/navi/config.py b/navi/config.py index 938a6fc..061028b 100644 --- a/navi/config.py +++ b/navi/config.py @@ -206,6 +206,15 @@ llm_stream_first_chunk_timeout: int = 90 # stream_complete(): max gap between any two subsequent tokens llm_stream_chunk_timeout: int = 60 + # complete()/embed(): attempts per (server, model) on a connection error + # (timeout, refused, DNS) before the fallback gives up. A cloud backend times + # out on its own schedule, and one ReadTimeout from the only configured + # server used to abort the call outright — on prod 2026-10-08 that ended 27 + # calls in a single day, 23 of them memory summarisation. Only connection + # errors are retried; a 4xx/5xx or a missing model is a real answer, not a + # hiccup. + llm_complete_retries: int = 2 + llm_retry_backoff_sec: float = 2.0 # Context compression context_compression_enabled: bool = True diff --git a/navi/llm/base.py b/navi/llm/base.py index 506b105..914771c 100644 --- a/navi/llm/base.py +++ b/navi/llm/base.py @@ -39,7 +39,12 @@ # set on tool result messages tool_call_id: str | None = None name: str | None = None # tool name on tool result messages - created_at: datetime | None = None + # Stamped at creation. It used to default to None, and since only some + # construction sites set it, 85% of stored rows ended up with no timestamp at + # all (every tool message, 14 168/14 168) — so no query could slice history by + # time or measure a pause. Loading a row passes the column through explicitly, + # so NULL rows stay NULL rather than being restamped with "now". + created_at: datetime | None = Field(default_factory=lambda: datetime.now(timezone.utc)) # marks a compressed history block injected by the context compressor is_summary: bool = False # reasoning text produced during this turn (not sent to LLM, display only) diff --git a/navi/llm/fallback.py b/navi/llm/fallback.py index d091573..94e294a 100644 --- a/navi/llm/fallback.py +++ b/navi/llm/fallback.py @@ -138,6 +138,7 @@ models = self._model_list(model) last_err: Exception = LLMBackendError("No backends configured") single_server = len(self._servers) <= 1 + attempts = max(1, settings.llm_complete_retries) for server in self._servers: if _is_dead_server(server.host): @@ -145,33 +146,50 @@ for m in models: if _is_dead_model(server.host, m): continue - try: - resp = await self._get_client(server).complete( - messages, tools=tools, temperature=temperature, - model=m, think=think, max_tokens=max_tokens, - top_k=top_k, top_p=top_p, num_thread=num_thread, - ) - # Record which model actually served (m is the resolved model - # from the priority list that survived on this server). - if resp.model is None: - resp.model = m - return resp - except LLMConnectionError as e: - log.warning("fallback.server_dead", host=server.host, error=str(e)) - last_err = e - if single_server: + # A connection error is a statement about the moment, not about + # the server: retry the same server+model before spending the + # rest of the fallback list (stream_complete has always done this + # for the first chunk). + server_gone = False + for attempt in range(attempts): + try: + resp = await self._get_client(server).complete( + messages, tools=tools, temperature=temperature, + model=m, think=think, max_tokens=max_tokens, + top_k=top_k, top_p=top_p, num_thread=num_thread, + ) + except LLMConnectionError as e: + last_err = e + if attempt + 1 < attempts: + log.warning( + "fallback.complete_retry", + host=server.host, model=m, + attempt=attempt + 1, error=str(e), + ) + await asyncio.sleep(settings.llm_retry_backoff_sec) + continue + log.warning("fallback.server_dead", host=server.host, error=str(e)) # Do not blacklist the only server — the next request # should retry immediately instead of being blocked # for _TTL seconds. + if not single_server: + _dead_servers[server.host] = time.monotonic() + server_gone = True break - _dead_servers[server.host] = time.monotonic() - break # Skip remaining models — server is gone - except LLMModelNotFoundError as e: - log.warning("fallback.model_dead", host=server.host, model=m, error=str(e)) - if not single_server: - _dead_models[(server.host, m)] = time.monotonic() - last_err = e - # Continue to next model on the same server + except LLMModelNotFoundError as e: + log.warning("fallback.model_dead", host=server.host, model=m, error=str(e)) + if not single_server: + _dead_models[(server.host, m)] = time.monotonic() + last_err = e + break + else: + # Record which model actually served (m is the resolved + # model from the priority list that survived here). + if resp.model is None: + resp.model = m + return resp + if server_gone: + break # Skip remaining models — this server is unreachable raise LLMBackendError(f"All backends exhausted: {last_err}") from last_err @@ -183,6 +201,7 @@ models = self._model_list(model) last_err: Exception = LLMBackendError("No backends configured") single_server = len(self._servers) <= 1 + attempts = max(1, settings.llm_complete_retries) for server in self._servers: if _is_dead_server(server.host): @@ -190,21 +209,33 @@ for m in models: if _is_dead_model(server.host, m): continue - try: - return await self._get_client(server).embed(texts, model=m) - except LLMConnectionError as e: - log.warning("fallback.server_dead", host=server.host, error=str(e)) - last_err = e - if single_server: + server_gone = False + for attempt in range(attempts): + try: + return await self._get_client(server).embed(texts, model=m) + except LLMConnectionError as e: + last_err = e + if attempt + 1 < attempts: + log.warning( + "fallback.embed_retry", + host=server.host, model=m, + attempt=attempt + 1, error=str(e), + ) + await asyncio.sleep(settings.llm_retry_backoff_sec) + continue + log.warning("fallback.server_dead", host=server.host, error=str(e)) + if not single_server: + _dead_servers[server.host] = time.monotonic() + server_gone = True break - _dead_servers[server.host] = time.monotonic() + except LLMModelNotFoundError as e: + log.warning("fallback.model_dead", host=server.host, model=m, error=str(e)) + if not single_server: + _dead_models[(server.host, m)] = time.monotonic() + last_err = e + break + if server_gone: break - except LLMModelNotFoundError as e: - log.warning("fallback.model_dead", host=server.host, model=m, error=str(e)) - if not single_server: - _dead_models[(server.host, m)] = time.monotonic() - last_err = e - continue raise LLMBackendError(f"All backends exhausted: {last_err}") from last_err diff --git a/navi/main.py b/navi/main.py index e38b0ee..61a99ba 100644 --- a/navi/main.py +++ b/navi/main.py @@ -29,6 +29,35 @@ # Suppress noisy MCP SDK health-check chatter. logging.getLogger("mcp.server.lowlevel.server").setLevel(logging.WARNING) +log = structlog.get_logger() + + +def _is_transient_db_error(e: Exception) -> bool: + """Connection-level failures a starting Postgres clears on its own.""" + import asyncpg + + return isinstance(e, (OSError, asyncio.TimeoutError, asyncpg.PostgresError)) + + +async def _create_container_with_retry(attempts: int = 15, delay: float = 2.0): + """create_container() opens the Postgres pool eagerly, and after a host reboot + this service comes up before docker does. A single ConnectError there aborted + the whole lifespan (prod, 2026-10-07 15:20 — the agent had just run + `sudo reboot` itself), and the existing retry loop for the table DDL sits + *below* this call, so it never got the chance. Retry the connection errors; + anything else (a missing DATABASE_URL, say) is a real misconfiguration and + still fails fast. Nothing is started before the pool, so a retry is clean. + """ + for attempt in range(1, attempts + 1): + try: + return await create_container() + except Exception as e: + if attempt == attempts or not _is_transient_db_error(e): + raise + log.warning("startup.container_retry", attempt=attempt, error=str(e)) + await asyncio.sleep(delay) + + _base = Path(__file__).parent.parent @@ -93,7 +122,7 @@ port=settings.navi_ui_mcp_port, ) - container = await create_container() + container = await _create_container_with_retry() app.state.container = container # The navi_ui MCP server needs no orchestrator/session-store wiring: it diff --git a/navi/profiles/loader.py b/navi/profiles/loader.py index fa36cc0..a0e587e 100644 --- a/navi/profiles/loader.py +++ b/navi/profiles/loader.py @@ -30,6 +30,11 @@ """Load all valid profile directories under profiles_dir.""" base = Path(profiles_dir) profiles: list[AgentProfile] = [] + # A profile dropped by a bad config.json does not stop the server — it just + # stops existing, and the UI shows a shorter profile list with nothing to + # explain it. Report the tally once so a partial set is never silent. + loaded: list[str] = [] + skipped: list[str] = [] for entry in sorted(base.iterdir()): if not entry.is_dir(): @@ -48,6 +53,7 @@ profile_dir=entry.name, missing=sorted(missing), ) + skipped.append(entry.name) continue system_prompt = prompt_file.read_text(encoding="utf-8").strip() @@ -109,9 +115,16 @@ compression_prompt_file=config.get("compression_prompt_file", None), )) log.debug("profile.loader.loaded", profile_id=config["id"]) + loaded.append(entry.name) except Exception as exc: log.error("profile.loader.error", profile_dir=entry.name, error=str(exc)) + skipped.append(entry.name) + + if skipped: + log.warning("profile.loader.summary", loaded=len(loaded), skipped=skipped) + else: + log.debug("profile.loader.summary", loaded=len(loaded), skipped=[]) return profiles diff --git a/navi/tools/todo.py b/navi/tools/todo.py index 4b1da81..b71ee40 100644 --- a/navi/tools/todo.py +++ b/navi/tools/todo.py @@ -4,6 +4,8 @@ import json from dataclasses import dataclass +import structlog + from navi.llm.base import Message from navi.tools._internal.base import ( Tool, @@ -22,6 +24,35 @@ "skipped": "—", } +log = structlog.get_logger() + +_OPS = ("set", "view", "update", "add", "clear") + + +def _infer_op(params: dict) -> str | None: + """Recover 'op' when the model drops the discriminator and sends only the + operation's own arguments. + + All five operations sit behind a single required `op` key, and models + regularly flatten it away: of the 3385 todo calls stored on prod, 73 arrived + as {"index": 1, "status": "in_progress"}, {"action": "view"} or + {"": "add", "tasks": [...]} and each got a bare "Unknown op: None" — the + intent is unambiguous from the rest of the arguments, so recover it instead + of burning a turn on the round trip. + """ + action = params.get("action") + if isinstance(action, str) and action in _OPS: + return action + # The same slip with the key lost altogether: {"": "add", "tasks": [...]}. + for value in params.values(): + if isinstance(value, str) and value in _OPS: + return value + if params.get("index") is not None or params.get("status") is not None: + return "update" + if params.get("tasks"): + return "set" + return None + @dataclass class _Task: text: str @@ -141,6 +172,11 @@ sid = _sid(ctx.session_id if ctx else None) op = params.get("op") + if op is None: + op = _infer_op(params) + if op is not None: + log.info("todo.op_inferred", op=op, args=params) + if op == "set": raw = params.get("tasks") or [] if not raw: @@ -223,7 +259,15 @@ await _kv_store.clear_scope(_uid(ctx.user_id if ctx else None), sid, "todo") return ToolResult(success=True, output="Plan cleared.") - return ToolResult(success=False, output="", error=f"Unknown op: {op!r}") + return ToolResult( + success=False, + output="", + error=( + f"Unknown op: {op!r}. Send 'op' — one of: " + "'set' (with 'tasks'), 'view', 'update' (with 'index' and 'status'), " + "'add' (with 'tasks'), 'clear'." + ), + ) def _render(self, sid: str, tasks: list[_Task]) -> str: if not tasks: diff --git a/tests/unit/llm/test_fallback.py b/tests/unit/llm/test_fallback.py new file mode 100644 index 0000000..d5f028f --- /dev/null +++ b/tests/unit/llm/test_fallback.py @@ -0,0 +1,153 @@ +"""The multi-server Ollama fallback. + +A connection error is a statement about the moment, not about the server: on +2026-10-08 the only configured server (ollama.com) timed out 27 times in a day, +and because complete() made no second attempt, every one of those aborted the +caller — memory summarisation and planning included — with +"All backends exhausted: ReadTimeout". +""" + +import pytest + +from navi.config import Settings +from navi.exceptions import LLMBackendError, LLMConnectionError, LLMModelNotFoundError +from navi.llm import fallback as fb +from navi.llm.base import LLMResponse, Message +from navi.llm.fallback import FallbackOllamaBackend, ServerEntry + +CLOUD = "https://ollama.com" +LOCAL = "http://localhost:11434" + + +class FakeClient: + """Fails the first `fail_times` calls with `error`, then succeeds.""" + + def __init__(self, fail_times: int = 0, error: Exception | None = None): + self.calls = 0 + self.fail_times = fail_times + self.error = error or LLMConnectionError("ReadTimeout") + + async def complete(self, messages, **kwargs): + self.calls += 1 + if self.calls <= self.fail_times: + raise self.error + return LLMResponse(content="ok", tool_calls=None, finish_reason="stop", model=None) + + async def embed(self, texts, model=None): + self.calls += 1 + if self.calls <= self.fail_times: + raise self.error + return [[0.1] * 4 for _ in texts] + + +@pytest.fixture(autouse=True) +def _fast_retries(monkeypatch): + """No sleeping between attempts, and the retry count the prod config means. + + Settings is a frozen model, so the fixture swaps in a fresh instance rather + than poking attributes at the shared singleton. + """ + monkeypatch.setattr(fb, "settings", retry_settings(2, 0.0)) + + +def retry_settings(retries: int, backoff: float) -> Settings: + return Settings( + _env_file=None, llm_complete_retries=retries, llm_retry_backoff_sec=backoff + ) + + +@pytest.fixture(autouse=True) +def _clean_blacklists(): + fb._dead_servers.clear() + fb._dead_models.clear() + yield + fb._dead_servers.clear() + fb._dead_models.clear() + + +def backend(monkeypatch, servers, clients): + b = FallbackOllamaBackend([ServerEntry(host=h) for h in servers]) + monkeypatch.setattr(b, "_get_client", lambda server: clients[server.host]) + return b + + +class TestCompleteRetry: + async def test_a_transient_timeout_is_retried_on_the_same_server(self, monkeypatch): + client = FakeClient(fail_times=1) + b = backend(monkeypatch, [CLOUD], {CLOUD: client}) + + resp = await b.complete([Message(role="user", content="hi")], model="m") + + assert resp.content == "ok" + assert client.calls == 2 + + async def test_persistent_failure_still_exhausts_the_backend(self, monkeypatch): + client = FakeClient(fail_times=99) + b = backend(monkeypatch, [CLOUD], {CLOUD: client}) + + with pytest.raises(LLMBackendError): + await b.complete([Message(role="user", content="hi")], model="m") + + assert client.calls == fb.settings.llm_complete_retries + + async def test_the_only_server_is_not_blacklisted(self, monkeypatch): + """Blacklisting it would block the next request for _TTL even though the + timeout says nothing about the server's health.""" + client = FakeClient(fail_times=99) + b = backend(monkeypatch, [CLOUD], {CLOUD: client}) + + with pytest.raises(LLMBackendError): + await b.complete([Message(role="user", content="hi")], model="m") + + assert fb._is_dead_server(CLOUD) is False + + async def test_a_dead_server_falls_through_to_the_next_one(self, monkeypatch): + dead, alive = FakeClient(fail_times=99), FakeClient() + b = backend(monkeypatch, [CLOUD, LOCAL], {CLOUD: dead, LOCAL: alive}) + + resp = await b.complete([Message(role="user", content="hi")], model="m") + + assert resp.content == "ok" + assert dead.calls == fb.settings.llm_complete_retries + assert alive.calls == 1 + assert fb._is_dead_server(CLOUD) is True + + async def test_a_missing_model_is_not_retried(self, monkeypatch): + """A 404 is an answer, not a hiccup — retrying it just wastes a timeout.""" + client = FakeClient(fail_times=99, error=LLMModelNotFoundError("model not found")) + b = backend(monkeypatch, [CLOUD], {CLOUD: client}) + + with pytest.raises(LLMBackendError): + await b.complete([Message(role="user", content="hi")], model="m") + + assert client.calls == 1 + + async def test_retries_can_be_switched_off(self, monkeypatch): + monkeypatch.setattr(fb, "settings", retry_settings(1, 0.0)) + client = FakeClient(fail_times=99) + b = backend(monkeypatch, [CLOUD], {CLOUD: client}) + + with pytest.raises(LLMBackendError): + await b.complete([Message(role="user", content="hi")], model="m") + + assert client.calls == 1 + + async def test_the_resolved_model_is_still_stamped(self, monkeypatch): + """The retry loop must not lose the model the agent surfaces to the UI.""" + client = FakeClient(fail_times=1) + b = backend(monkeypatch, [CLOUD], {CLOUD: client}) + + resp = await b.complete([Message(role="user", content="hi")], model="glm-5.3-flash:cloud") + + assert resp.model == "glm-5.3-flash:cloud" + + +class TestEmbedRetry: + async def test_a_transient_timeout_is_retried(self, monkeypatch): + client = FakeClient(fail_times=1) + b = backend(monkeypatch, [CLOUD], {CLOUD: client}) + + vectors = await b.embed(["hello"], model="nomic-embed-text") + + assert len(vectors) == 1 + assert client.calls == 2 diff --git a/tests/unit/llm/test_message.py b/tests/unit/llm/test_message.py index a42d376..50f0b4d 100644 --- a/tests/unit/llm/test_message.py +++ b/tests/unit/llm/test_message.py @@ -1,6 +1,8 @@ """Message is the boundary every stored text passes through.""" -from navi.llm.base import Message +from datetime import datetime, timezone + +from navi.llm.base import Message, ToolCallRequest class TestNulBytes: @@ -22,3 +24,32 @@ def test_absent_content_stays_none(self): assert Message(role="assistant").content is None + + +class TestCreatedAt: + """created_at used to default to None, and since only some construction sites + filled it, 85% of prod rows stored no timestamp — every tool message + (14 168/14 168), so no query could slice history by time or measure a pause. + """ + + def test_new_messages_are_stamped(self): + before = datetime.now(timezone.utc) + message = Message(role="tool", content="output") + assert message.created_at is not None + assert before <= message.created_at <= datetime.now(timezone.utc) + + def test_tool_call_only_messages_are_stamped_too(self): + message = Message( + role="assistant", + tool_calls=[ToolCallRequest(id="1", name="terminal", arguments={"command": "ls"})], + ) + assert message.created_at is not None + + def test_an_explicit_value_is_kept(self): + stamp = datetime(2026, 10, 7, 12, 0, tzinfo=timezone.utc) + assert Message(role="user", content="hi", created_at=stamp).created_at == stamp + + def test_loading_a_row_without_a_timestamp_does_not_invent_one(self): + """PgSessionStore passes the column through explicitly, so a NULL row + stays NULL instead of being restamped with the time of the read.""" + assert Message(role="tool", content="old", created_at=None).created_at is None diff --git a/tests/unit/profiles/test_profile_loader.py b/tests/unit/profiles/test_profile_loader.py new file mode 100644 index 0000000..c383b92 --- /dev/null +++ b/tests/unit/profiles/test_profile_loader.py @@ -0,0 +1,94 @@ +"""load_profiles_from_dir — and the tally that keeps a partial set from being silent. + +A profile whose config.json is unreadable is skipped by design (it must not take +the server down), but the UI then just shows a shorter list. On prod, 2026-10-07 +15:20, four of them (modeler_3d, navi_code, secretary, server_admin) were dropped +with `Extra data: line 125 column 1` — a second JSON document appended to the +file — and the only trace was a per-profile error line: nothing said that four +profiles had gone missing. +""" + +import json + +from structlog.testing import capture_logs + +from navi.profiles.loader import load_profiles_from_dir + +GOOD_CONFIG = {"id": "good", "name": "Good", "description": "a valid profile"} + + +def write_profile(base, dirname, config=GOOD_CONFIG, prompt="You are helpful.\n"): + """Create a profile directory; `config` is written raw as JSON text.""" + d = base / dirname + d.mkdir() + (d / "config.json").write_text( + config if isinstance(config, str) else json.dumps(config), encoding="utf-8" + ) + (d / "system_prompt.txt").write_text(prompt, encoding="utf-8") + return d + + +class TestLoading: + def test_valid_profiles_load(self, tmp_path): + write_profile(tmp_path, "alpha", {**GOOD_CONFIG, "id": "alpha"}) + write_profile(tmp_path, "beta", {**GOOD_CONFIG, "id": "beta"}) + + profiles = load_profiles_from_dir(tmp_path) + + assert [p.id for p in profiles] == ["alpha", "beta"] + + def test_a_broken_config_does_not_take_the_others_with_it(self, tmp_path, capfd): + write_profile(tmp_path, "alpha", {**GOOD_CONFIG, "id": "alpha"}) + write_profile(tmp_path, "corrupt", "{ not json at all") + + profiles = load_profiles_from_dir(tmp_path) + + assert [p.id for p in profiles] == ["alpha"] + out = capfd.readouterr().out + assert "profile.loader.error" in out + assert "corrupt" in out + + def test_a_config_missing_required_keys_is_skipped(self, tmp_path): + write_profile(tmp_path, "alpha", {**GOOD_CONFIG, "id": "alpha"}) + write_profile(tmp_path, "nameless", {"description": "no id, no name"}) + + profiles = load_profiles_from_dir(tmp_path) + + assert [p.id for p in profiles] == ["alpha"] + + def test_subdirectories_without_the_files_are_not_dropped_profiles(self, tmp_path): + """Stray directories are not corruption — counting them would make the + tally cry wolf on every ordinary tree (e.g. a __pycache__ dir).""" + write_profile(tmp_path, "alpha", {**GOOD_CONFIG, "id": "alpha"}) + (tmp_path / "__pycache__").mkdir() + (tmp_path / "notes.txt").write_text("not a profile", encoding="utf-8") + + profiles = load_profiles_from_dir(tmp_path) + + assert [p.id for p in profiles] == ["alpha"] + + +class TestSummaryLog: + def test_the_tally_names_what_was_skipped(self, tmp_path): + write_profile(tmp_path, "alpha", {**GOOD_CONFIG, "id": "alpha"}) + write_profile(tmp_path, "corrupt", "{ definitely not json") + + with capture_logs() as captured: + load_profiles_from_dir(tmp_path) + + errors = [e for e in captured if e["event"] == "profile.loader.error"] + assert [e["profile_dir"] for e in errors] == ["corrupt"] + + summary = [e for e in captured if e["event"] == "profile.loader.summary"] + assert len(summary) == 1 + assert summary[0]["log_level"] == "warning" + assert summary[0]["loaded"] == 1 + assert summary[0]["skipped"] == ["corrupt"] + + def test_a_clean_load_warns_about_nothing(self, tmp_path): + write_profile(tmp_path, "alpha", {**GOOD_CONFIG, "id": "alpha"}) + + with capture_logs() as captured: + load_profiles_from_dir(tmp_path) + + assert [e for e in captured if e["log_level"] in ("warning", "error")] == [] diff --git a/tests/unit/test_startup_retry.py b/tests/unit/test_startup_retry.py new file mode 100644 index 0000000..1f1ac0a --- /dev/null +++ b/tests/unit/test_startup_retry.py @@ -0,0 +1,78 @@ +"""Startup must survive Postgres arriving late. + +On prod, 2026-10-07 15:20 the service came up before docker did (the agent had +just rebooted the host itself) and a single ConnectError from +create_container() aborted the whole lifespan — the table-DDL retry loop sits +*below* that call and never got a turn. +""" + +import pytest + +import navi.main as main_mod + + +def stub_create_container(monkeypatch, errors): + """`errors` — exceptions to raise, in order; then a sentinel container.""" + container = object() + calls = {"n": 0} + + async def fake(): + calls["n"] += 1 + if calls["n"] <= len(errors): + raise errors[calls["n"] - 1] + return container + + monkeypatch.setattr(main_mod, "create_container", fake) + return container, calls + + +class TestTransientClassification: + def test_connection_failures_are_transient(self): + assert main_mod._is_transient_db_error(ConnectionRefusedError()) is True + assert main_mod._is_transient_db_error(OSError("no route to host")) is True + # what asyncpg raises when the pool gives up mid-handshake; the code + # spells it asyncio.TimeoutError, which is the same class on 3.11+ + assert main_mod._is_transient_db_error(TimeoutError()) is True + + def test_misconfiguration_is_not(self): + assert main_mod._is_transient_db_error(RuntimeError("DATABASE_URL is not set")) is False + assert main_mod._is_transient_db_error(ValueError("bad port")) is False + + +class TestContainerRetry: + async def test_a_refused_connection_is_retried(self, monkeypatch): + container, calls = stub_create_container(monkeypatch, [ConnectionRefusedError()]) + + result = await main_mod._create_container_with_retry(attempts=5, delay=0) + + assert result is container + assert calls["n"] == 2 + + async def test_a_late_postgres_is_waited_out(self, monkeypatch): + _, calls = stub_create_container( + monkeypatch, [ConnectionRefusedError(), OSError("timeout")] * 3 + ) + + await main_mod._create_container_with_retry(attempts=8, delay=0) + + assert calls["n"] == 7 + + async def test_exhausting_the_attempts_still_raises(self, monkeypatch): + _, calls = stub_create_container(monkeypatch, [ConnectionRefusedError()] * 10) + + with pytest.raises(ConnectionRefusedError): + await main_mod._create_container_with_retry(attempts=3, delay=0) + + assert calls["n"] == 3 + + async def test_misconfiguration_fails_fast(self, monkeypatch): + """Retrying a missing DATABASE_URL just delays an error that will never + clear itself.""" + _, calls = stub_create_container( + monkeypatch, [RuntimeError("DATABASE_URL is not set")] * 10 + ) + + with pytest.raises(RuntimeError): + await main_mod._create_container_with_retry(attempts=5, delay=0) + + assert calls["n"] == 1 diff --git a/tests/unit/tools/test_todo.py b/tests/unit/tools/test_todo.py index 2ef821c..4e7f3bb 100644 --- a/tests/unit/tools/test_todo.py +++ b/tests/unit/tools/test_todo.py @@ -545,3 +545,73 @@ ctx = ToolContext(session_id="no-plan", user_id="user1") assert await started_metadata_for_call(_TC(), ctx) == {} + + +# ── op inference: the model drops the discriminator ────────────────────────── +# All five operations hide behind one required `op` key, and models flatten it +# away — on prod 73 of 3385 calls arrived without it ({"index": 1, +# "status": "in_progress"}, {"action": "view"}, {"": "add", "tasks": [...]}) and +# each got a bare "Unknown op: None". The intent is unambiguous from the other +# arguments, so the tool recovers it instead of burning a turn on the round trip. + + +@pytest.mark.asyncio +async def test_update_is_inferred_from_index_and_status(_fake_kv): + tool = TodoTool() + ctx = ToolContext(session_id="sess1", user_id="user1") + await tool.execute({"op": "set", "tasks": ["task A"]}, ctx=ctx) + + result = await tool.execute({"index": 1, "status": "done", "validation": "tested"}, ctx=ctx) + + assert result.success is True + assert "done" in result.output + + +@pytest.mark.asyncio +async def test_view_is_inferred_from_action(_fake_kv): + tool = TodoTool() + ctx = ToolContext(session_id="sess1", user_id="user1") + await tool.execute({"op": "set", "tasks": ["task A"]}, ctx=ctx) + + result = await tool.execute({"action": "view"}, ctx=ctx) + + assert result.success is True + assert "task A" in result.output + + +@pytest.mark.asyncio +async def test_bare_op_value_is_honoured(_fake_kv): + """{"": "add", "tasks": [...]} — the key was lost, the value survived. 'add' + must not be treated as 'set': the existing step keeps its status.""" + tool = TodoTool() + ctx = ToolContext(session_id="sess1", user_id="user1") + await tool.execute({"op": "set", "tasks": ["task A"]}, ctx=ctx) + await tool.execute({"index": 1, "status": "done", "validation": "ok"}, ctx=ctx) + + result = await tool.execute({"": "add", "tasks": ["task B"]}, ctx=ctx) + + assert result.success is True + assert dict(await get_task_snapshot("sess1"))["task A"] == "done" + + +@pytest.mark.asyncio +async def test_set_is_inferred_from_tasks_on_an_empty_plan(_fake_kv): + tool = TodoTool() + result = await tool.execute( + {"tasks": ["task A", "task B"]}, + ctx=ToolContext(session_id="sess1", user_id="user1"), + ) + assert result.success is True + assert "task A" in result.output + + +@pytest.mark.asyncio +async def test_uninferable_arguments_get_an_actionable_error(_fake_kv): + """When nothing in the payload names an operation, the error has to tell the + model what to send — an empty result sends it into the same wall again.""" + tool = TodoTool() + result = await tool.execute({}, ctx=ToolContext(session_id="sess1", user_id="user1")) + assert result.success is False + assert "'op'" in result.error + assert "update" in result.error + assert "add" in result.error