From 64a30760586d98f0621a2ec79ff87d769a2cf464 Mon Sep 17 00:00:00 2001 From: Dennis-yxchen Date: Tue, 31 Mar 2026 17:03:00 +0800 Subject: [PATCH 1/4] fix: pass max_iterations to grounding agent in no-skill execution path Without this, the no-skill path ignores the resolved max_iterations and uses whatever default the agent has, instead of the configured grounding_max_iterations value. Co-authored-by: wul48527-code --- openspace/tool_layer.py | 1 + 1 file changed, 1 insertion(+) diff --git a/openspace/tool_layer.py b/openspace/tool_layer.py index 1ea419f..4cf4b7b 100644 --- a/openspace/tool_layer.py +++ b/openspace/tool_layer.py @@ -507,6 +507,7 @@ class OpenSpace: f"Executing with GroundingAgent " f"(max {max_iterations} iterations, no skills)..." ) + execution_context["max_iterations"] = max_iterations result = await self._grounding_agent.process(execution_context) execution_time = asyncio.get_event_loop().time() - start_time From e48e22afc8ec5c9c223630817f8526d09fbb07b9 Mon Sep 17 00:00:00 2001 From: Dennis-yxchen Date: Tue, 31 Mar 2026 17:18:38 +0800 Subject: [PATCH 2/4] fix: use unique workflow ID to prevent collision across roots workflow_dir.name was used as the discovery key and API ID, so two different WORKFLOW_ROOTS containing a leaf directory with the same name would silently drop one. Use root name + relative path joined with __ as a stable unique ID instead. Co-authored-by: wul48527-code --- openspace/dashboard_server.py | 20 +++++++-- tests/test_workflow_id.py | 79 +++++++++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+), 4 deletions(-) create mode 100644 tests/test_workflow_id.py diff --git a/openspace/dashboard_server.py b/openspace/dashboard_server.py index 96d3e50..4333aef 100644 --- a/openspace/dashboard_server.py +++ b/openspace/dashboard_server.py @@ -419,6 +419,18 @@ def _build_lineage_payload(skill_id: str, store: SkillStore) -> Dict[str, Any]: } +def _workflow_id(workflow_dir: Path) -> str: + """Stable short ID for a workflow directory, unique across roots.""" + resolved = workflow_dir.resolve() + for root in WORKFLOW_ROOTS: + try: + rel = resolved.relative_to(root.resolve()) + return f"{root.name}__{'__'.join(rel.parts)}" + except ValueError: + continue + return workflow_dir.name + + def _discover_workflow_dirs() -> List[Path]: discovered: Dict[str, Path] = {} for root in WORKFLOW_ROOTS: @@ -439,14 +451,14 @@ def _scan_workflow_tree(directory: Path, discovered: Dict[str, Path], *, _depth: if not child.is_dir(): continue if (child / "metadata.json").exists() or (child / "traj.jsonl").exists(): - discovered.setdefault(child.name, child) + discovered.setdefault(str(child.resolve()), child) else: _scan_workflow_tree(child, discovered, _depth=_depth + 1, _max_depth=_max_depth) def _get_workflow_dir(workflow_id: str) -> Optional[Path]: for path in _discover_workflow_dirs(): - if path.name == workflow_id: + if _workflow_id(path) == workflow_id: return path return None @@ -464,7 +476,7 @@ def _build_workflow_summary(workflow_dir: Path) -> Dict[str, Any]: for candidate in video_candidates: if candidate.exists(): rel = candidate.relative_to(workflow_dir).as_posix() - video_url = url_for("workflow_artifact", workflow_id=workflow_dir.name, artifact_path=rel) + video_url = url_for("workflow_artifact", workflow_id=_workflow_id(workflow_dir), artifact_path=rel) break outcome = metadata.get("execution_outcome") or {} @@ -514,7 +526,7 @@ def _build_workflow_summary(workflow_dir: Path) -> Dict[str, Any]: iterations = len(trajectory) return { - "id": workflow_dir.name, + "id": _workflow_id(workflow_dir), "path": str(workflow_dir), "task_id": metadata.get("task_id") or metadata.get("task_name") or workflow_dir.name, "task_name": metadata.get("task_name") or metadata.get("task_id") or workflow_dir.name, diff --git a/tests/test_workflow_id.py b/tests/test_workflow_id.py new file mode 100644 index 0000000..d4ca82f --- /dev/null +++ b/tests/test_workflow_id.py @@ -0,0 +1,79 @@ +"""Tests for workflow ID uniqueness and discovery.""" + +from pathlib import Path +from unittest.mock import patch + +import pytest + +from openspace import dashboard_server + + +def _make_workflow(path: Path): + """Create a minimal workflow directory with metadata.json.""" + path.mkdir(parents=True, exist_ok=True) + (path / "metadata.json").write_text("{}", encoding="utf-8") + + +class TestWorkflowId: + def test_same_leaf_name_different_roots_get_unique_ids(self, tmp_path): + root_a = tmp_path / "root_a" + root_b = tmp_path / "root_b" + wf_a = root_a / "task1" + wf_b = root_b / "task1" + _make_workflow(wf_a) + _make_workflow(wf_b) + + with patch.object(dashboard_server, "WORKFLOW_ROOTS", [root_a, root_b]): + id_a = dashboard_server._workflow_id(wf_a) + id_b = dashboard_server._workflow_id(wf_b) + + assert id_a != id_b + assert "task1" in id_a + assert "task1" in id_b + + def test_nested_workflow_id_includes_path(self, tmp_path): + root = tmp_path / "root" + wf = root / "sub" / "deep" / "task1" + _make_workflow(wf) + + with patch.object(dashboard_server, "WORKFLOW_ROOTS", [root]): + wf_id = dashboard_server._workflow_id(wf) + + assert wf_id == "root__sub__deep__task1" + + def test_workflow_outside_roots_falls_back_to_name(self, tmp_path): + wf = tmp_path / "orphan_workflow" + _make_workflow(wf) + + with patch.object(dashboard_server, "WORKFLOW_ROOTS", []): + wf_id = dashboard_server._workflow_id(wf) + + assert wf_id == "orphan_workflow" + + +class TestDiscoverWorkflowDirs: + def test_same_name_workflows_both_discovered(self, tmp_path): + root_a = tmp_path / "root_a" + root_b = tmp_path / "root_b" + _make_workflow(root_a / "task1") + _make_workflow(root_b / "task1") + + with patch.object(dashboard_server, "WORKFLOW_ROOTS", [root_a, root_b]): + dirs = dashboard_server._discover_workflow_dirs() + + assert len(dirs) == 2 + + def test_get_workflow_dir_resolves_correct_path(self, tmp_path): + root_a = tmp_path / "root_a" + root_b = tmp_path / "root_b" + _make_workflow(root_a / "task1") + _make_workflow(root_b / "task1") + + with patch.object(dashboard_server, "WORKFLOW_ROOTS", [root_a, root_b]): + id_a = dashboard_server._workflow_id(root_a / "task1") + id_b = dashboard_server._workflow_id(root_b / "task1") + found_a = dashboard_server._get_workflow_dir(id_a) + found_b = dashboard_server._get_workflow_dir(id_b) + + assert found_a.resolve() == (root_a / "task1").resolve() + assert found_b.resolve() == (root_b / "task1").resolve() From 4032fe682d903c96a8fdb40d418bf7db7aec89b8 Mon Sep 17 00:00:00 2001 From: Dennis-yxchen Date: Tue, 31 Mar 2026 17:24:12 +0800 Subject: [PATCH 3/4] test: add regression tests for max_iterations in no-skill path --- tests/test_max_iterations.py | 64 ++++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) create mode 100644 tests/test_max_iterations.py diff --git a/tests/test_max_iterations.py b/tests/test_max_iterations.py new file mode 100644 index 0000000..04e6863 --- /dev/null +++ b/tests/test_max_iterations.py @@ -0,0 +1,64 @@ +"""Test that max_iterations is passed to grounding agent in no-skill path.""" + +import asyncio +import types +from unittest.mock import AsyncMock, patch + +import pytest + +from openspace.tool_layer import OpenSpace, OpenSpaceConfig + + +def _make_openspace(configured_max: int = 50) -> OpenSpace: + config = OpenSpaceConfig() + config.grounding_max_iterations = configured_max + os_inst = OpenSpace(config) + os_inst._initialized = True + os_inst._running = False + os_inst._task_done = asyncio.Event() + os_inst._task_done.set() + os_inst._grounding_client = types.SimpleNamespace(_registry={}) + os_inst._recording_manager = None + os_inst._skill_registry = None + os_inst._execution_analyzer = None + os_inst._skill_evolver = None + return os_inst + + +@pytest.mark.asyncio +async def test_no_skill_path_passes_configured_max_iterations(): + """When no skills match, the configured grounding_max_iterations + must be forwarded to the agent, not silently dropped.""" + os_inst = _make_openspace(configured_max=50) + recorded = {} + + async def fake_process(context): + recorded.update(context) + return {"status": "success", "iterations": 1, "tool_executions": []} + + os_inst._grounding_agent = types.SimpleNamespace(process=fake_process) + os_inst._maybe_analyze_execution = AsyncMock() + os_inst._maybe_evolve_quality = AsyncMock() + + await os_inst.execute("do something") + + assert recorded.get("max_iterations") == 50 + + +@pytest.mark.asyncio +async def test_no_skill_path_uses_caller_override_when_larger(): + """Caller-provided max_iterations should win when larger than config.""" + os_inst = _make_openspace(configured_max=20) + recorded = {} + + async def fake_process(context): + recorded.update(context) + return {"status": "success", "iterations": 1, "tool_executions": []} + + os_inst._grounding_agent = types.SimpleNamespace(process=fake_process) + os_inst._maybe_analyze_execution = AsyncMock() + os_inst._maybe_evolve_quality = AsyncMock() + + await os_inst.execute("do something", max_iterations=100) + + assert recorded.get("max_iterations") == 100 From 028c5b01f9515dece7f4164c9564803b577de0b2 Mon Sep 17 00:00:00 2001 From: Dennis-yxchen Date: Tue, 31 Mar 2026 17:38:39 +0800 Subject: [PATCH 4/4] fix: use hash-based workflow ID to prevent separator collisions The previous __-joined scheme was not injective: a directory named a__b and a nested path a/b both mapped to the same ID. Use a sha256 hash suffix of the resolved path instead, which is collision-free and keeps the dir name as a human-readable prefix. Added regression test for separator collision case. --- openspace/dashboard_server.py | 18 +++++++++--------- tests/test_workflow_id.py | 29 ++++++++++++++++++++++------- 2 files changed, 31 insertions(+), 16 deletions(-) diff --git a/openspace/dashboard_server.py b/openspace/dashboard_server.py index 4333aef..f6916b6 100644 --- a/openspace/dashboard_server.py +++ b/openspace/dashboard_server.py @@ -420,15 +420,15 @@ def _build_lineage_payload(skill_id: str, store: SkillStore) -> Dict[str, Any]: def _workflow_id(workflow_dir: Path) -> str: - """Stable short ID for a workflow directory, unique across roots.""" - resolved = workflow_dir.resolve() - for root in WORKFLOW_ROOTS: - try: - rel = resolved.relative_to(root.resolve()) - return f"{root.name}__{'__'.join(rel.parts)}" - except ValueError: - continue - return workflow_dir.name + """Stable short ID for a workflow directory, unique across roots. + + Uses a hash suffix derived from the resolved path to avoid collisions + when directory names contain the separator character. + """ + import hashlib + resolved = str(workflow_dir.resolve()) + path_hash = hashlib.sha256(resolved.encode()).hexdigest()[:8] + return f"{workflow_dir.name}_{path_hash}" def _discover_workflow_dirs() -> List[Path]: diff --git a/tests/test_workflow_id.py b/tests/test_workflow_id.py index d4ca82f..84bf3e1 100644 --- a/tests/test_workflow_id.py +++ b/tests/test_workflow_id.py @@ -28,27 +28,42 @@ class TestWorkflowId: id_b = dashboard_server._workflow_id(wf_b) assert id_a != id_b - assert "task1" in id_a - assert "task1" in id_b + assert id_a.startswith("task1_") + assert id_b.startswith("task1_") - def test_nested_workflow_id_includes_path(self, tmp_path): + def test_id_contains_dir_name_and_hash(self, tmp_path): root = tmp_path / "root" - wf = root / "sub" / "deep" / "task1" + wf = root / "my-task" _make_workflow(wf) with patch.object(dashboard_server, "WORKFLOW_ROOTS", [root]): wf_id = dashboard_server._workflow_id(wf) - assert wf_id == "root__sub__deep__task1" + assert wf_id.startswith("my-task_") + assert len(wf_id) == len("my-task_") + 8 # 8-char hex hash - def test_workflow_outside_roots_falls_back_to_name(self, tmp_path): + def test_separator_collision_produces_different_ids(self, tmp_path): + """Regression: a dir named 'a__b' must not collide with path a/b.""" + root = tmp_path / "root" + flat = root / "a__b" + nested = root / "a" / "b" + _make_workflow(flat) + _make_workflow(nested) + + with patch.object(dashboard_server, "WORKFLOW_ROOTS", [root]): + id_flat = dashboard_server._workflow_id(flat) + id_nested = dashboard_server._workflow_id(nested) + + assert id_flat != id_nested + + def test_workflow_outside_roots_still_works(self, tmp_path): wf = tmp_path / "orphan_workflow" _make_workflow(wf) with patch.object(dashboard_server, "WORKFLOW_ROOTS", []): wf_id = dashboard_server._workflow_id(wf) - assert wf_id == "orphan_workflow" + assert wf_id.startswith("orphan_workflow_") class TestDiscoverWorkflowDirs: