From dfc3919b52e5f086ed611eb8630c46f03b4fb8bc Mon Sep 17 00:00:00 2001 From: chengyongru Date: Tue, 21 Jul 2026 11:25:02 +0800 Subject: [PATCH] fix: stop masking runtime failures --- nanobot/agent/memory.py | 59 +++++------ nanobot/agent/runner.py | 49 +++------ nanobot/api/server.py | 21 +--- nanobot/config/schema.py | 11 ++ nanobot/optional_features.py | 49 ++++----- nanobot/session/manager.py | 35 ++++--- nanobot/utils/gitstore.py | 59 +++++------ nanobot/utils/helpers.py | 121 +++++++++++++--------- nanobot/webui/settings_api.py | 6 +- tests/agent/test_consolidator.py | 47 +++++++++ tests/agent/test_git_store.py | 13 ++- tests/agent/test_runner_governance.py | 28 ++--- tests/agent/test_runner_tool_execution.py | 24 +++++ tests/agent/test_unified_session.py | 2 + tests/channels/test_channel_plugins.py | 30 ++++++ tests/config/test_model_presets.py | 5 + tests/session/test_session_fsync.py | 24 +++++ tests/test_openai_api.py | 62 +---------- tests/utils/test_gitstore.py | 14 +-- tests/utils/test_helpers.py | 14 ++- tests/utils/test_token_estimation.py | 58 ++++++++++- tests/webui/test_settings_api.py | 15 +++ 22 files changed, 446 insertions(+), 300 deletions(-) diff --git a/nanobot/agent/memory.py b/nanobot/agent/memory.py index 39ddf889..4e6c2e08 100644 --- a/nanobot/agent/memory.py +++ b/nanobot/agent/memory.py @@ -941,18 +941,19 @@ class Consolidator: messages_to_summarize = public_history_messages( summary_messages if summary_messages is not None else messages ) + formatted = MemoryStore._format_messages(messages_to_summarize) + formatted = self._truncate_to_token_budget(formatted, runtime=runtime) + system_prompt = render_template( + "agent/consolidator_archive.md", + strip=True, + ) try: - formatted = MemoryStore._format_messages(messages_to_summarize) - formatted = self._truncate_to_token_budget(formatted, runtime=runtime) response = await runtime.provider.chat_with_retry( model=runtime.model, messages=[ { "role": "system", - "content": render_template( - "agent/consolidator_archive.md", - strip=True, - ), + "content": system_prompt, }, {"role": "user", "content": formatted}, ], @@ -962,19 +963,21 @@ class Consolidator: max_tokens=runtime.generation.max_tokens, reasoning_effort=runtime.generation.reasoning_effort, ) - if response.finish_reason == "error": - raise RuntimeError(f"LLM returned error: {response.content}") - summary = response.content or "[no summary]" - self.store.append_history( - summary, - max_chars=_ARCHIVE_SUMMARY_MAX_CHARS, - session_key=session_key, - ) - return summary except Exception: - logger.warning("Consolidation LLM call failed, raw-dumping to history") + logger.warning("Consolidation provider call failed, raw-dumping to history") self.store.raw_archive(messages, session_key=session_key) return None + if response.finish_reason == "error": + logger.warning("Consolidation provider returned an error, raw-dumping to history") + self.store.raw_archive(messages, session_key=session_key) + return None + summary = response.content or "[no summary]" + self.store.append_history( + summary, + max_chars=_ARCHIVE_SUMMARY_MAX_CHARS, + session_key=session_key, + ) + return summary async def maybe_consolidate_by_tokens( self, @@ -1007,14 +1010,10 @@ class Consolidator: replay_max_messages, runtime=runtime, ) - try: - estimated, source = self.estimate_session_prompt_tokens( - session, - runtime=runtime, - ) - except Exception: - logger.exception("Token estimation failed for {}", session.key) - estimated, source = 0, "error" + estimated, source = self.estimate_session_prompt_tokens( + session, + runtime=runtime, + ) if estimated <= 0: self._persist_last_summary(session, last_summary) return @@ -1077,14 +1076,10 @@ class Consolidator: # the next invocation can retry a fresh chunk. break - try: - estimated, source = self.estimate_session_prompt_tokens( - session, - runtime=runtime, - ) - except Exception: - logger.exception("Token estimation failed for {}", session.key) - estimated, source = 0, "error" + estimated, source = self.estimate_session_prompt_tokens( + session, + runtime=runtime, + ) if estimated <= 0: break diff --git a/nanobot/agent/runner.py b/nanobot/agent/runner.py index 0bc32938..7c84e032 100644 --- a/nanobot/agent/runner.py +++ b/nanobot/agent/runner.py @@ -5,7 +5,6 @@ from __future__ import annotations import asyncio import inspect import os -from contextlib import suppress from copy import deepcopy from dataclasses import dataclass, field from pathlib import Path @@ -353,37 +352,16 @@ class AgentRunner: ) for iteration in range(spec.max_iterations): - try: - # Keep the persisted conversation untouched. Context governance - # may repair or compact historical messages for the model, but - # those synthetic edits must not shift the append boundary used - # later when the caller saves only the new turn. - messages_for_model = self.context_governor.prepare_for_model( - governance_config, - messages, - compacted_tool_call_ids, - ) - except Exception: - logger.exception( - "Context governance failed on turn {} for {}; applying minimal repair", - iteration, - spec.session_key or "default", - ) - try: - messages_for_model = ContextGovernor.strip_placeholder_assistant_messages( - messages - ) - messages_for_model = ContextGovernor.strip_malformed_tool_calls( - messages_for_model - ) - messages_for_model = ContextGovernor.drop_orphan_tool_results( - messages_for_model - ) - messages_for_model = ContextGovernor.backfill_missing_tool_results( - messages_for_model - ) - except Exception: - messages_for_model = messages + # Keep the persisted conversation untouched. Context governance + # may repair or compact historical messages for the model, but + # those synthetic edits must not shift the append boundary used + # later when the caller saves only the new turn. A governance + # failure must stop the run instead of sending an ungoverned copy. + messages_for_model = self.context_governor.prepare_for_model( + governance_config, + messages, + compacted_tool_call_ids, + ) context = AgentHookContext( iteration=iteration, messages=messages, @@ -1167,10 +1145,9 @@ class AgentRunner: prepare_call = getattr(spec.tools, "prepare_call", None) tool, params, prep_error = None, tool_call.arguments, None if callable(prepare_call): - with suppress(Exception): - prepared = prepare_call(tool_call.name, tool_call.arguments) - if isinstance(prepared, tuple) and len(prepared) == 3: - tool, params, prep_error = prepared + prepared = prepare_call(tool_call.name, tool_call.arguments) + if isinstance(prepared, tuple) and len(prepared) == 3: + tool, params, prep_error = prepared if prep_error: event = { "name": tool_call.name, diff --git a/nanobot/api/server.py b/nanobot/api/server.py index 2f21131f..bc2f8a7c 100644 --- a/nanobot/api/server.py +++ b/nanobot/api/server.py @@ -344,8 +344,6 @@ async def handle_chat_completions(request: web.Request) -> web.Response: return resp # -- non-streaming path (original logic) -- - fallback = EMPTY_FINAL_RESPONSE_MESSAGE - try: async with session_lock: try: @@ -360,24 +358,9 @@ async def handle_chat_completions(request: web.Request) -> web.Response: timeout=timeout_s, ) response_text = _response_text(response) - if not response_text or not response_text.strip(): - logger.warning("Empty response for session {}, retrying", session_key) - retry_response = await asyncio.wait_for( - agent_loop.process_direct( - content=text, - media=media_paths if media_paths else None, - session_key=session_key, - channel="api", - chat_id=API_CHAT_ID, - persist_user_message=False, - ), - timeout=timeout_s, - ) - response_text = _response_text(retry_response) - if not response_text or not response_text.strip(): - logger.warning("Empty response after retry, using fallback") - response_text = fallback + logger.warning("Empty response for session {}, using fallback", session_key) + response_text = EMPTY_FINAL_RESPONSE_MESSAGE except asyncio.TimeoutError: return _error_json(504, f"Request timed out after {timeout_s}s") diff --git a/nanobot/config/schema.py b/nanobot/config/schema.py index de70ab89..6a496d97 100644 --- a/nanobot/config/schema.py +++ b/nanobot/config/schema.py @@ -163,6 +163,17 @@ class AgentDefaults(Base): ) # Consolidation target ratio (0.5 = 50% of budget retained after compression) dream: DreamConfig = Field(default_factory=DreamConfig) + @field_validator("timezone") + @classmethod + def validate_timezone(cls, value: str) -> str: + from zoneinfo import ZoneInfo, ZoneInfoNotFoundError + + try: + ZoneInfo(value) + except ZoneInfoNotFoundError: + raise ValueError(f"unknown timezone {value!r}") from None + return value + class AgentsConfig(Base): """Agent configuration.""" diff --git a/nanobot/optional_features.py b/nanobot/optional_features.py index b70c415d..05b36f03 100644 --- a/nanobot/optional_features.py +++ b/nanobot/optional_features.py @@ -53,36 +53,32 @@ _BUNDLED_FEATURE_ALIASES = {"documents", "pdf"} def load_pyproject(path: Path) -> dict[str, Any]: - try: - import tomllib + import tomllib - return tomllib.loads(path.read_text(encoding="utf-8")) - except Exception: + try: + content = path.read_text(encoding="utf-8") + except FileNotFoundError: return {} + return tomllib.loads(content) def optional_dependency_groups_from_metadata() -> dict[str, list[str] | None]: - try: - from importlib.metadata import metadata, requires - except Exception: - return {} + from importlib.metadata import metadata, requires try: extras = metadata("nanobot-ai").get_all("Provides-Extra") or [] - groups: dict[str, list[str] | None] = {name: [] for name in extras if name != "dev"} - for raw in requires("nanobot-ai") or []: - try: - req = Requirement(raw) - except Exception: - continue - if not req.marker: - continue - for extra, deps in groups.items(): - if deps is not None and req.marker.evaluate({"extra": extra}): - deps.append(raw) - return groups - except Exception: + raw_requirements = requires("nanobot-ai") or [] + except PackageNotFoundError: return {} + groups: dict[str, list[str] | None] = {name: [] for name in extras if name != "dev"} + for raw in raw_requirements: + req = Requirement(raw) + if not req.marker: + continue + for extra, deps in groups.items(): + if deps is not None and req.marker.evaluate({"extra": extra}): + deps.append(raw) + return groups def optional_dependency_groups() -> dict[str, list[str] | None]: @@ -105,11 +101,7 @@ def optional_dependency_groups() -> dict[str, list[str] | None]: def _install_requirements_for_extra(extra: str, deps: list[str]) -> list[str]: install_args: list[str] = [] for raw in deps: - try: - req = Requirement(raw) - except Exception: - install_args.append(raw) - continue + req = Requirement(raw) if req.marker and not req.marker.evaluate({"extra": extra}): continue req.marker = None @@ -168,10 +160,7 @@ def _extra_dependencies_installed( matched = False for raw in dist.requires or []: - try: - req = Requirement(raw) - except Exception: - continue + req = Requirement(raw) if req.marker and not req.marker.evaluate({"extra": requested_extra}): continue matched = True diff --git a/nanobot/session/manager.py b/nanobot/session/manager.py index dfc49c3e..912d60de 100644 --- a/nanobot/session/manager.py +++ b/nanobot/session/manager.py @@ -43,6 +43,7 @@ _TOOL_CALL_ECHO_RE = re.compile(r'^\s*(?:generate_image|message)\([^)]*\)\s*$') _SESSION_PREVIEW_MAX_CHARS = 120 _SESSION_LIST_PREVIEW_MAX_RECORDS = 200 _SESSION_LIST_PREVIEW_MAX_CHARS = 1_000_000 +_SESSION_DATA_ERRORS = (ValueError, TypeError, AttributeError, KeyError) _FORK_VOLATILE_METADATA_KEYS = { "goal_state", "pending_user_turn", @@ -466,7 +467,7 @@ class SessionManager: if padding != 4: stem += "=" * padding return base64.urlsafe_b64decode(stem).decode("utf-8") - except Exception: + except _SESSION_DATA_ERRORS: return None def _get_session_path(self, key: str) -> Path: @@ -491,11 +492,13 @@ class SessionManager: if not line: continue data = json.loads(line) + if not isinstance(data, dict): + raise ValueError("session records must be JSON objects") if data.get("_type") == "metadata": stored_key = data.get("key") return stored_key if isinstance(stored_key, str) else None return None - except Exception: + except _SESSION_DATA_ERRORS: return None return None @@ -540,11 +543,8 @@ class SessionManager: stored_key, ) continue - try: - shutil.move(str(fallback_path), str(path)) - logger.info("Migrated session {} from {}", key, description) - except Exception: - logger.exception("Failed to migrate session {}", key) + shutil.move(str(fallback_path), str(path)) + logger.info("Migrated session {} from {}", key, description) break if not path.exists(): @@ -564,6 +564,8 @@ class SessionManager: continue data = json.loads(line) + if not isinstance(data, dict): + raise ValueError("session records must be JSON objects") if data.get("_type") == "metadata": metadata = data.get("metadata", {}) @@ -581,7 +583,7 @@ class SessionManager: metadata=metadata, last_consolidated=last_consolidated ) - except Exception as e: + except _SESSION_DATA_ERRORS as e: logger.warning("Failed to load session {}: {}", key, e) repaired = self._repair(key) if repaired is not None: @@ -613,6 +615,9 @@ class SessionManager: except json.JSONDecodeError: skipped += 1 continue + if not isinstance(data, dict): + skipped += 1 + continue if data.get("_type") == "metadata": metadata = data.get("metadata", {}) @@ -640,7 +645,7 @@ class SessionManager: metadata=metadata, last_consolidated=last_consolidated ) - except Exception as e: + except _SESSION_DATA_ERRORS as e: logger.warning("Repair failed for session {}: {}", key, e) return None @@ -842,7 +847,7 @@ class SessionManager: "metadata": metadata, "messages": messages, } - except Exception as e: + except _SESSION_DATA_ERRORS as e: logger.warning("Failed to read session {}: {}", key, e) repaired = self._repair(key) if repaired is not None: @@ -866,6 +871,8 @@ class SessionManager: if not line: continue data = json.loads(line) + if not isinstance(data, dict): + raise ValueError("session records must be JSON objects") if data.get("_type") != "metadata": return None metadata = data.get("metadata", {}) @@ -876,7 +883,7 @@ class SessionManager: "metadata": metadata if isinstance(metadata, dict) else {}, } return None - except Exception as e: + except _SESSION_DATA_ERRORS as e: logger.warning("Failed to read session metadata {}: {}", key, e) repaired = self._repair(key) if repaired is not None: @@ -907,6 +914,8 @@ class SessionManager: first_line = f.readline().strip() if first_line: data = json.loads(first_line) + if not isinstance(data, dict): + raise ValueError("session records must be JSON objects") if data.get("_type") == "metadata": key = data.get("key") or fallback_key metadata = data.get("metadata", {}) @@ -926,6 +935,8 @@ class SessionManager: ): break item = json.loads(line) + if not isinstance(item, dict): + raise ValueError("session records must be JSON objects") if item.get("_type") == "metadata": continue text = _message_preview_text(item) @@ -948,7 +959,7 @@ class SessionManager: "path": str(path), } ) - except Exception: + except _SESSION_DATA_ERRORS: repaired = self._repair(fallback_key, path=path) if repaired is not None: sessions.append( diff --git a/nanobot/utils/gitstore.py b/nanobot/utils/gitstore.py index 46199b06..51f3c9b6 100644 --- a/nanobot/utils/gitstore.py +++ b/nanobot/utils/gitstore.py @@ -16,6 +16,10 @@ from loguru import logger _WORKING_TREE_DIFF_MAX_CHARS = 6000 +class GitStoreError(RuntimeError): + """Raised when the memory Git repository cannot complete an operation.""" + + @dataclass class CommitInfo: sha: str # Short SHA (8 chars) @@ -125,9 +129,8 @@ class GitStore: ) logger.info("Git store initialized at {}", self._workspace) return True - except Exception: - logger.exception("Git store init failed for {}", self._workspace) - return False + except Exception as exc: + raise GitStoreError(f"Git store init failed for {self._workspace}") from exc # -- daily operations ------------------------------------------------------ @@ -161,9 +164,8 @@ class GitStore: sha = sha_bytes.hex()[:8] logger.debug("Git auto-commit: {} ({})", sha, message) return sha - except Exception: - logger.exception("Git auto-commit failed: {}", message) - return None + except Exception as exc: + raise GitStoreError(f"Git auto-commit failed: {message}") from exc # -- internal helpers ------------------------------------------------------ @@ -190,8 +192,8 @@ class GitStore: break sha = commit.parents[0] if commit.parents else None return None - except Exception: - return None + except Exception as exc: + raise GitStoreError(f"Git SHA resolution failed: {short_sha}") from exc def _is_inside_git_repo(self) -> bool: """Check if self._workspace is already inside a git repository. @@ -268,16 +270,15 @@ class GitStore: sha = commit.parents[0] if commit.parents else None return entries - except Exception: - logger.exception("Git log failed") - return [] + except Exception as exc: + raise GitStoreError("Git log failed") from exc def line_ages(self, file_path: str) -> list[LineAge]: """Compute the age of each line in a tracked file via git blame. Returns one LineAge per line, in order. - Returns an empty list if the repo is not initialized, the file is - empty, or annotation fails. + Returns an empty list if the repo is not initialized or the file is + empty. Annotation failures raise :class:`GitStoreError`. """ if not self.is_initialized(): @@ -291,9 +292,8 @@ class GitStore: from dulwich import porcelain annotated = porcelain.annotate(str(self._workspace), file_path) - except Exception: - logger.exception("Git line_ages annotate failed for {}", file_path) - return [] + except Exception as exc: + raise GitStoreError(f"Git line annotation failed for {file_path}") from exc if not annotated: return [] @@ -321,9 +321,8 @@ class GitStore: outstream=out, ) return out.getvalue().decode("utf-8", errors="replace") - except Exception: - logger.exception("Git diff_commits failed") - return "" + except Exception as exc: + raise GitStoreError(f"Git diff failed for {sha1}..{sha2}") from exc def summarize_working_tree(self, paths: list[str]) -> str: """Structured summary of working-tree changes vs HEAD for *paths*. @@ -354,8 +353,8 @@ class GitStore: import difflib from dulwich.repo import Repo - except ImportError: - return "" + except ImportError as exc: + raise GitStoreError("Git working-tree summary dependencies are unavailable") from exc summary_lines: list[str] = [] diff_lines: list[str] = [] @@ -409,9 +408,8 @@ class GitStore: total_removed += removed summary_lines.append(f"{path}: +{added} -{removed}") diff_lines.extend(hunks) - except Exception: - logger.exception("Git summarize_working_tree failed") - return "" + except Exception as exc: + raise GitStoreError("Git working-tree summary failed") from exc if changed == 0: return "" @@ -471,9 +469,8 @@ class GitStore: diff = self.diff_commits(parent.hex()[:8], c.sha) if parent else "" return c, diff return None - except Exception: - logger.exception("Git show_commit_diff failed") - return None + except Exception as exc: + raise GitStoreError(f"Git commit display failed for {short_sha}") from exc # -- restore --------------------------------------------------------------- @@ -485,7 +482,8 @@ class GitStore: is provided, commits outside that history are rejected before any files are changed. - Returns the new commit SHA, or None on failure. + Returns the new commit SHA, or ``None`` when the commit cannot be reverted. + Repository and filesystem failures raise :class:`GitStoreError`. """ if not self.is_initialized(): return None @@ -534,9 +532,8 @@ class GitStore: # Commit the restored state msg = f"revert: undo {commit}" return self.auto_commit(msg) - except Exception: - logger.exception("Git revert failed for {}", commit) - return None + except Exception as exc: + raise GitStoreError(f"Git revert failed for {commit}") from exc @staticmethod def _read_blob_from_tree(repo, tree, filepath: str) -> str | None: diff --git a/nanobot/utils/helpers.py b/nanobot/utils/helpers.py index eebee792..86b4789d 100644 --- a/nanobot/utils/helpers.py +++ b/nanobot/utils/helpers.py @@ -278,11 +278,7 @@ def current_time_str(timezone: str | None = None) -> str: """Return the current time string.""" from zoneinfo import ZoneInfo - try: - tz = ZoneInfo(timezone) if timezone else None - except (KeyError, Exception): - tz = None - + tz = ZoneInfo(timezone) if timezone else None now = datetime.now(tz=tz) if tz else datetime.now().astimezone() offset = now.strftime("%z") offset_fmt = f"{offset[:3]}:{offset[3:]}" if len(offset) == 5 else offset @@ -320,8 +316,7 @@ def truncate_text_to_tokens(text: str, max_tokens: int) -> str: Unlike :func:`truncate_text`, this measures actual tokens, so the cap holds regardless of language or content (CJK and code cost more tokens per char). - Falls back to a char-based estimate (~4 chars/token) if tiktoken is - unavailable. + Falls back to a conservative UTF-8 byte budget if tiktoken is unavailable. """ if max_tokens <= 0: return text @@ -340,11 +335,23 @@ def truncate_text_to_tokens(text: str, max_tokens: int) -> str: return result return enc.decode(tokens[:max_tokens]) except Exception: - max_chars = max_tokens * 4 - suffix_chars = len(_TRUNCATED_SUFFIX) - if max_chars <= suffix_chars: - return text[:max_chars] - return truncate_text(text, max_chars - suffix_chars) + if len(text.encode("utf-8")) <= max_tokens: + return text + suffix_bytes = len(_TRUNCATED_SUFFIX.encode("utf-8")) + if max_tokens <= suffix_bytes: + return _truncate_text_to_utf8_bytes(text, max_tokens) + body = _truncate_text_to_utf8_bytes(text, max_tokens - suffix_bytes) + return body + _TRUNCATED_SUFFIX + + +def _truncate_text_to_utf8_bytes(text: str, max_bytes: int) -> str: + """Return the longest code-point prefix within a UTF-8 byte budget.""" + if max_bytes <= 0: + return "" + encoded = text.encode("utf-8") + if len(encoded) <= max_bytes: + return text + return encoded[:max_bytes].decode("utf-8", errors="ignore") def recent_message_start_index( @@ -569,51 +576,67 @@ def build_assistant_message( return msg -def estimate_prompt_tokens( +def _estimate_prompt_tokens_with_source( messages: list[dict[str, Any]], tools: list[dict[str, Any]] | None = None, -) -> int: - """Estimate prompt tokens with tiktoken. +) -> tuple[int, str]: + """Estimate prompt tokens and identify the counter used. Counts all fields that providers send to the LLM: content, tool_calls, reasoning_content, tool_call_id, name, plus per-message framing overhead. """ + parts: list[str] = [] + for msg in messages: + content = msg.get("content") + if isinstance(content, str): + parts.append(content) + elif isinstance(content, list): + for part in content: + if isinstance(part, dict) and part.get("type") == "text": + txt = part.get("text", "") + if txt: + parts.append(txt) + + tc = msg.get("tool_calls") + if tc: + parts.append(json.dumps(tc, ensure_ascii=False)) + + rc = msg.get("reasoning_content") + if isinstance(rc, str) and rc: + parts.append(rc) + + for key in ("name", "tool_call_id"): + value = msg.get(key) + if isinstance(value, str) and value: + parts.append(value) + + message_payload = "\n".join(parts) + per_message_overhead = len(messages) * 4 try: enc = _get_token_encoding() - parts: list[str] = [] - for msg in messages: - content = msg.get("content") - if isinstance(content, str): - parts.append(content) - elif isinstance(content, list): - for part in content: - if isinstance(part, dict) and part.get("type") == "text": - txt = part.get("text", "") - if txt: - parts.append(txt) - - tc = msg.get("tool_calls") - if tc: - parts.append(json.dumps(tc, ensure_ascii=False)) - - rc = msg.get("reasoning_content") - if isinstance(rc, str) and rc: - parts.append(rc) - - for key in ("name", "tool_call_id"): - value = msg.get(key) - if isinstance(value, str) and value: - parts.append(value) - tool_tokens = ( _estimate_tools_tokens(enc, tools, leading_separator=bool(parts)) if tools else 0 ) - - per_message_overhead = len(messages) * 4 - message_tokens = len(enc.encode("\n".join(parts))) if parts else 0 - return message_tokens + tool_tokens + per_message_overhead + message_tokens = len(enc.encode(message_payload)) if message_payload else 0 + return message_tokens + tool_tokens + per_message_overhead, "tiktoken" except Exception: - return 0 + tool_payload = ( + ("\n" if message_payload else "") + json.dumps(tools, ensure_ascii=False) + if tools + else "" + ) + payload = message_payload + tool_payload + estimated = len(payload.encode("utf-8")) + return estimated + per_message_overhead, "heuristic" + + +def estimate_prompt_tokens( + messages: list[dict[str, Any]], + tools: list[dict[str, Any]] | None = None, +) -> int: + """Estimate prompt tokens with tiktoken and a conservative byte fallback.""" + estimated, _ = _estimate_prompt_tokens_with_source(messages, tools) + return estimated def estimate_message_tokens(message: dict[str, Any]) -> int: @@ -651,7 +674,7 @@ def estimate_message_tokens(message: dict[str, Any]) -> int: enc = _get_token_encoding() return max(4, len(enc.encode(payload)) + 4) except Exception: - return max(4, len(payload) // 4 + 4) + return max(4, len(payload.encode("utf-8")) + 4) def estimate_prompt_tokens_chain( @@ -660,16 +683,16 @@ def estimate_prompt_tokens_chain( messages: list[dict[str, Any]], tools: list[dict[str, Any]] | None = None, ) -> tuple[int, str]: - """Estimate prompt tokens via provider counter first, then tiktoken fallback.""" + """Estimate prompt tokens via provider, tiktoken, then a byte heuristic.""" provider_counter = getattr(provider, "estimate_prompt_tokens", None) if callable(provider_counter): with suppress(Exception): tokens, source = provider_counter(messages, tools, model) if isinstance(tokens, (int, float)) and tokens > 0: return int(tokens), str(source or "provider_counter") - estimated = estimate_prompt_tokens(messages, tools) + estimated, source = _estimate_prompt_tokens_with_source(messages, tools) if estimated > 0: - return int(estimated), "tiktoken" + return int(estimated), source return 0, "none" diff --git a/nanobot/webui/settings_api.py b/nanobot/webui/settings_api.py index 2be1d131..7d3880a2 100644 --- a/nanobot/webui/settings_api.py +++ b/nanobot/webui/settings_api.py @@ -758,11 +758,7 @@ def settings_payload( config = load_config() defaults = config.agents.defaults active_preset_name = defaults.model_preset or "default" - try: - effective_preset = config.resolve_preset() - except Exception: - effective_preset = config.resolve_default_preset() - active_preset_name = "default" + effective_preset = config.resolve_preset() provider_name = ( config.get_provider_name(effective_preset.model, preset=effective_preset) diff --git a/tests/agent/test_consolidator.py b/tests/agent/test_consolidator.py index 33af0930..1e48b595 100644 --- a/tests/agent/test_consolidator.py +++ b/tests/agent/test_consolidator.py @@ -260,6 +260,42 @@ class TestConsolidatorArchiveErrorHandling: assert len(entries) == 1 assert "[RAW]" not in entries[0]["content"] + async def test_archive_propagates_history_write_failure( + self, consolidator, mock_provider, runtime + ): + mock_provider.chat_with_retry.return_value = MagicMock( + content="Summary.", + finish_reason="stop", + ) + consolidator.store.append_history = MagicMock(side_effect=OSError("disk full")) + consolidator.store.raw_archive = MagicMock() + + with pytest.raises(OSError, match="disk full"): + await consolidator.archive( + [{"role": "user", "content": "important"}], + runtime=runtime, + ) + + consolidator.store.raw_archive.assert_not_called() + + async def test_archive_propagates_template_failure_without_raw_archive( + self, consolidator, mock_provider, runtime, monkeypatch + ): + consolidator.store.raw_archive = MagicMock() + monkeypatch.setattr( + "nanobot.agent.memory.render_template", + MagicMock(side_effect=RuntimeError("template failed")), + ) + + with pytest.raises(RuntimeError, match="template failed"): + await consolidator.archive( + [{"role": "user", "content": "important"}], + runtime=runtime, + ) + + mock_provider.chat_with_retry.assert_not_awaited() + consolidator.store.raw_archive.assert_not_called() + class TestConsolidatorTokenBudget: async def test_prompt_below_threshold_does_not_consolidate( @@ -276,6 +312,17 @@ class TestConsolidatorTokenBudget: await consolidator.maybe_consolidate_by_tokens(session, runtime=runtime) consolidator.archive.assert_not_called() + async def test_token_estimation_failure_propagates(self, consolidator, runtime): + session = Session(key="test:estimate-failure") + session.add_message("user", "hello") + consolidator.sessions._session_cache[session.key] = session + consolidator.estimate_session_prompt_tokens = MagicMock( + side_effect=RuntimeError("counter failed") + ) + + with pytest.raises(RuntimeError, match="counter failed"): + await consolidator.maybe_consolidate_by_tokens(session, runtime=runtime) + async def test_estimate_uses_full_unconsolidated_tail(self, consolidator, runtime): """Consolidation pressure must see messages hidden by the replay window.""" session = Session(key="test:full-tail") diff --git a/tests/agent/test_git_store.py b/tests/agent/test_git_store.py index fa893d66..b3c28160 100644 --- a/tests/agent/test_git_store.py +++ b/tests/agent/test_git_store.py @@ -1,9 +1,10 @@ """Tests for GitStore — git-backed version control for memory files.""" +from unittest.mock import patch import pytest -from nanobot.utils.gitstore import CommitInfo, GitStore +from nanobot.utils.gitstore import CommitInfo, GitStore, GitStoreError TRACKED = ["SOUL.md", "USER.md", "memory/MEMORY.md"] @@ -49,6 +50,11 @@ class TestInit: assert len(commits) == 1 assert "init" in commits[0].message + def test_init_failure_is_explicit(self, git): + with patch("dulwich.porcelain.init", side_effect=OSError("cannot initialize")): + with pytest.raises(GitStoreError, match="init failed"): + git.init() + class TestBuildGitignore: def test_subdirectory_dirs(self, git): @@ -97,6 +103,11 @@ class TestAutoCommit: git_ready.auto_commit("nothing 2") assert len(git_ready.log()) == 1 # only init commit + def test_status_failure_is_explicit(self, git_ready): + with patch("dulwich.porcelain.status", side_effect=OSError("broken index")): + with pytest.raises(GitStoreError, match="auto-commit failed"): + git_ready.auto_commit("update") + class TestLog: def test_empty_when_not_initialized(self, git): diff --git a/tests/agent/test_runner_governance.py b/tests/agent/test_runner_governance.py index 7acbb684..f4fe2965 100644 --- a/tests/agent/test_runner_governance.py +++ b/tests/agent/test_runner_governance.py @@ -58,17 +58,11 @@ def _make_loop(tmp_path): return loop -async def test_runner_uses_raw_messages_when_context_governance_fails(): +async def test_runner_propagates_context_governance_failure(): from nanobot.agent.runner import AgentRunner provider = MagicMock() - captured_messages: list[dict] = [] - - async def chat_with_retry(*, messages, **kwargs): - captured_messages[:] = messages - return LLMResponse(content="done", tool_calls=[], usage={}) - - provider.chat_with_retry = chat_with_retry + provider.chat_with_retry = AsyncMock() tools = MagicMock() tools.get_definitions.return_value = [] initial_messages = [ @@ -80,16 +74,16 @@ async def test_runner_uses_raw_messages_when_context_governance_fails(): runner.context_governor.prepare_for_model = MagicMock( # type: ignore[method-assign] side_effect=RuntimeError("boom") ) - result = await runner.run(make_run_spec(provider, - initial_messages=initial_messages, - tools=tools, - model="test-model", - max_iterations=1, - max_tool_result_chars=_MAX_TOOL_RESULT_CHARS, - )) + with pytest.raises(RuntimeError, match="boom"): + await runner.run(make_run_spec(provider, + initial_messages=initial_messages, + tools=tools, + model="test-model", + max_iterations=1, + max_tool_result_chars=_MAX_TOOL_RESULT_CHARS, + )) - assert result.final_content == "done" - assert captured_messages == initial_messages + provider.chat_with_retry.assert_not_awaited() def test_snip_history_drops_orphaned_tool_results_from_trimmed_slice(monkeypatch): diff --git a/tests/agent/test_runner_tool_execution.py b/tests/agent/test_runner_tool_execution.py index 57f6bfa0..3d2a85d4 100644 --- a/tests/agent/test_runner_tool_execution.py +++ b/tests/agent/test_runner_tool_execution.py @@ -149,6 +149,30 @@ def _tool_message(result, tool_call_id: str) -> dict: ][0] +@pytest.mark.asyncio +async def test_runner_propagates_tool_preparation_failure(): + tools = MagicMock() + tools.prepare_call.side_effect = RuntimeError("tool preparation failed") + tools.execute = AsyncMock() + + with pytest.raises(RuntimeError, match="tool preparation failed"): + await AgentRunner()._run_tool( + make_run_spec( + MagicMock(), + initial_messages=[], + tools=tools, + model="test-model", + max_iterations=1, + max_tool_result_chars=_MAX_TOOL_RESULT_CHARS, + ), + ToolCallRequest(id="call-1", name="demo", arguments={}), + {}, + {}, + ) + + tools.execute.assert_not_awaited() + + @pytest.mark.asyncio async def test_runner_batches_read_only_tools_before_exclusive_work(): tools = ToolRegistry() diff --git a/tests/agent/test_unified_session.py b/tests/agent/test_unified_session.py index 8f436b3b..8ee5e723 100644 --- a/tests/agent/test_unified_session.py +++ b/tests/agent/test_unified_session.py @@ -349,6 +349,7 @@ class TestConsolidationUnaffectedByUnifiedSession: session = Session(key="unified:default") session.messages = [] + sessions.get_or_create.return_value = session await consolidator.maybe_consolidate_by_tokens(session, runtime=runtime) @@ -378,6 +379,7 @@ class TestConsolidationUnaffectedByUnifiedSession: session = Session(key=key) session.messages = [] # empty → exits immediately for both keys + sessions.get_or_create.return_value = session consolidator.archive = AsyncMock() await consolidator.maybe_consolidate_by_tokens( diff --git a/tests/channels/test_channel_plugins.py b/tests/channels/test_channel_plugins.py index df7bac3b..8399242f 100644 --- a/tests/channels/test_channel_plugins.py +++ b/tests/channels/test_channel_plugins.py @@ -2508,6 +2508,36 @@ def test_optional_dependency_groups_falls_back_to_package_metadata(monkeypatch): ) +def test_load_pyproject_propagates_malformed_toml(tmp_path): + from nanobot import optional_features + + path = tmp_path / "pyproject.toml" + path.write_text("[project\nname = 'nanobot'", encoding="utf-8") + + with pytest.raises(tomllib.TOMLDecodeError): + optional_features.load_pyproject(path) + + +def test_optional_dependency_metadata_propagates_malformed_requirement(monkeypatch): + from packaging.requirements import InvalidRequirement + + from nanobot import optional_features + + class _Metadata: + def get_all(self, key: str): + assert key == "Provides-Extra" + return ["bedrock"] + + monkeypatch.setattr("importlib.metadata.metadata", lambda _name: _Metadata()) + monkeypatch.setattr( + "importlib.metadata.requires", + lambda _name: ["not a valid requirement ???"], + ) + + with pytest.raises(InvalidRequirement): + optional_features.optional_dependency_groups_from_metadata() + + def test_install_args_for_extra_resolves_metadata_markers_for_current_platform(): from nanobot import optional_features diff --git a/tests/config/test_model_presets.py b/tests/config/test_model_presets.py index aa8c192e..0805a456 100644 --- a/tests/config/test_model_presets.py +++ b/tests/config/test_model_presets.py @@ -16,6 +16,11 @@ def test_resolve_preset_returns_defaults_when_no_preset() -> None: assert resolved.reasoning_effort == config.agents.defaults.reasoning_effort +def test_agent_timezone_rejects_unknown_iana_name() -> None: + with pytest.raises(ValueError, match="unknown timezone"): + Config.model_validate({"agents": {"defaults": {"timezone": "Not/AZone"}}}) + + def test_provider_api_type_accepts_exact_values_only() -> None: config = Config.model_validate({ "providers": { diff --git a/tests/session/test_session_fsync.py b/tests/session/test_session_fsync.py index ba18581b..b559a02a 100644 --- a/tests/session/test_session_fsync.py +++ b/tests/session/test_session_fsync.py @@ -170,3 +170,27 @@ class TestFlushAll: assert len(history) == 2 assert history[0]["content"] == "remember this" assert history[1]["content"] == "noted" + + +class TestLoadErrors: + @pytest.mark.parametrize( + "operation", + ("get_or_create", "read_session_file", "read_session_metadata", "list_sessions"), + ) + def test_permission_error_is_not_treated_as_corrupt_data( + self, + sessions_dir: Path, + operation: str, + ) -> None: + writer = SessionManager(workspace=sessions_dir) + session = writer.get_or_create("test:permission") + session.add_message("user", "must not disappear") + writer.save(session) + + reader = SessionManager(workspace=sessions_dir) + with patch("builtins.open", side_effect=PermissionError("access denied")): + with pytest.raises(PermissionError, match="access denied"): + if operation == "list_sessions": + reader.list_sessions() + else: + getattr(reader, operation)("test:permission") diff --git a/tests/test_openai_api.py b/tests/test_openai_api.py index 4c2056cf..f557c19b 100644 --- a/tests/test_openai_api.py +++ b/tests/test_openai_api.py @@ -421,65 +421,7 @@ async def test_multimodal_remote_image_url_returns_400(aiohttp_client, mock_agen @pytest.mark.skipif(not HAS_AIOHTTP, reason="aiohttp not installed") @pytest.mark.asyncio -async def test_empty_response_retry_then_success(aiohttp_client) -> None: - call_count = 0 - - async def sometimes_empty(content, session_key="", channel="", chat_id="", **kwargs): - nonlocal call_count - call_count += 1 - if call_count == 1: - return "" - return "recovered response" - - agent = MagicMock() - agent.process_direct = sometimes_empty - agent._connect_mcp = AsyncMock() - agent.close_mcp = AsyncMock() - agent._last_usage = {} - - app = create_app(agent, model_name="m", api_key=API_KEY) - client = await aiohttp_client(app) - resp = await client.post( - "/v1/chat/completions", - headers=AUTH_HEADERS, - json={"messages": [{"role": "user", "content": "hello"}]}, - ) - assert resp.status == 200 - body = await resp.json() - assert body["choices"][0]["message"]["content"] == "recovered response" - assert call_count == 2 - - -@pytest.mark.skipif(not HAS_AIOHTTP, reason="aiohttp not installed") -@pytest.mark.asyncio -async def test_empty_response_retry_does_not_duplicate_user_turn(aiohttp_client) -> None: - persist_flags = [] - - async def record(content, session_key="", channel="", chat_id="", **kwargs): - persist_flags.append(kwargs.get("persist_user_message", True)) - return "" if len(persist_flags) == 1 else "recovered response" - - agent = MagicMock() - agent.process_direct = record - agent._connect_mcp = AsyncMock() - agent.close_mcp = AsyncMock() - agent._last_usage = {} - - app = create_app(agent, model_name="m", api_key=API_KEY) - client = await aiohttp_client(app) - resp = await client.post( - "/v1/chat/completions", - headers=AUTH_HEADERS, - json={"messages": [{"role": "user", "content": "hello"}]}, - ) - assert resp.status == 200 - # first call persists the user turn; the retry must not persist it again - assert persist_flags == [True, False] - - -@pytest.mark.skipif(not HAS_AIOHTTP, reason="aiohttp not installed") -@pytest.mark.asyncio -async def test_empty_response_falls_back(aiohttp_client) -> None: +async def test_empty_response_falls_back_without_retry(aiohttp_client) -> None: from nanobot.utils.runtime import EMPTY_FINAL_RESPONSE_MESSAGE call_count = 0 @@ -505,7 +447,7 @@ async def test_empty_response_falls_back(aiohttp_client) -> None: assert resp.status == 200 body = await resp.json() assert body["choices"][0]["message"]["content"] == EMPTY_FINAL_RESPONSE_MESSAGE - assert call_count == 2 + assert call_count == 1 @pytest.mark.asyncio diff --git a/tests/utils/test_gitstore.py b/tests/utils/test_gitstore.py index 5611f3dd..c5b7c16f 100644 --- a/tests/utils/test_gitstore.py +++ b/tests/utils/test_gitstore.py @@ -7,7 +7,7 @@ from unittest.mock import patch import pytest -from nanobot.utils.gitstore import GitStore +from nanobot.utils.gitstore import GitStore, GitStoreError @pytest.fixture @@ -63,11 +63,13 @@ class TestLineAges: assert len(ages) == 2 assert all(a.age_days == 30 for a in ages) - def test_annotate_failure_returns_empty(self, tmp_path): - """If annotate fails, line_ages should return [] gracefully.""" - git = GitStore(tmp_path, tracked_files=["MEMORY.md"]) - # Don't init — annotate will fail - assert git.line_ages("MEMORY.md") == [] + def test_annotate_failure_is_explicit(self, git, tmp_path): + (tmp_path / "MEMORY.md").write_text("important\n", encoding="utf-8") + git.auto_commit("initial") + + with patch("dulwich.porcelain.annotate", side_effect=OSError("broken repo")): + with pytest.raises(GitStoreError, match="annotation failed"): + git.line_ages("MEMORY.md") def test_partial_edit_only_updates_changed_lines(self, git, tmp_path): """Only modified lines should reflect the new commit's timestamp.""" diff --git a/tests/utils/test_helpers.py b/tests/utils/test_helpers.py index 3d7daf96..b3a95f72 100644 --- a/tests/utils/test_helpers.py +++ b/tests/utils/test_helpers.py @@ -1,9 +1,16 @@ from pathlib import Path +from zoneinfo import ZoneInfoNotFoundError +import pytest import tiktoken from nanobot.utils import helpers -from nanobot.utils.helpers import _write_text_atomic, split_message, truncate_text_to_tokens +from nanobot.utils.helpers import ( + _write_text_atomic, + current_time_str, + split_message, + truncate_text_to_tokens, +) def test_split_message_no_code_blocks_unchanged(): @@ -43,6 +50,11 @@ def test_truncate_text_to_tokens_non_positive_budget_returns_text(): assert truncate_text_to_tokens(text, 0) == text +def test_current_time_str_rejects_unknown_timezone(): + with pytest.raises(ZoneInfoNotFoundError): + current_time_str("Not/AZone") + + def test_write_text_atomic_fsyncs_file_and_parent_directory( tmp_path: Path, monkeypatch ) -> None: diff --git a/tests/utils/test_token_estimation.py b/tests/utils/test_token_estimation.py index 254bf530..f3949ad4 100644 --- a/tests/utils/test_token_estimation.py +++ b/tests/utils/test_token_estimation.py @@ -1,7 +1,12 @@ import json from nanobot.utils import helpers -from nanobot.utils.helpers import estimate_prompt_tokens, estimate_prompt_tokens_chain +from nanobot.utils.helpers import ( + estimate_message_tokens, + estimate_prompt_tokens, + estimate_prompt_tokens_chain, + truncate_text_to_tokens, +) class _NoCounterProvider: @@ -35,6 +40,57 @@ def test_estimate_prompt_tokens_chain_falls_back_when_provider_counter_fails() - assert source == "tiktoken" +def test_estimate_prompt_tokens_uses_conservative_fallback_when_tiktoken_fails( + monkeypatch, +) -> None: + monkeypatch.setattr( + helpers, + "_get_token_encoding", + lambda: (_ for _ in ()).throw(RuntimeError("encoding unavailable")), + ) + + content = "你" * 1_000 + messages = [{"role": "user", "content": content}] + tokens = estimate_prompt_tokens(messages) + chain_tokens, source = estimate_prompt_tokens_chain( + _NoCounterProvider(), + "test-model", + messages, + ) + + actual_tokens = len(helpers.tiktoken.get_encoding("cl100k_base").encode(content)) + 4 + assert tokens == len(content.encode("utf-8")) + 4 + assert tokens >= actual_tokens + assert chain_tokens == tokens + assert source == "heuristic" + + +def test_estimate_message_tokens_uses_utf8_byte_fallback(monkeypatch) -> None: + monkeypatch.setattr( + helpers, + "_get_token_encoding", + lambda: (_ for _ in ()).throw(RuntimeError("encoding unavailable")), + ) + content = "🙂你" * 100 + + assert estimate_message_tokens({"role": "user", "content": content}) == ( + len(content.encode("utf-8")) + 4 + ) + + +def test_truncate_text_to_tokens_uses_utf8_byte_budget_fallback(monkeypatch) -> None: + monkeypatch.setattr( + helpers, + "_get_token_encoding", + lambda: (_ for _ in ()).throw(RuntimeError("encoding unavailable")), + ) + + result = truncate_text_to_tokens("🙂你" * 100, 40) + + assert result.endswith("\n... (truncated)") + assert len(result.encode("utf-8")) <= 40 + + def test_estimate_prompt_tokens_caches_tools_encoding(monkeypatch) -> None: helpers._get_token_encoding.cache_clear() helpers._TOOLS_TOKEN_CACHE.clear() diff --git a/tests/webui/test_settings_api.py b/tests/webui/test_settings_api.py index b20366bd..42f628d7 100644 --- a/tests/webui/test_settings_api.py +++ b/tests/webui/test_settings_api.py @@ -34,6 +34,21 @@ DYNAMIC_PROVIDER_NAME = "my-company-api" DYNAMIC_PROVIDER_API_BASE = "https://example.test/v1" +def test_settings_payload_propagates_preset_resolution_failure( + monkeypatch: pytest.MonkeyPatch, +) -> None: + config = Config() + monkeypatch.setattr("nanobot.webui.settings_api.load_config", lambda: config) + monkeypatch.setattr( + Config, + "resolve_preset", + lambda _self: (_ for _ in ()).throw(RuntimeError("invalid preset")), + ) + + with pytest.raises(RuntimeError, match="invalid preset"): + settings_payload() + + def test_docs_version_uses_released_versions_and_falls_back_for_dev() -> None: assert _docs_version("0.2.3") == "0.2.3" assert _docs_version("0.2.3.post1") == "0.2.3.post1"