Skip to content

Commit b0bfd3f

Browse files
Brian KrafftCopilot
andcommitted
feat(4.6): refactor OpenSpace into clean facade
Restructure initialize() from 190-line monolith into clean orchestration: - initialize(): 70-line high-level flow with numbered steps - _load_grounding_config(): config loading, backend scope resolution - _setup_skill_engine(): skill registry, store, analyzer, evolver Cleanup: - Remove dead imports (asyncio, uuid — now in ExecutionEngine) - Remove stale extraction comments (4.1/4.3 notes) - Update from_container docstring (remove Phase 4 TODO — it's done) Public API unchanged. All 1,356 tests pass. P4 tool_layer.py decomposition summary (788 → 477 lines, -39%): - 4.1: ToolRegistry (206 lines, PR #31) - 4.3: ExecutionEngine (459 lines, PR #32) - 4.4: RecordingService (70 lines, PR #33) - 4.5: LLMFactory (68 lines, PR #34) - 4.6: Facade cleanup (this PR) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 5537f7f commit b0bfd3f

1 file changed

Lines changed: 126 additions & 146 deletions

File tree

openspace/tool_layer.py

Lines changed: 126 additions & 146 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,5 @@
11
from __future__ import annotations
22

3-
import asyncio
4-
import uuid
53
from dataclasses import dataclass, field
64
from pathlib import Path
75
from typing import TYPE_CHECKING, Any, Dict, List, Optional
@@ -151,15 +149,7 @@ def from_container(
151149
container: AppContainer,
152150
config: Optional[OpenSpaceConfig] = None,
153151
) -> "OpenSpace":
154-
"""Create an OpenSpace instance backed by an AppContainer.
155-
156-
.. note::
157-
158-
Phase 1 only stores the container for property access.
159-
``initialize()`` still constructs its own services.
160-
Phase 4 will wire ``initialize()`` to resolve from the
161-
container, making injected services authoritative.
162-
"""
152+
"""Create an OpenSpace instance backed by an AppContainer."""
163153
return cls(config=config, container=container)
164154

165155
# ── Public property accessors (replace private field access) ──────
@@ -207,95 +197,30 @@ async def initialize(self) -> None:
207197
logger.info("Initializing OpenSpace...")
208198

209199
try:
200+
# 1. LLM clients
210201
self._llm_factory = LLMFactory(config=self.config)
211202
self._llm_client = self._llm_factory.create_main()
212203
logger.info(f"✓ LLM Client: {self.config.llm_model}")
213204

214-
# Load grounding config
215-
# If custom config is provided, merge it with default configs
216-
# load_config supports multiple files and deep merges them (later files override earlier ones)
217-
if self.config.grounding_config_path:
218-
from openspace.config.constants import CONFIG_GROUNDING, CONFIG_SECURITY
219-
from openspace.config.loader import CONFIG_DIR
220-
221-
# Load default configs + custom config (custom values will override defaults)
222-
grounding_config = load_config(
223-
CONFIG_DIR / CONFIG_GROUNDING, CONFIG_DIR / CONFIG_SECURITY, self.config.grounding_config_path
224-
)
225-
logger.info(f"Merged custom grounding config: {self.config.grounding_config_path}")
226-
else:
227-
# Load default configs only
228-
grounding_config = get_config()
229-
230-
# Optional: enable ClawWork productivity tools for fair benchmark comparison
231-
if getattr(self.config, "use_clawwork_productivity", False):
232-
shell_cfg = grounding_config.shell.model_copy(
233-
update={
234-
"use_clawwork_productivity": True,
235-
"working_dir": self.config.workspace_dir or grounding_config.shell.working_dir,
236-
}
237-
)
238-
grounding_config = grounding_config.model_copy(update={"shell": shell_cfg})
239-
logger.info("ClawWork productivity tools enabled (shell.working_dir used as sandbox root)")
240-
241-
# Resolve backend_scope early so we can skip initializing
242-
# providers that are not in scope (e.g. web when only shell is needed).
243-
agent_config = get_agent_config("GroundingAgent")
244-
_cli_max_iter = self.config.grounding_max_iterations
245-
_default_max_iter = OpenSpaceConfig().grounding_max_iterations # dataclass default (20)
246-
if agent_config:
247-
cfg_max_iter = agent_config.get("max_iterations", _default_max_iter)
248-
if _cli_max_iter != _default_max_iter:
249-
max_iterations = _cli_max_iter
250-
else:
251-
max_iterations = cfg_max_iter
252-
backend_scope = (
253-
self.config.backend_scope
254-
or agent_config.get("backend_scope")
255-
or ["gui", "shell", "mcp", "web", "system"]
256-
)
257-
visual_analysis_timeout = agent_config.get("visual_analysis_timeout", 30.0)
258-
self.config.grounding_max_iterations = max_iterations
259-
logger.info(
260-
f"Loaded GroundingAgent config from config_agents.json (max_iterations={max_iterations}, visual_analysis_timeout={visual_analysis_timeout}s)"
261-
)
262-
else:
263-
max_iterations = self.config.grounding_max_iterations
264-
backend_scope = self.config.backend_scope or ["gui", "shell", "mcp", "web", "system"]
265-
visual_analysis_timeout = 30.0
266-
logger.warning(f"config_agents.json not found, using default config (max_iterations={max_iterations})")
267-
268-
# Filter enabled_backends in grounding config to only those in scope,
269-
# so providers outside scope (e.g. web) are never registered/initialized.
270-
if grounding_config.enabled_backends:
271-
scope_set = set(backend_scope)
272-
filtered = [
273-
entry for entry in grounding_config.enabled_backends if entry.get("name", "").lower() in scope_set
274-
]
275-
if len(filtered) != len(grounding_config.enabled_backends):
276-
skipped = [
277-
entry.get("name")
278-
for entry in grounding_config.enabled_backends
279-
if entry.get("name", "").lower() not in scope_set
280-
]
281-
logger.info(f"Skipping backends not in scope: {skipped}")
282-
grounding_config = grounding_config.model_copy(update={"enabled_backends": filtered})
283-
205+
# 2. Grounding config + client
206+
grounding_config, backend_scope, max_iterations, visual_analysis_timeout = (
207+
self._load_grounding_config()
208+
)
284209
self._grounding_config = grounding_config
285210
self._grounding_client = GroundingClient(config=grounding_config)
286211
await self._grounding_client.initialize_all_providers()
287-
288212
backends = list(self._grounding_client.list_providers().keys())
289213
logger.info(f"✓ Grounding Client: {len(backends)} backends")
290214
logger.debug(f" Available backends: {[b.value for b in backends]}")
291215

216+
# 3. Recording
292217
if self.config.enable_recording:
293218
self._recording_service = RecordingService(config=self.config)
294219
self._recording_manager = self._recording_service.create(llm_client=self._llm_client)
295220
self._recording_service.wire(grounding_client=self._grounding_client)
296221
logger.info(f"✓ Recording enabled: {len(self._recording_manager.backends or [])} backends")
297222

298-
# Create separate LLM client for tool retrieval if configured
223+
# 4. Tool retrieval LLM + GroundingAgent
299224
tool_retrieval_llm = self._llm_factory.create_tool_retrieval()
300225
if tool_retrieval_llm:
301226
logger.info(f"✓ Tool retrieval LLM: {self.config.tool_retrieval_model}")
@@ -314,62 +239,10 @@ async def initialize(self) -> None:
314239
)
315240
logger.info(f"✓ GroundingAgent: {', '.join(backend_scope)}")
316241

317-
# Initialize SkillRegistry (settings from config_grounding.json → skills)
318-
if self._grounding_config and self._grounding_config.skills.enabled:
319-
self._tool_registry = ToolRegistry(
320-
config=self.config,
321-
grounding_config=self._grounding_config,
322-
llm_client=self._llm_client,
323-
)
324-
if self._tool_registry.discover():
325-
self._skill_registry = self._tool_registry.registry
326-
skills = self._skill_registry.list_skills()
327-
logger.info(f"✓ Skills: {len(skills)} discovered")
328-
self._grounding_agent.set_skill_registry(self._skill_registry)
329-
else:
330-
self._skill_registry = None
331-
332-
# Initialize ExecutionAnalyzer (requires recording + skills)
333-
if self.config.enable_recording and self._skill_registry:
334-
try:
335-
skill_store = SkillStore()
336-
self._skill_store = skill_store # Expose for MCP server reuse
337-
338-
# Sync filesystem skills → DB (creates initial records
339-
# for newly discovered skills so that analysis stats
340-
# can be recorded against them from the very first run).
341-
await skill_store.sync_from_registry(self._skill_registry.list_skills())
342-
343-
# Bridge: pass quality_manager so analysis can feed back
344-
# LLM-identified tool issues to the tool quality system.
345-
quality_mgr = self._grounding_client.quality_manager if self._grounding_client else None
346-
self._execution_analyzer = ExecutionAnalyzer(
347-
store=skill_store,
348-
llm_client=self._llm_client,
349-
model=self.config.execution_analyzer_model,
350-
skill_registry=self._skill_registry,
351-
quality_manager=quality_mgr,
352-
)
353-
logger.info("✓ Execution analysis enabled")
354-
355-
# Share store with GroundingAgent so retrieve_skill
356-
# can access quality metrics for LLM selection.
357-
self._grounding_agent._skill_store = skill_store
358-
359-
# Initialize SkillEvolver (reuses the same store & registry)
360-
# available_tools will be updated before each evolution cycle
361-
self._skill_evolver = SkillEvolver(
362-
store=skill_store,
363-
registry=self._skill_registry,
364-
llm_client=self._llm_client,
365-
model=self.config.skill_evolver_model,
366-
max_concurrent=self.config.evolution_max_concurrent,
367-
)
368-
logger.info(f"✓ Skill evolution enabled (concurrent={self.config.evolution_max_concurrent})")
369-
except Exception as e:
370-
logger.warning(f"Execution analyzer init failed (non-fatal): {e}")
242+
# 5. Skill engine (registry, store, analyzer, evolver)
243+
await self._setup_skill_engine()
371244

372-
# Create ExecutionEngine with all dependencies
245+
# 6. Execution engine (wires all deps)
373246
self._execution_engine = ExecutionEngine(
374247
config=self.config,
375248
grounding_agent=self._grounding_agent,
@@ -392,6 +265,121 @@ async def initialize(self) -> None:
392265
await self.cleanup()
393266
raise
394267

268+
def _load_grounding_config(self):
269+
"""Load and merge grounding config, resolve backend scope and agent params.
270+
271+
Returns (grounding_config, backend_scope, max_iterations, visual_analysis_timeout).
272+
"""
273+
if self.config.grounding_config_path:
274+
from openspace.config.constants import CONFIG_GROUNDING, CONFIG_SECURITY
275+
from openspace.config.loader import CONFIG_DIR
276+
277+
grounding_config = load_config(
278+
CONFIG_DIR / CONFIG_GROUNDING, CONFIG_DIR / CONFIG_SECURITY, self.config.grounding_config_path
279+
)
280+
logger.info(f"Merged custom grounding config: {self.config.grounding_config_path}")
281+
else:
282+
grounding_config = get_config()
283+
284+
if getattr(self.config, "use_clawwork_productivity", False):
285+
shell_cfg = grounding_config.shell.model_copy(
286+
update={
287+
"use_clawwork_productivity": True,
288+
"working_dir": self.config.workspace_dir or grounding_config.shell.working_dir,
289+
}
290+
)
291+
grounding_config = grounding_config.model_copy(update={"shell": shell_cfg})
292+
logger.info("ClawWork productivity tools enabled (shell.working_dir used as sandbox root)")
293+
294+
agent_config = get_agent_config("GroundingAgent")
295+
_cli_max_iter = self.config.grounding_max_iterations
296+
_default_max_iter = OpenSpaceConfig().grounding_max_iterations
297+
298+
if agent_config:
299+
cfg_max_iter = agent_config.get("max_iterations", _default_max_iter)
300+
max_iterations = _cli_max_iter if _cli_max_iter != _default_max_iter else cfg_max_iter
301+
backend_scope = (
302+
self.config.backend_scope
303+
or agent_config.get("backend_scope")
304+
or ["gui", "shell", "mcp", "web", "system"]
305+
)
306+
visual_analysis_timeout = agent_config.get("visual_analysis_timeout", 30.0)
307+
self.config.grounding_max_iterations = max_iterations
308+
logger.info(
309+
f"Loaded GroundingAgent config from config_agents.json "
310+
f"(max_iterations={max_iterations}, visual_analysis_timeout={visual_analysis_timeout}s)"
311+
)
312+
else:
313+
max_iterations = self.config.grounding_max_iterations
314+
backend_scope = self.config.backend_scope or ["gui", "shell", "mcp", "web", "system"]
315+
visual_analysis_timeout = 30.0
316+
logger.warning(f"config_agents.json not found, using default config (max_iterations={max_iterations})")
317+
318+
if grounding_config.enabled_backends:
319+
scope_set = set(backend_scope)
320+
filtered = [
321+
entry for entry in grounding_config.enabled_backends if entry.get("name", "").lower() in scope_set
322+
]
323+
if len(filtered) != len(grounding_config.enabled_backends):
324+
skipped = [
325+
entry.get("name")
326+
for entry in grounding_config.enabled_backends
327+
if entry.get("name", "").lower() not in scope_set
328+
]
329+
logger.info(f"Skipping backends not in scope: {skipped}")
330+
grounding_config = grounding_config.model_copy(update={"enabled_backends": filtered})
331+
332+
return grounding_config, backend_scope, max_iterations, visual_analysis_timeout
333+
334+
async def _setup_skill_engine(self) -> None:
335+
"""Initialize skill registry, store, analyzer, and evolver."""
336+
if not (self._grounding_config and self._grounding_config.skills.enabled):
337+
return
338+
339+
self._tool_registry = ToolRegistry(
340+
config=self.config,
341+
grounding_config=self._grounding_config,
342+
llm_client=self._llm_client,
343+
)
344+
if self._tool_registry.discover():
345+
self._skill_registry = self._tool_registry.registry
346+
skills = self._skill_registry.list_skills()
347+
logger.info(f"✓ Skills: {len(skills)} discovered")
348+
self._grounding_agent.set_skill_registry(self._skill_registry)
349+
else:
350+
self._skill_registry = None
351+
352+
if not (self.config.enable_recording and self._skill_registry):
353+
return
354+
355+
try:
356+
skill_store = SkillStore()
357+
self._skill_store = skill_store
358+
await skill_store.sync_from_registry(self._skill_registry.list_skills())
359+
360+
quality_mgr = self._grounding_client.quality_manager if self._grounding_client else None
361+
self._execution_analyzer = ExecutionAnalyzer(
362+
store=skill_store,
363+
llm_client=self._llm_client,
364+
model=self.config.execution_analyzer_model,
365+
skill_registry=self._skill_registry,
366+
quality_manager=quality_mgr,
367+
)
368+
logger.info("✓ Execution analysis enabled")
369+
370+
self._grounding_agent._skill_store = skill_store
371+
372+
self._skill_evolver = SkillEvolver(
373+
store=skill_store,
374+
registry=self._skill_registry,
375+
llm_client=self._llm_client,
376+
model=self.config.skill_evolver_model,
377+
max_concurrent=self.config.evolution_max_concurrent,
378+
)
379+
logger.info(f"✓ Skill evolution enabled (concurrent={self.config.evolution_max_concurrent})")
380+
except Exception as e:
381+
logger.warning(f"Execution analyzer init failed (non-fatal): {e}")
382+
395383
async def execute(
396384
self,
397385
task: str,
@@ -413,14 +401,6 @@ async def execute(
413401
task_id=task_id,
414402
)
415403

416-
# NOTE: _init_skill_registry, _select_and_inject_skills, and
417-
# _get_skill_selection_llm have been extracted to ToolRegistry
418-
# (openspace/tool_registry.py) in Epic 4.1.
419-
#
420-
# execute(), _maybe_analyze_execution(), and _maybe_evolve_quality()
421-
# have been extracted to ExecutionEngine
422-
# (openspace/execution_engine.py) in Epic 4.3.
423-
424404
async def cleanup(self) -> None:
425405
"""
426406
Close all sessions and release resources.

0 commit comments

Comments
 (0)