From f3a064df1cbd1fe0e1bc09fdb934c945a5a093a3 Mon Sep 17 00:00:00 2001 From: yc111233 Date: Mon, 6 Apr 2026 01:02:35 +0800 Subject: [PATCH 1/3] fix: address 8 runtime bugs found during code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. Race condition on _addressed_degradations (evolver.py) — add asyncio.Lock 2. Silent exception swallowing in wait_background (evolver.py) — log failures 3. Workspace cleanup could delete user files (tool_layer.py) — add mtime guard 4. WAL cleanup without lock check (store.py) — probe for DB lock first 5. Edit distance threshold too loose (analyzer.py) — adaptive threshold + ambiguity rejection 6. Message truncation drops context (grounding_agent.py) — add truncation notice 7. Whitespace-only empty response not detected (grounding_agent.py) — strip before check 8. Tool name reverse parsing with __ (client.py, manager.py) — use rsplit --- openspace/agents/grounding_agent.py | 11 ++++++-- openspace/llm/client.py | 5 ++-- openspace/recording/manager.py | 3 ++- openspace/skill_engine/analyzer.py | 10 +++++--- openspace/skill_engine/evolver.py | 13 +++++++++- openspace/skill_engine/store.py | 16 ++++++++++++ openspace/tool_layer.py | 40 +++++++++++++++++++++-------- 7 files changed, 78 insertions(+), 20 deletions(-) diff --git a/openspace/agents/grounding_agent.py b/openspace/agents/grounding_agent.py index 72a4fe0..bafb64b 100644 --- a/openspace/agents/grounding_agent.py +++ b/openspace/agents/grounding_agent.py @@ -185,10 +185,17 @@ class GroundingAgent(BaseAgent): conversation_messages.append(msg) recent_messages = conversation_messages[-(keep_recent * 2):] if conversation_messages else [] - + truncated = system_messages.copy() if user_instruction: truncated.append(user_instruction) + dropped = len(conversation_messages) - len(recent_messages) + if dropped > 0: + truncated.append({ + "role": "user", + "content": f"[System: {dropped} earlier messages were truncated to save context. " + f"The original task instruction is preserved above.]" + }) truncated.extend(recent_messages) logger.info(f"After truncation: {len(truncated)} messages, " @@ -350,7 +357,7 @@ class GroundingAgent(BaseAgent): f"Tool results: {len(tool_results_this_iteration)}, " f"Content length: {len(assistant_content)} chars") - if len(assistant_content) > 0: + if len(assistant_content.strip()) > 0: logger.info(f"Iteration {current_iteration} - Assistant content preview: {repr(assistant_content[:300])}") consecutive_empty_responses = 0 # Reset counter on valid response else: diff --git a/openspace/llm/client.py b/openspace/llm/client.py index c7fd696..0c11c5e 100644 --- a/openspace/llm/client.py +++ b/openspace/llm/client.py @@ -169,9 +169,10 @@ def _infer_backend_from_tool_name(tool_name: str) -> Optional[str]: if not tool_name or not isinstance(tool_name, str): return None name = tool_name.strip() - # Dedup format: "server__toolname" -> use suffix + # Dedup format: "server__toolname" -> use suffix. + # Use rsplit to handle server names that themselves contain "__". if "__" in name: - name = name.split("__", 1)[-1] + name = name.rsplit("__", 1)[-1] shell_tools = {"shell_agent", "read_file", "write_file", "list_dir", "run_shell"} if name in shell_tools: return "shell" diff --git a/openspace/recording/manager.py b/openspace/recording/manager.py index fd6d7ed..4bbb382 100644 --- a/openspace/recording/manager.py +++ b/openspace/recording/manager.py @@ -668,8 +668,9 @@ class RecordingManager: if not tool_name or not isinstance(tool_name, str): return None name = tool_name.strip() + # Use rsplit to handle server names that themselves contain "__". if "__" in name: - name = name.split("__", 1)[-1] + name = name.rsplit("__", 1)[-1] shell_tools = {"shell_agent", "read_file", "write_file", "list_dir", "run_shell"} if name in shell_tools: return "shell" diff --git a/openspace/skill_engine/analyzer.py b/openspace/skill_engine/analyzer.py index fa5b74f..a809bd7 100644 --- a/openspace/skill_engine/analyzer.py +++ b/openspace/skill_engine/analyzer.py @@ -84,13 +84,17 @@ def _correct_skill_ids( if prefix and k.split("__")[0] == prefix ] - best, best_dist = None, 4 # threshold: edit distance ≤ 3 + # Adaptive threshold: tighten when many candidates share the prefix + max_dist = 2 if len(candidates) > 20 else 4 # ≤1 or ≤3 + best, best_dist, ambiguous = None, max_dist, False for cand in candidates: d = _edit_distance(raw_id, cand) if d < best_dist: - best, best_dist = cand, d + best, best_dist, ambiguous = cand, d, False + elif d == best_dist and cand != best: + ambiguous = True # multiple candidates at same distance - if best is not None: + if best is not None and not ambiguous: logger.info( f"Corrected LLM skill ID: {raw_id!r} → {best!r} " f"(edit_distance={best_dist})" diff --git a/openspace/skill_engine/evolver.py b/openspace/skill_engine/evolver.py index ad9afdc..818d8bf 100644 --- a/openspace/skill_engine/evolver.py +++ b/openspace/skill_engine/evolver.py @@ -201,6 +201,7 @@ class SkillEvolver: # evolved for each degraded tool. Keyed by tool_key. # Pruned when a tool leaves the problematic list (= recovered). self._addressed_degradations: Dict[str, Set[str]] = {} + self._degradation_lock = asyncio.Lock() # Track background tasks so they can be awaited on shutdown. self._background_tasks: Set[asyncio.Task] = set() @@ -219,7 +220,10 @@ class SkillEvolver: f"Waiting for {len(self._background_tasks)} background " f"evolution task(s) to finish..." ) - await asyncio.gather(*self._background_tasks, return_exceptions=True) + results = await asyncio.gather(*self._background_tasks, return_exceptions=True) + for r in results: + if isinstance(r, BaseException): + logger.warning(f"Background evolution task failed during shutdown: {r}") self._background_tasks.clear() async def evolve(self, ctx: EvolutionContext) -> Optional[SkillRecord]: @@ -306,6 +310,13 @@ class SkillEvolver: if not problematic_tools: return [] + async with self._degradation_lock: + return await self._process_tool_degradation_locked(problematic_tools) + + async def _process_tool_degradation_locked( + self, problematic_tools: List, + ) -> List[SkillRecord]: + """Inner body of process_tool_degradation, called under _degradation_lock.""" # Prune recovered tools: if a tool_key used to be tracked but is # no longer in the current problematic list, it recovered — clear # its addressed set so future re-degradation gets a fresh pass. diff --git a/openspace/skill_engine/store.py b/openspace/skill_engine/store.py index f7c7ee5..60d68a9 100644 --- a/openspace/skill_engine/store.py +++ b/openspace/skill_engine/store.py @@ -242,6 +242,9 @@ class SkillStore: If the main DB file is empty (0 bytes) but WAL/SHM companions exist, the database is unrecoverable — delete the companions so SQLite can start fresh. + + Safety: check that no other process currently has the DB open + before removing WAL/SHM, to avoid corrupting concurrent writes. """ if not self._db_path.exists(): return @@ -250,6 +253,19 @@ class SkillStore: if self._db_path.stat().st_size == 0 and ( wal.exists() or shm.exists() ): + # Verify no other process holds the database open. + import sqlite3 + try: + test_conn = sqlite3.connect(str(self._db_path), timeout=0.1) + test_conn.execute("PRAGMA journal_mode") + test_conn.close() + except sqlite3.OperationalError: + logger.info( + "DB appears locked by another process — " + "skipping WAL/SHM cleanup to avoid corruption" + ) + return + logger.warning( "Empty DB with WAL/SHM — removing for crash recovery" ) diff --git a/openspace/tool_layer.py b/openspace/tool_layer.py index 4cf4b7b..4d30f25 100644 --- a/openspace/tool_layer.py +++ b/openspace/tool_layer.py @@ -429,15 +429,24 @@ class OpenSpace: execution_context_p1 = {**execution_context} execution_context_p1["max_iterations"] = max_iterations - # Snapshot workspace files before skill-guided execution + # Snapshot workspace files before skill-guided execution. + # Record names AND mtimes so cleanup only removes files + # that were actually created during the skill phase. workspace_path = execution_context.get("workspace_dir", "") - pre_skill_files: set = set() + pre_skill_files: Dict[str, float] = {} + snapshot_time: float = 0.0 if workspace_path: try: from pathlib import Path as _P - pre_skill_files = { - f.name for f in _P(workspace_path).iterdir() - } if _P(workspace_path).exists() else set() + import time as _time + snapshot_time = _time.time() + ws_p = _P(workspace_path) + if ws_p.exists(): + for f in ws_p.iterdir(): + try: + pre_skill_files[f.name] = f.stat().st_mtime + except OSError: + pre_skill_files[f.name] = 0.0 except Exception: pass @@ -478,12 +487,21 @@ class OpenSpace: removed = 0 if ws.exists(): for f in list(ws.iterdir()): - if f.name not in pre_skill_files: - if f.is_dir(): - shutil.rmtree(f, ignore_errors=True) - else: - f.unlink(missing_ok=True) - removed += 1 + # Keep files that existed before skill phase + if f.name in pre_skill_files: + continue + # Keep files whose mtime predates the snapshot + # (created by external processes before we started) + try: + if snapshot_time and f.stat().st_mtime < snapshot_time: + continue + except OSError: + pass + if f.is_dir(): + shutil.rmtree(f, ignore_errors=True) + else: + f.unlink(missing_ok=True) + removed += 1 if removed: logger.info( f"[Phase 2 — Fallback] Cleaned {removed} artifact(s) " From a3a73406a5804f98691ffa351e60d7f1580c6d74 Mon Sep 17 00:00:00 2001 From: Dennis-yxchen Date: Mon, 6 Apr 2026 19:18:22 +0800 Subject: [PATCH 2/3] fix: resolve pr-60 review regressions --- openspace/__main__.py | 10 +++++-- openspace/agents/grounding_agent.py | 16 +++++++++-- openspace/host_detection/__init__.py | 9 +++++- openspace/host_detection/openclaw.py | 8 +++++- openspace/host_detection/resolver.py | 9 ++++-- openspace/llm/client.py | 5 ++-- openspace/mcp_server.py | 8 +++++- openspace/tool_layer.py | 42 ++++++++-------------------- 8 files changed, 65 insertions(+), 42 deletions(-) diff --git a/openspace/__main__.py b/openspace/__main__.py index 4a1d6e0..153eebc 100644 --- a/openspace/__main__.py +++ b/openspace/__main__.py @@ -322,7 +322,13 @@ async def refresh_mcp_cache(config_path: Optional[str] = None): def _load_config(args) -> OpenSpaceConfig: """Load configuration""" import os - from openspace.host_detection import build_llm_kwargs, build_grounding_config_path + from openspace.host_detection import ( + build_grounding_config_path, + build_llm_kwargs, + load_runtime_env, + ) + + load_runtime_env() cli_overrides = {} if args.max_iterations is not None: @@ -494,4 +500,4 @@ def run_main(): if __name__ == "__main__": - run_main() \ No newline at end of file + run_main() diff --git a/openspace/agents/grounding_agent.py b/openspace/agents/grounding_agent.py index bafb64b..b75b5f3 100644 --- a/openspace/agents/grounding_agent.py +++ b/openspace/agents/grounding_agent.py @@ -754,9 +754,19 @@ class GroundingAgent(BaseAgent): } }) - # Use dedicated visual analysis model if configured, otherwise use main LLM model + # Resolve visual-model credentials independently when the visual + # model differs from the main reasoning model. visual_model = self._visual_analysis_model or (self._llm_client.model if self._llm_client else "openrouter/anthropic/claude-sonnet-4.5") - _llm_extra = getattr(self._llm_client, 'litellm_kwargs', {}) if self._llm_client else {} + _llm_extra = {} + if self._llm_client and visual_model == self._llm_client.model: + _llm_extra = getattr(self._llm_client, 'litellm_kwargs', {}) or {} + elif self._visual_analysis_model: + try: + from openspace.host_detection import build_llm_kwargs + visual_model, _llm_extra = build_llm_kwargs(visual_model) + except Exception as e: + logger.debug(f"Failed to resolve dedicated visual model credentials: {e}") + _llm_extra = {} response = await asyncio.wait_for( litellm.acompletion( model=visual_model, @@ -1225,4 +1235,4 @@ class GroundingAgent(BaseAgent): "step": self.step, "instruction": instruction, } - ) \ No newline at end of file + ) diff --git a/openspace/host_detection/__init__.py b/openspace/host_detection/__init__.py index d8125b3..cb31b95 100644 --- a/openspace/host_detection/__init__.py +++ b/openspace/host_detection/__init__.py @@ -21,7 +21,11 @@ Supported host agents: import logging from typing import Dict, Optional -from openspace.host_detection.resolver import build_llm_kwargs, build_grounding_config_path +from openspace.host_detection.resolver import ( + build_grounding_config_path, + build_llm_kwargs, + load_runtime_env, +) from openspace.host_detection.nanobot import ( get_openai_api_key as _nanobot_get_openai_api_key, read_nanobot_mcp_env, @@ -29,6 +33,7 @@ from openspace.host_detection.nanobot import ( ) from openspace.host_detection.openclaw import ( get_openclaw_openai_api_key as _openclaw_get_openai_api_key, + is_openclaw_host, read_openclaw_skill_env, try_read_openclaw_config, ) @@ -80,10 +85,12 @@ def get_openai_api_key() -> Optional[str]: __all__ = [ "build_llm_kwargs", "build_grounding_config_path", + "load_runtime_env", "get_openai_api_key", "read_host_mcp_env", "read_nanobot_mcp_env", "try_read_nanobot_config", + "is_openclaw_host", "read_openclaw_skill_env", "try_read_openclaw_config", ] diff --git a/openspace/host_detection/openclaw.py b/openspace/host_detection/openclaw.py index a97104a..6a109e0 100644 --- a/openspace/host_detection/openclaw.py +++ b/openspace/host_detection/openclaw.py @@ -279,6 +279,13 @@ def get_openclaw_openai_api_key() -> Optional[str]: return None +def is_openclaw_host() -> bool: + """Detect if the current environment is running under OpenClaw.""" + if os.environ.get("OPENCLAW_STATE_DIR") or os.environ.get("OPENCLAW_CONFIG_PATH"): + return True + return _resolve_openclaw_config_path() is not None + + def try_read_openclaw_config(model: str) -> Optional[Dict[str, Any]]: """Read LLM credentials from OpenClaw's env-style config blocks.""" env_block = _get_openclaw_env("openspace") @@ -306,4 +313,3 @@ def try_read_openclaw_config(model: str) -> Optional[Dict[str, Any]]: return result - diff --git a/openspace/host_detection/resolver.py b/openspace/host_detection/resolver.py index ee43408..612c044 100644 --- a/openspace/host_detection/resolver.py +++ b/openspace/host_detection/resolver.py @@ -62,6 +62,11 @@ def _load_env_once() -> None: load_dotenv() +def load_runtime_env() -> None: + """Public wrapper for one-time runtime .env loading.""" + _load_env_once() + + def _pick_first_env(names: tuple[str, ...]) -> str: for name in names: value = os.environ.get(name, "").strip() @@ -148,10 +153,10 @@ def build_llm_kwargs(model: str) -> tuple[str, Dict[str, Any]]: resolved_model or _DEFAULT_MODEL ) - # --- Tier 3: host config fallback (only when no local keys) --- + # --- Tier 3: host config fallback (blocked only by provider-native env) --- host_config = None host_source = None - if not has_explicit_llm_override and not provider_native_env_used: + if not provider_native_env_used: from openspace.host_detection.nanobot import try_read_nanobot_config host_config = try_read_nanobot_config(model) if host_config: diff --git a/openspace/llm/client.py b/openspace/llm/client.py index 0c11c5e..611cf87 100644 --- a/openspace/llm/client.py +++ b/openspace/llm/client.py @@ -9,8 +9,9 @@ from openspace.grounding.core.types import ToolSchema, ToolResult, ToolStatus from openspace.grounding.core.tool import BaseTool from openspace.utils.logging import Logger -# .env loading is centralized in host_detection.resolver._load_env_once() -# which is called by build_llm_kwargs / build_grounding_config_path. +# .env loading is centralized in host_detection.resolver.load_runtime_env(). +# CLI/MCP entrypoints call it before reading startup env vars, and the +# resolver helpers also call it defensively. # Disable LiteLLM verbose logging to prevent stdout blocking with large tool schemas litellm.set_verbose = False diff --git a/openspace/mcp_server.py b/openspace/mcp_server.py index 168485c..ba4ca31 100644 --- a/openspace/mcp_server.py +++ b/openspace/mcp_server.py @@ -137,7 +137,13 @@ async def _get_openspace(): logger.info("Initializing OpenSpace engine ...") from openspace.tool_layer import OpenSpace, OpenSpaceConfig - from openspace.host_detection import build_llm_kwargs, build_grounding_config_path + from openspace.host_detection import ( + build_grounding_config_path, + build_llm_kwargs, + load_runtime_env, + ) + + load_runtime_env() env_model = os.environ.get("OPENSPACE_MODEL", "") workspace = os.environ.get("OPENSPACE_WORKSPACE") diff --git a/openspace/tool_layer.py b/openspace/tool_layer.py index 4d30f25..c2eaca9 100644 --- a/openspace/tool_layer.py +++ b/openspace/tool_layer.py @@ -429,24 +429,15 @@ class OpenSpace: execution_context_p1 = {**execution_context} execution_context_p1["max_iterations"] = max_iterations - # Snapshot workspace files before skill-guided execution. - # Record names AND mtimes so cleanup only removes files - # that were actually created during the skill phase. + # Snapshot workspace files before skill-guided execution workspace_path = execution_context.get("workspace_dir", "") - pre_skill_files: Dict[str, float] = {} - snapshot_time: float = 0.0 + pre_skill_files: set = set() if workspace_path: try: from pathlib import Path as _P - import time as _time - snapshot_time = _time.time() - ws_p = _P(workspace_path) - if ws_p.exists(): - for f in ws_p.iterdir(): - try: - pre_skill_files[f.name] = f.stat().st_mtime - except OSError: - pre_skill_files[f.name] = 0.0 + pre_skill_files = { + f.name for f in _P(workspace_path).iterdir() + } if _P(workspace_path).exists() else set() except Exception: pass @@ -487,21 +478,12 @@ class OpenSpace: removed = 0 if ws.exists(): for f in list(ws.iterdir()): - # Keep files that existed before skill phase - if f.name in pre_skill_files: - continue - # Keep files whose mtime predates the snapshot - # (created by external processes before we started) - try: - if snapshot_time and f.stat().st_mtime < snapshot_time: - continue - except OSError: - pass - if f.is_dir(): - shutil.rmtree(f, ignore_errors=True) - else: - f.unlink(missing_ok=True) - removed += 1 + if f.name not in pre_skill_files: + if f.is_dir(): + shutil.rmtree(f, ignore_errors=True) + else: + f.unlink(missing_ok=True) + removed += 1 if removed: logger.info( f"[Phase 2 — Fallback] Cleaned {removed} artifact(s) " @@ -952,4 +934,4 @@ class OpenSpace: if self._running: status = "running" backends = ", ".join(self.config.backend_scope) if self.config.backend_scope else "all" - return f"" \ No newline at end of file + return f"" From a23792a66b8c956b9ade4f625e6142266f322789 Mon Sep 17 00:00:00 2001 From: Dennis-yxchen Date: Mon, 6 Apr 2026 20:00:16 +0800 Subject: [PATCH 3/3] fix: tighten pr-60 review follow-ups --- openspace/agents/grounding_agent.py | 12 +++++++----- openspace/host_detection/resolver.py | 4 ++-- openspace/skill_engine/store.py | 16 ---------------- 3 files changed, 9 insertions(+), 23 deletions(-) diff --git a/openspace/agents/grounding_agent.py b/openspace/agents/grounding_agent.py index b75b5f3..d64116c 100644 --- a/openspace/agents/grounding_agent.py +++ b/openspace/agents/grounding_agent.py @@ -187,15 +187,17 @@ class GroundingAgent(BaseAgent): recent_messages = conversation_messages[-(keep_recent * 2):] if conversation_messages else [] truncated = system_messages.copy() - if user_instruction: - truncated.append(user_instruction) dropped = len(conversation_messages) - len(recent_messages) if dropped > 0: truncated.append({ - "role": "user", - "content": f"[System: {dropped} earlier messages were truncated to save context. " - f"The original task instruction is preserved above.]" + "role": "system", + "content": ( + f"{self._ITERATION_GUIDANCE_PREFIX} {dropped} earlier messages were " + "truncated to save context. The original task instruction is preserved below." + ), }) + if user_instruction: + truncated.append(user_instruction) truncated.extend(recent_messages) logger.info(f"After truncation: {len(truncated)} messages, " diff --git a/openspace/host_detection/resolver.py b/openspace/host_detection/resolver.py index 612c044..90b095b 100644 --- a/openspace/host_detection/resolver.py +++ b/openspace/host_detection/resolver.py @@ -153,10 +153,10 @@ def build_llm_kwargs(model: str) -> tuple[str, Dict[str, Any]]: resolved_model or _DEFAULT_MODEL ) - # --- Tier 3: host config fallback (blocked only by provider-native env) --- + # --- Tier 3: host config fallback (only when no local keys) --- host_config = None host_source = None - if not provider_native_env_used: + if not has_explicit_llm_override and not provider_native_env_used: from openspace.host_detection.nanobot import try_read_nanobot_config host_config = try_read_nanobot_config(model) if host_config: diff --git a/openspace/skill_engine/store.py b/openspace/skill_engine/store.py index 60d68a9..f7c7ee5 100644 --- a/openspace/skill_engine/store.py +++ b/openspace/skill_engine/store.py @@ -242,9 +242,6 @@ class SkillStore: If the main DB file is empty (0 bytes) but WAL/SHM companions exist, the database is unrecoverable — delete the companions so SQLite can start fresh. - - Safety: check that no other process currently has the DB open - before removing WAL/SHM, to avoid corrupting concurrent writes. """ if not self._db_path.exists(): return @@ -253,19 +250,6 @@ class SkillStore: if self._db_path.stat().st_size == 0 and ( wal.exists() or shm.exists() ): - # Verify no other process holds the database open. - import sqlite3 - try: - test_conn = sqlite3.connect(str(self._db_path), timeout=0.1) - test_conn.execute("PRAGMA journal_mode") - test_conn.close() - except sqlite3.OperationalError: - logger.info( - "DB appears locked by another process — " - "skipping WAL/SHM cleanup to avoid corruption" - ) - return - logger.warning( "Empty DB with WAL/SHM — removing for crash recovery" )