mirror of
https://github.com/HKUDS/OpenSpace.git
synced 2026-10-08 03:07:51 +00:00
fix: address 8 runtime bugs found during code review
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
This commit is contained in:
parent
1bd1a3d377
commit
f3a064df1c
7 changed files with 78 additions and 20 deletions
|
|
@ -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:
|
||||
|
|
|
|||
|
|
@ -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"
|
||||
|
|
|
|||
|
|
@ -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"
|
||||
|
|
|
|||
|
|
@ -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})"
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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"
|
||||
)
|
||||
|
|
|
|||
|
|
@ -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) "
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue