Fix duplicated cycle detection with consistent error handling

[henry0816191]

Summary

• Unify job dependency cycle detection into a single canonical path: DependencyResolver.resolve() in cli/localci/core/queue.py.
• Make Workflow.dependency_order() delegate to DependencyResolver instead of running its own DFS that logged a warning and returned a partial order on cycles.
• Extend CyclicDependencyError with a cycle: list[str] attribute and a message that shows the full cycle path (e.g. a -> b -> a) for easier debugging.

What changed

Area	Change
cli/localci/errors.py	CyclicDependencyError now accepts cycle: list[str]; message uses " -> ".join(cycle); job_id retained as cycle[0] for compatibility
cli/localci/core/queue.py	DependencyResolver.resolve() builds the cycle path on detection and raises CyclicDependencyError(cycle)
cli/localci/core/workflow.py	dependency_order() populates a DependencyResolver, filters unknown needs (with existing warning), and calls resolve()
Tests	Strengthened queue cyclic test; added workflow and error-attribute tests

Behavior change

Cyclic job dependencies now always raise CyclicDependencyError with a cycle path, whether detected via DependencyResolver.resolve() or Workflow.dependency_order(). Previously, dependency_order() could silently return a partial topological order.

Test plan

• pytest tests/test_queue.py::TestDependencyResolver::test_cyclic_dependency — cycle detected, cycle populated, path in message
• pytest tests/test_workflow.py::TestDependencyOrder::test_cyclic_dependency_raises — workflow path delegates and raises same error type
• pytest tests/test_errors.py::TestErrorAttributes::test_cyclic_dependency_error_attributes — cycle, message, WorkflowError hierarchy
• Existing happy-path dependency order tests pass (test_workflow.py, test_full_workflow.py)
• Full suite: cd cli && pytest --cov=localci --cov-report=term-missing (490 passed)

Out of scope

• Enqueue-time cycle validation in PriorityJobQueue (still uses all_dependencies_met() only; not required by w4_issue_03)

Related issue

close #40

Summary by CodeRabbit

• Bug Fixes

  • Improved cycle detection and reporting: circular dependency errors now include the full ordered cycle (e.g., "a -> b -> a"), expose the cycle sequence, and validate the reported cycle format.
  • Dependency ordering now uses a shared resolver and emits warnings for dependencies referenced but not registered.

• Tests

  • Added and strengthened tests to verify cycle reporting, exception contents, validation behavior, and that dependency ordering raises the appropriate error.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 79e8736b-c538-41ea-9e0f-7df98515e3fb

📥 Commits

Reviewing files that changed from the base of the PR and between 45f358d and c42ca7d.

📒 Files selected for processing (6)

• cli/localci/core/queue.py
• cli/localci/core/workflow.py
• cli/localci/errors.py
• cli/tests/test_errors.py
• cli/tests/test_queue.py
• cli/tests/test_workflow.py

🚧 Files skipped from review as they are similar to previous changes (5)

• cli/tests/test_queue.py
• cli/localci/core/workflow.py
• cli/tests/test_errors.py
• cli/localci/errors.py
• cli/localci/core/queue.py

---

📝 Walkthrough

Walkthrough

Consolidates cycle detection into DependencyResolver, changes CyclicDependencyError to carry the ordered cycle path, updates Workflow.dependency_order() to use the resolver, and strengthens tests to assert the new error attributes and message.

Changes

Unified Cycle Detection and Error Handling

Layer / File(s)	Summary
Error Contract: CyclicDependencyError with Cycle Path cli/localci/errors.py	CyclicDependencyError.__init__ now accepts cycle: list[str], stores cycle, sets job_id from the first cycle element (or ""), validates the argument type, and formats the message as a " -> "-joined path.
DependencyResolver Cycle Detection and Path Tracking cli/localci/core/queue.py	DependencyResolver.resolve() tracks a DFS recursion path; on a back-edge into the active path it computes the ordered cycle segment and raises CyclicDependencyError(cycle). Root traversal calls visit(jid, []). Docstring notes unregistered needs are visited with empty edges.
Workflow.dependency_order() Delegation to Resolver cli/localci/core/workflow.py	Workflow.dependency_order() imports DependencyResolver, builds the resolver graph from jobs (filters unknown needs and logs warnings), and returns resolver.resolve(), removing the previous inline DFS/topo-sort and local cycle handling.
Test Coverage: Error Contract, Resolver, and Workflow cli/tests/test_errors.py, cli/tests/test_queue.py, cli/tests/test_workflow.py	Added/updated tests to assert CyclicDependencyError.cycle equals the ordered job IDs (start==end), job_id is derived from the first element, the message includes the " -> "-joined path, and Workflow.dependency_order() propagates the error on cycles.

sequenceDiagram
  participant Workflow
  participant DependencyResolver
  participant CyclicDependencyError
  Workflow->>DependencyResolver: build graph (add_job / add_edge(filtered needs))
  Workflow->>DependencyResolver: resolve()
  DependencyResolver->>DependencyResolver: DFS with path tracking
  alt cycle detected
    DependencyResolver->>CyclicDependencyError: raise with ordered cycle list
    CyclicDependencyError-->>Workflow: exception with cycle and message
  else no cycle
    DependencyResolver-->>Workflow: return ordered job list
  end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰
I hopped through graphs with paws so spry,
Found tangled loops that made me sigh.
One resolver now follows the trail,
And shows the cycle in a tidy tale.
Thank you, humans — I fixed the tie!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 15.38% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main objective: unifying cycle detection and fixing error handling inconsistencies across DependencyResolver and Workflow.
Linked Issues check	✅ Passed	All acceptance criteria from issue #40 are met: single canonical cycle detection (DependencyResolver), consistent CyclicDependencyError, non-canonical path delegates, cycle path included in error, tests verify detection/error-type/cycle-path, tests passing.
Out of Scope Changes check	✅ Passed	All changes directly address issue #40 objectives; no out-of-scope modifications detected beyond the centralized cycle detection and error handling improvements.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[henry0816191]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[coderabbitai]

🧹 Nitpick comments (1)

cli/tests/test_queue.py (1)

124-129: ⚡ Quick win

Strengthen cycle-path assertions to enforce ordering contract.

Current checks only verify membership and delimiter presence, so an incorrectly ordered path could still pass. Assert cycle closure and expected sequence for this fixed 2-node cycle.

Proposed test tightening

-        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 in (["a", "b", "a"], ["b", "a", "b"])
+        assert exc.cycle[0] == exc.cycle[-1]
+        assert " -> ".join(exc.cycle) in str(exc)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cli/tests/test_queue.py` around lines 124 - 129, The test currently only
checks membership and delimiter presence for exc.cycle and str(exc), which
allows wrong ordering; update the assertions to enforce the exact sequence and
closure for the 2-node cycle by asserting exc.cycle == ["a","b","a"] (or the
canonical cyclic path expected) and that str(exc) contains the ordered string "a
-> b -> a" (or the exact formatted representation your exception uses), ensuring
both the cycle list and its string representation preserve the required ordering
and closure; update assertions referencing exc.cycle and str(exc) accordingly.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@cli/tests/test_queue.py`:
- Around line 124-129: The test currently only checks membership and delimiter
presence for exc.cycle and str(exc), which allows wrong ordering; update the
assertions to enforce the exact sequence and closure for the 2-node cycle by
asserting exc.cycle == ["a","b","a"] (or the canonical cyclic path expected) and
that str(exc) contains the ordered string "a -> b -> a" (or the exact formatted
representation your exception uses), ensuring both the cycle list and its string
representation preserve the required ordering and closure; update assertions
referencing exc.cycle and str(exc) accordingly.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 6f5762c3-b768-43f5-9503-9ae0c3b668b2

📥 Commits

Reviewing files that changed from the base of the PR and between a9d22d6 and 45f358d.

📒 Files selected for processing (6)

• cli/localci/core/queue.py
• cli/localci/core/workflow.py
• cli/localci/errors.py
• cli/tests/test_errors.py
• cli/tests/test_queue.py
• cli/tests/test_workflow.py

[henry0816191]

@coderabbitai
Don't skip.
Full review again.

[coderabbitai]

@henry0816191 Understood — triggering a full review now, no skipping.

✅ Actions performed

Full review triggered.

[bradjin8]

Overall implementations look good.

Please address the inline comments (mainly tighter cycle-path test assertions and CyclicDependencyError API/docs) before I approve. Happy to re-review after updates.

[coderabbitai]

Actionable comments posted: 0