From 45f358d19d6c7d96d8ba8aadebe5d5a60c22904b Mon Sep 17 00:00:00 2001 From: mac Date: Thu, 28 May 2026 04:00:51 +0800 Subject: [PATCH 1/2] Fix duplicated cycle detection with consistent error handling --- cli/localci/core/queue.py | 12 ++++++++---- cli/localci/core/workflow.py | 35 +++++++++++------------------------ cli/localci/errors.py | 8 +++++--- cli/tests/test_errors.py | 8 ++++++++ cli/tests/test_queue.py | 9 ++++++++- cli/tests/test_workflow.py | 30 ++++++++++++++++++++++++++++++ 6 files changed, 70 insertions(+), 32 deletions(-) diff --git a/cli/localci/core/queue.py b/cli/localci/core/queue.py index ba39b6f..63e2f52 100644 --- a/cli/localci/core/queue.py +++ b/cli/localci/core/queue.py @@ -109,20 +109,24 @@ def resolve(self) -> list[str]: temp_visited: set[str] = set() order: list[str] = [] - def visit(jid: str) -> None: + def visit(jid: str, path: list[str]) -> None: if jid in temp_visited: - raise CyclicDependencyError(jid) + idx = path.index(jid) + cycle = path[idx:] + [jid] + raise CyclicDependencyError(cycle) if jid in visited: return temp_visited.add(jid) + path.append(jid) for dep in self._graph.get(jid, []): - visit(dep) + visit(dep, path) + path.pop() temp_visited.discard(jid) visited.add(jid) order.append(jid) for jid in self._graph: - visit(jid) + visit(jid, []) return order def get_dependencies(self, job_id: str) -> list[str]: diff --git a/cli/localci/core/workflow.py b/cli/localci/core/workflow.py index df5198d..369c92a 100644 --- a/cli/localci/core/workflow.py +++ b/cli/localci/core/workflow.py @@ -302,30 +302,17 @@ def platform_summary(self) -> dict[Platform, int]: def dependency_order(self) -> list[str]: """Topological sort of jobs by dependencies.""" - visited: set[str] = set() - in_progress: set[str] = set() - order: list[str] = [] - - def visit(job_id: str) -> None: - if job_id in visited: - return - if job_id not in self.jobs: - logger.warning("Dependency '%s' not found in jobs", job_id) - return - if job_id in in_progress: - logger.warning("Circular dependency detected involving '%s'", job_id) - return - in_progress.add(job_id) - visited.add(job_id) - for dep in self.jobs[job_id].needs: - visit(dep) - in_progress.discard(job_id) - order.append(job_id) - - for job_id in self.jobs: - visit(job_id) - - return order + from localci.core.queue import DependencyResolver + + resolver = DependencyResolver() + job_ids = set(self.jobs) + for job_id, job in self.jobs.items(): + needs = [d for d in job.needs if d in job_ids] + for dep in job.needs: + if dep not in job_ids: + logger.warning("Dependency '%s' not found in jobs", dep) + resolver.add_job(job_id, needs) + return resolver.resolve() # ===================================================================== diff --git a/cli/localci/errors.py b/cli/localci/errors.py index c0b0b05..5ee03e9 100644 --- a/cli/localci/errors.py +++ b/cli/localci/errors.py @@ -190,9 +190,11 @@ def __init__(self, entry: dict[str, Any], detail: str) -> None: class CyclicDependencyError(WorkflowError): """Circular dependency detected in job graph.""" - def __init__(self, job_id: str) -> None: - super().__init__(f"Cyclic dependency detected involving job: {job_id}") - self.job_id = job_id + def __init__(self, cycle: list[str]) -> None: + self.cycle = list(cycle) + self.job_id = self.cycle[0] if self.cycle else "" + path = " -> ".join(self.cycle) + super().__init__(f"Cyclic dependency detected: {path}") # --------------------------------------------------------------------------- diff --git a/cli/tests/test_errors.py b/cli/tests/test_errors.py index ab51954..6486ef1 100644 --- a/cli/tests/test_errors.py +++ b/cli/tests/test_errors.py @@ -168,6 +168,14 @@ def test_workflow_parse_error_attributes(self, tmp_path): assert "unexpected key" in str(exc) assert str(p) in str(exc) + def test_cyclic_dependency_error_attributes(self): + cycle = ["a", "b", "a"] + exc = CyclicDependencyError(cycle) + assert exc.cycle == cycle + assert exc.job_id == "a" + assert "a -> b -> a" in str(exc) + assert isinstance(exc, WorkflowError) + # --------------------------------------------------------------------------- # Backward-compatibility: caught by built-in types diff --git a/cli/tests/test_queue.py b/cli/tests/test_queue.py index 34bafbc..2f53800 100644 --- a/cli/tests/test_queue.py +++ b/cli/tests/test_queue.py @@ -118,8 +118,15 @@ def test_cyclic_dependency(self): resolver = DependencyResolver() resolver.add_job("a", ["b"]) resolver.add_job("b", ["a"]) - with pytest.raises(CyclicDependencyError): + with pytest.raises(CyclicDependencyError) as exc_info: resolver.resolve() + exc = exc_info.value + assert exc.cycle + assert "a" in exc.cycle + assert "b" in exc.cycle + assert "a" in str(exc) + assert "b" in str(exc) + assert " -> " in str(exc) def test_all_dependencies_met(self): resolver = DependencyResolver() diff --git a/cli/tests/test_workflow.py b/cli/tests/test_workflow.py index 4ed9de2..d644786 100644 --- a/cli/tests/test_workflow.py +++ b/cli/tests/test_workflow.py @@ -11,6 +11,7 @@ import pytest +from localci.errors import CyclicDependencyError from localci.core.workflow import ( BuildSystem, BuildVariant, @@ -350,6 +351,35 @@ def test_changelog_needs_build(self, sample_workflow): changelog = sample_workflow.jobs["changelog"] assert "build" in changelog.needs + def test_cyclic_dependency_raises(self, tmp_path): + workflow = Workflow( + name="cyclic", + file_path=tmp_path / "cyclic.yml", + events=["push"], + jobs={ + "a": Job( + id="a", + name="A", + runs_on="ubuntu-latest", + needs=["b"], + ), + "b": Job( + id="b", + name="B", + runs_on="ubuntu-latest", + needs=["a"], + ), + }, + ) + with pytest.raises(CyclicDependencyError) as exc_info: + workflow.dependency_order() + exc = exc_info.value + assert exc.cycle + assert "a" in exc.cycle + assert "b" in exc.cycle + assert "a" in str(exc) + assert "b" in str(exc) + # ===================================================================== # Container info From c42ca7d83c03e498ba0358e4a11a7b48d1b97db8 Mon Sep 17 00:00:00 2001 From: mac Date: Fri, 29 May 2026 01:23:49 +0800 Subject: [PATCH 2/2] addressed reviewer opinions --- cli/localci/core/queue.py | 7 +++++++ cli/localci/core/workflow.py | 3 +-- cli/localci/errors.py | 16 +++++++++++++++- cli/tests/test_errors.py | 4 ++++ cli/tests/test_queue.py | 10 ++++------ cli/tests/test_workflow.py | 9 ++++----- 6 files changed, 35 insertions(+), 14 deletions(-) diff --git a/cli/localci/core/queue.py b/cli/localci/core/queue.py index 63e2f52..9477dbe 100644 --- a/cli/localci/core/queue.py +++ b/cli/localci/core/queue.py @@ -105,6 +105,13 @@ def add_job(self, job_id: str, needs: list[str]) -> None: self._reverse[dep].append(job_id) def resolve(self) -> list[str]: + """Return a topological order of registered jobs. + + Dependencies listed in ``needs`` but never passed to :meth:`add_job` + are still visited with an empty edge list and included in the order. + Callers (e.g. ``Workflow.dependency_order``) must register a complete + graph. + """ visited: set[str] = set() temp_visited: set[str] = set() order: list[str] = [] diff --git a/cli/localci/core/workflow.py b/cli/localci/core/workflow.py index 369c92a..4291dc3 100644 --- a/cli/localci/core/workflow.py +++ b/cli/localci/core/workflow.py @@ -15,6 +15,7 @@ from pathlib import Path from typing import Any, Optional +from localci.core.queue import DependencyResolver from localci.errors import ( LocalCIError, MissingFieldError, @@ -302,8 +303,6 @@ def platform_summary(self) -> dict[Platform, int]: def dependency_order(self) -> list[str]: """Topological sort of jobs by dependencies.""" - from localci.core.queue import DependencyResolver - resolver = DependencyResolver() job_ids = set(self.jobs) for job_id, job in self.jobs.items(): diff --git a/cli/localci/errors.py b/cli/localci/errors.py index 5ee03e9..455899e 100644 --- a/cli/localci/errors.py +++ b/cli/localci/errors.py @@ -188,9 +188,23 @@ def __init__(self, entry: dict[str, Any], detail: str) -> None: class CyclicDependencyError(WorkflowError): - """Circular dependency detected in job graph.""" + """Circular dependency detected in job graph. + + Attributes + ---------- + cycle: + Ordered job IDs forming the loop; first and last elements are equal + (e.g. ``["a", "b", "a"]``). + job_id: + First job in the cycle, kept for backward compatibility (``cycle[0]``). + """ def __init__(self, cycle: list[str]) -> None: + if not isinstance(cycle, list): + raise TypeError( + f"cycle must be a list[str], got {type(cycle).__name__!r}; " + "pass an ordered path such as ['a', 'b', 'a'], not a single job_id string" + ) self.cycle = list(cycle) self.job_id = self.cycle[0] if self.cycle else "" path = " -> ".join(self.cycle) diff --git a/cli/tests/test_errors.py b/cli/tests/test_errors.py index 6486ef1..dabe955 100644 --- a/cli/tests/test_errors.py +++ b/cli/tests/test_errors.py @@ -176,6 +176,10 @@ def test_cyclic_dependency_error_attributes(self): assert "a -> b -> a" in str(exc) assert isinstance(exc, WorkflowError) + def test_cyclic_dependency_error_rejects_string(self): + with pytest.raises(TypeError, match="cycle must be a list"): + CyclicDependencyError("ab") + # --------------------------------------------------------------------------- # Backward-compatibility: caught by built-in types diff --git a/cli/tests/test_queue.py b/cli/tests/test_queue.py index 2f53800..b598247 100644 --- a/cli/tests/test_queue.py +++ b/cli/tests/test_queue.py @@ -121,12 +121,10 @@ def test_cyclic_dependency(self): with pytest.raises(CyclicDependencyError) as exc_info: resolver.resolve() exc = exc_info.value - assert exc.cycle - assert "a" in exc.cycle - assert "b" in exc.cycle - assert "a" in str(exc) - assert "b" in str(exc) - assert " -> " in str(exc) + assert exc.cycle == ["a", "b", "a"] + assert exc.cycle[0] == exc.cycle[-1] + assert exc.job_id == "a" + assert "a -> b -> a" in str(exc) def test_all_dependencies_met(self): resolver = DependencyResolver() diff --git a/cli/tests/test_workflow.py b/cli/tests/test_workflow.py index d644786..f9b90c7 100644 --- a/cli/tests/test_workflow.py +++ b/cli/tests/test_workflow.py @@ -374,11 +374,10 @@ def test_cyclic_dependency_raises(self, tmp_path): with pytest.raises(CyclicDependencyError) as exc_info: workflow.dependency_order() exc = exc_info.value - assert exc.cycle - assert "a" in exc.cycle - assert "b" in exc.cycle - assert "a" in str(exc) - assert "b" in str(exc) + assert exc.cycle == ["a", "b", "a"] + assert exc.cycle[0] == exc.cycle[-1] + assert exc.job_id == "a" + assert "a -> b -> a" in str(exc) # =====================================================================