From 2b9a8e4ad4a96d7683c6eba78a101e43f8b65155 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Andr=C3=A9=20=22decko=22=20de=20Brito?= <decko@redhat.com>
Date: Tue, 26 May 2026 22:31:34 -0300
Subject: [PATCH] fix(271): extract phase_dot_class() from Jinja2 template to
 eliminate vacuous assertions
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Phase dot-class logic was previously embedded in a Jinja2 template conditional,
making it impossible to unit-test directly. Tests in TestPhaseTimelineDotColoring
matched strings in the full HTML content, including the CSS <style> block, which
contained class name tokens like '.phase-status-superseded' — making assertions
like 'assert ".phase-status-superseded" in content' vacuous (always true
regardless of rendering correctness).

Changes:
- Add phase_dot_class(phase: PhaseResult) -> str in html_report.py, implementing
  the priority rules (status always wins, then verify/review verdict logic, then
  generation-based rework detection) that were previously in the Jinja2 template.
- Pass phase_dot_class to the template context; replace the 20-line conditional
  block with {% set dot_class = phase_dot_class(phase) %}.
- Add TestPhaseDotClass with 26 unit tests that exercise phase_dot_class() directly
  via PhaseResult objects — no HTML rendering, no CSS-string false positives.
- Rename test_superseded_phase_css_rule_defined → test_superseded_phase_css_rule_has_opacity
  and replace the vacuous '.phase-status-superseded' in content assertion with a
  line-level check that the CSS rule body includes 'opacity' on the same line as
  the selector (plus an HTML element class-attribute assertion).

Assisted-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Assigned-by: soda-orchestrator
---
 changes/271.fix                          |   6 +
 src/raki/report/html_report.py           |  45 ++++
 src/raki/report/templates/report.html.j2 |  26 +-
 tests/test_report_html.py                | 287 ++++++++++++++++++++++-
 4 files changed, 337 insertions(+), 27 deletions(-)
 create mode 100644 changes/271.fix

diff --git a/changes/271.fix b/changes/271.fix
new file mode 100644
index 0000000..d177d7b
--- /dev/null
+++ b/changes/271.fix
@@ -0,0 +1,6 @@
+Extract ``phase_dot_class()`` from Jinja2 template into a Python function so
+that dot-color logic is unit-tested directly rather than through full-HTML
+string matching, which previously matched CSS class definitions in the
+``<style>`` block (vacuous assertions).  The vacuous ``test_superseded_phase_css_rule_defined``
+assertion (``".phase-status-superseded" in content``) is replaced with a
+line-level check that confirms the CSS rule body includes ``opacity``.
diff --git a/src/raki/report/html_report.py b/src/raki/report/html_report.py
index 4912926..23f54f4 100644
--- a/src/raki/report/html_report.py
+++ b/src/raki/report/html_report.py
@@ -212,6 +212,50 @@
 ]
 
 
+def phase_dot_class(phase: PhaseResult) -> str:
+    """Return the CSS class suffix for the phase status dot in the HTML timeline.
+
+    Priority rules (highest to lowest):
+    1. ``failed``, ``skipped``, ``superseded`` status always wins over any verdict.
+    2. ``verify`` phase with ``output_structured.verdict``:
+       - ``"pass"`` (case-insensitive) → ``"completed"`` (green dot)
+       - any other value → ``"failed"`` (red dot)
+    3. ``review`` phase with ``output_structured.verdict``:
+       - ``"approve"``, ``"pass"``, ``"pass-with-follow-ups"`` → ``"completed"``
+       - ``"rework"`` → ``"rework"`` (yellow dot)
+       - any other value → ``"failed"``
+    4. All other phases: generation > 1 and ``status == "completed"`` → ``"rework"``,
+       otherwise return ``phase.status`` verbatim.
+
+    Returns one of: ``"completed"``, ``"rework"``, ``"failed"``, ``"skipped"``,
+    ``"superseded"``.
+    """
+    if phase.status in ("failed", "skipped", "superseded"):
+        return phase.status
+
+    verdict: str | None = None
+    if isinstance(phase.output_structured, dict):
+        raw_verdict = phase.output_structured.get("verdict")
+        if isinstance(raw_verdict, str):
+            verdict = raw_verdict
+
+    if verdict is not None and phase.name == "verify":
+        return "completed" if verdict.lower() == "pass" else "failed"
+
+    if verdict is not None and phase.name == "review":
+        verdict_lc = verdict.lower()
+        if verdict_lc in ("approve", "pass", "pass-with-follow-ups"):
+            return "completed"
+        if verdict_lc == "rework":
+            return "rework"
+        return "failed"
+
+    if phase.status == "completed" and phase.generation > 1:
+        return "rework"
+
+    return phase.status
+
+
 def sort_phases(
     phases: list[PhaseResult],
     pipeline_phases: list[str] | None = None,
@@ -831,6 +875,7 @@ def color_name_fn(score: float | None, metric_name: str = "") -> str:
         needs_attention_count=needs_attention_count,
         format_duration=_format_duration,
         sort_phases=sort_phases,
+        phase_dot_class=phase_dot_class,
         no_data_metrics=no_data_metrics,
         agent_models=agent_models,
         judge_cost=judge_cost,
diff --git a/src/raki/report/templates/report.html.j2 b/src/raki/report/templates/report.html.j2
index 100284d..e0bc18d 100644
--- a/src/raki/report/templates/report.html.j2
+++ b/src/raki/report/templates/report.html.j2
@@ -1381,29 +1381,9 @@
         {% if sorted_phases %}
         <div class="phase-list">
           {% for phase in sorted_phases %}
-          {# Compute dot_class: failed/skipped/superseded status always wins; for verify/review
-             phases with structured output, use verdict to pick the color; otherwise fall back to
-             the existing gen>1 → rework logic. #}
-          {% if phase.status in ('failed', 'skipped', 'superseded') %}
-            {% set dot_class = phase.status %}
-          {% elif phase.output_structured and phase.output_structured is mapping
-                  and phase.output_structured.verdict is defined
-                  and phase.name == 'verify' %}
-            {% set dot_class = 'completed' if phase.output_structured.verdict | lower == 'pass' else 'failed' %}
-          {% elif phase.output_structured and phase.output_structured is mapping
-                  and phase.output_structured.verdict is defined
-                  and phase.name == 'review' %}
-            {% set verdict_lc = phase.output_structured.verdict | lower %}
-            {% if verdict_lc in ('approve', 'pass', 'pass-with-follow-ups') %}
-              {% set dot_class = 'completed' %}
-            {% elif verdict_lc == 'rework' %}
-              {% set dot_class = 'rework' %}
-            {% else %}
-              {% set dot_class = 'failed' %}
-            {% endif %}
-          {% else %}
-            {% set dot_class = 'rework' if phase.status == 'completed' and phase.generation > 1 else phase.status %}
-          {% endif %}
+          {# dot_class delegates to phase_dot_class() in html_report.py so the logic
+             is unit-tested directly without string-matching the full HTML output. #}
+          {% set dot_class = phase_dot_class(phase) %}
           <div class="phase-item">
             <span class="phase-status phase-status-{{ dot_class }}"></span>
             <span>{{ phase.name }} (gen {{ phase.generation }})</span>
diff --git a/tests/test_report_html.py b/tests/test_report_html.py
index fb7d90e..12171ff 100644
--- a/tests/test_report_html.py
+++ b/tests/test_report_html.py
@@ -3013,8 +3013,17 @@ def test_superseded_phase_has_superseded_dot(self, tmp_path: Path) -> None:
         assert 'class="phase-status phase-status-rework"' not in content
         assert 'class="phase-status phase-status-failed"' not in content
 
-    def test_superseded_phase_css_rule_defined(self, tmp_path: Path) -> None:
-        """The .phase-status-superseded CSS rule must exist in the rendered HTML."""
+    def test_superseded_phase_css_rule_has_opacity(self, tmp_path: Path) -> None:
+        """The .phase-status-superseded CSS rule must include opacity to visually dim the dot.
+
+        This distinguishes it from other phase-status rules (none of which use opacity).
+        The original assertion `'.phase-status-superseded' in content` was vacuous: it
+        matched the CSS selector token in the <style> block even when the phase was
+        rendered with a completely wrong dot class.  The replacement assertion checks
+        that a single HTML line contains BOTH the selector name AND the 'opacity'
+        property — proving the rule body is correct, not just that the class name
+        appears somewhere in the document.
+        """
         from raki.report.html_report import write_html_report
 
         report = self._make_report_with_phases(
@@ -3023,8 +3032,16 @@ def test_superseded_phase_css_rule_defined(self, tmp_path: Path) -> None:
         output = tmp_path / "report.html"
         write_html_report(report, output, include_sessions=True)
         content = output.read_text()
-        # The CSS selector must be defined so the dot is actually styled.
-        assert ".phase-status-superseded" in content
+        # Confirm the CSS rule body contains 'opacity' on the same line as the selector.
+        # 'opacity: 0.5' is unique to the superseded dot — no other phase-status CSS
+        # rule uses opacity.  This is more specific than just testing for the selector
+        # token, which would match even if the rule body was empty or wrong.
+        assert any(
+            "phase-status-superseded" in line and "opacity" in line for line in content.splitlines()
+        )
+        # Also assert the HTML span element is rendered with the correct class attribute
+        # (not just the CSS definition).
+        assert 'class="phase-status phase-status-superseded"' in content
 
     # --- Ticket #325: dot color should reflect verdict, not execution status ---
 
@@ -3278,6 +3295,268 @@ def test_non_verify_review_phase_uses_generation_logic(self, tmp_path: Path) ->
         assert 'class="phase-status phase-status-completed"' not in content
 
 
+# --- Ticket #271: phase_dot_class() unit tests replacing vacuous HTML string assertions ---
+
+
+class TestPhaseDotClass:
+    """Unit tests for phase_dot_class() — tests logic directly without HTML rendering.
+
+    These replace the vacuous approach of matching strings in the full HTML output
+    (which also matched CSS class definitions in the <style> block).
+    """
+
+    def _make_phase(
+        self,
+        *,
+        name: str = "implement",
+        generation: int = 1,
+        status: str = "completed",
+        output_structured: dict | None = None,
+    ):
+        """Build a minimal PhaseResult for dot-class testing."""
+        from raki.model.phases import PhaseResult
+
+        return PhaseResult(
+            name=name,
+            generation=generation,
+            status=status,  # type: ignore[arg-type]
+            output="done",
+            output_structured=output_structured,
+        )
+
+    # --- status always-wins cases ---
+
+    def test_failed_status_returns_failed(self) -> None:
+        """failed status → 'failed' dot regardless of generation or verdict."""
+        from raki.report.html_report import phase_dot_class
+
+        phase = self._make_phase(status="failed")
+        assert phase_dot_class(phase) == "failed"
+
+    def test_skipped_status_returns_skipped(self) -> None:
+        """skipped status → 'skipped' dot."""
+        from raki.report.html_report import phase_dot_class
+
+        phase = self._make_phase(status="skipped")
+        assert phase_dot_class(phase) == "skipped"
+
+    def test_superseded_status_returns_superseded(self) -> None:
+        """superseded status → 'superseded' dot."""
+        from raki.report.html_report import phase_dot_class
+
+        phase = self._make_phase(status="superseded")
+        assert phase_dot_class(phase) == "superseded"
+
+    def test_failed_status_overrides_pass_verdict_on_verify(self) -> None:
+        """failed status wins even when verify output_structured has verdict=PASS."""
+        from raki.report.html_report import phase_dot_class
+
+        phase = self._make_phase(
+            name="verify", status="failed", output_structured={"verdict": "PASS"}
+        )
+        assert phase_dot_class(phase) == "failed"
+
+    def test_skipped_status_overrides_pass_verdict_on_verify(self) -> None:
+        """skipped status wins even when verify output_structured has verdict=PASS."""
+        from raki.report.html_report import phase_dot_class
+
+        phase = self._make_phase(
+            name="verify", status="skipped", output_structured={"verdict": "PASS"}
+        )
+        assert phase_dot_class(phase) == "skipped"
+
+    def test_superseded_status_overrides_pass_verdict_on_verify(self) -> None:
+        """superseded status wins even when verify output_structured has verdict=PASS."""
+        from raki.report.html_report import phase_dot_class
+
+        phase = self._make_phase(
+            name="verify", status="superseded", output_structured={"verdict": "PASS"}
+        )
+        assert phase_dot_class(phase) == "superseded"
+
+    def test_failed_status_overrides_rework_verdict_on_review(self) -> None:
+        """failed status wins even when review output_structured has verdict=REWORK."""
+        from raki.report.html_report import phase_dot_class
+
+        phase = self._make_phase(
+            name="review", status="failed", output_structured={"verdict": "REWORK"}
+        )
+        assert phase_dot_class(phase) == "failed"
+
+    # --- generation-based logic (non-verify/review) ---
+
+    def test_completed_gen1_returns_completed(self) -> None:
+        """completed status at generation 1 → 'completed' dot."""
+        from raki.report.html_report import phase_dot_class
+
+        phase = self._make_phase(generation=1, status="completed")
+        assert phase_dot_class(phase) == "completed"
+
+    def test_completed_gen2_returns_rework(self) -> None:
+        """completed status at generation > 1 → 'rework' dot."""
+        from raki.report.html_report import phase_dot_class
+
+        phase = self._make_phase(generation=2, status="completed")
+        assert phase_dot_class(phase) == "rework"
+
+    def test_completed_gen3_returns_rework(self) -> None:
+        """completed status at generation 3 → 'rework' dot."""
+        from raki.report.html_report import phase_dot_class
+
+        phase = self._make_phase(generation=3, status="completed")
+        assert phase_dot_class(phase) == "rework"
+
+    # --- verify phase verdict logic ---
+
+    def test_verify_pass_verdict_returns_completed(self) -> None:
+        """verify phase with verdict=PASS and completed status → 'completed' dot."""
+        from raki.report.html_report import phase_dot_class
+
+        phase = self._make_phase(
+            name="verify", status="completed", output_structured={"verdict": "PASS"}
+        )
+        assert phase_dot_class(phase) == "completed"
+
+    def test_verify_pass_verdict_lowercase_returns_completed(self) -> None:
+        """verify phase with verdict=pass (lowercase) and completed status → 'completed' dot."""
+        from raki.report.html_report import phase_dot_class
+
+        phase = self._make_phase(
+            name="verify", status="completed", output_structured={"verdict": "pass"}
+        )
+        assert phase_dot_class(phase) == "completed"
+
+    def test_verify_fail_verdict_returns_failed(self) -> None:
+        """verify phase with verdict=FAIL and completed status → 'failed' dot."""
+        from raki.report.html_report import phase_dot_class
+
+        phase = self._make_phase(
+            name="verify", status="completed", output_structured={"verdict": "FAIL"}
+        )
+        assert phase_dot_class(phase) == "failed"
+
+    def test_verify_non_pass_verdict_returns_failed(self) -> None:
+        """verify phase with any non-PASS verdict and completed status → 'failed' dot."""
+        from raki.report.html_report import phase_dot_class
+
+        phase = self._make_phase(
+            name="verify", status="completed", output_structured={"verdict": "ERROR"}
+        )
+        assert phase_dot_class(phase) == "failed"
+
+    def test_verify_no_verdict_gen1_returns_completed(self) -> None:
+        """verify phase with no output_structured falls back to generation logic."""
+        from raki.report.html_report import phase_dot_class
+
+        phase = self._make_phase(name="verify", generation=1, status="completed")
+        assert phase_dot_class(phase) == "completed"
+
+    def test_verify_no_verdict_gen2_returns_rework(self) -> None:
+        """verify phase with no output_structured gen>1 falls back to rework logic."""
+        from raki.report.html_report import phase_dot_class
+
+        phase = self._make_phase(name="verify", generation=2, status="completed")
+        assert phase_dot_class(phase) == "rework"
+
+    # --- review phase verdict logic ---
+
+    def test_review_approve_verdict_returns_completed(self) -> None:
+        """review phase with verdict=approve → 'completed' dot."""
+        from raki.report.html_report import phase_dot_class
+
+        phase = self._make_phase(
+            name="review", status="completed", output_structured={"verdict": "approve"}
+        )
+        assert phase_dot_class(phase) == "completed"
+
+    def test_review_pass_verdict_returns_completed(self) -> None:
+        """review phase with verdict=pass → 'completed' dot."""
+        from raki.report.html_report import phase_dot_class
+
+        phase = self._make_phase(
+            name="review", status="completed", output_structured={"verdict": "pass"}
+        )
+        assert phase_dot_class(phase) == "completed"
+
+    def test_review_pass_with_follow_ups_returns_completed(self) -> None:
+        """review phase with verdict=pass-with-follow-ups → 'completed' dot."""
+        from raki.report.html_report import phase_dot_class
+
+        phase = self._make_phase(
+            name="review",
+            status="completed",
+            output_structured={"verdict": "pass-with-follow-ups"},
+        )
+        assert phase_dot_class(phase) == "completed"
+
+    def test_review_rework_verdict_returns_rework(self) -> None:
+        """review phase with verdict=REWORK → 'rework' dot."""
+        from raki.report.html_report import phase_dot_class
+
+        phase = self._make_phase(
+            name="review", status="completed", output_structured={"verdict": "REWORK"}
+        )
+        assert phase_dot_class(phase) == "rework"
+
+    def test_review_fail_verdict_returns_failed(self) -> None:
+        """review phase with verdict=FAIL → 'failed' dot."""
+        from raki.report.html_report import phase_dot_class
+
+        phase = self._make_phase(
+            name="review", status="completed", output_structured={"verdict": "FAIL"}
+        )
+        assert phase_dot_class(phase) == "failed"
+
+    def test_review_unknown_verdict_returns_failed(self) -> None:
+        """review phase with unrecognized verdict → 'failed' dot."""
+        from raki.report.html_report import phase_dot_class
+
+        phase = self._make_phase(
+            name="review", status="completed", output_structured={"verdict": "UNKNOWN"}
+        )
+        assert phase_dot_class(phase) == "failed"
+
+    def test_review_no_verdict_gen1_returns_completed(self) -> None:
+        """review phase with no output_structured falls back to generation logic."""
+        from raki.report.html_report import phase_dot_class
+
+        phase = self._make_phase(name="review", generation=1, status="completed")
+        assert phase_dot_class(phase) == "completed"
+
+    def test_review_no_verdict_gen2_returns_rework(self) -> None:
+        """review phase with no output_structured gen>1 falls back to rework logic."""
+        from raki.report.html_report import phase_dot_class
+
+        phase = self._make_phase(name="review", generation=2, status="completed")
+        assert phase_dot_class(phase) == "rework"
+
+    # --- non-verify/review phases ignore verdict ---
+
+    def test_implement_gen1_ignores_verdict_field(self) -> None:
+        """implement phase ignores output_structured.verdict; uses generation logic."""
+        from raki.report.html_report import phase_dot_class
+
+        phase = self._make_phase(
+            name="implement",
+            generation=1,
+            status="completed",
+            output_structured={"verdict": "FAIL"},
+        )
+        assert phase_dot_class(phase) == "completed"
+
+    def test_implement_gen2_ignores_verdict_field(self) -> None:
+        """implement gen>1 uses rework dot regardless of verdict field."""
+        from raki.report.html_report import phase_dot_class
+
+        phase = self._make_phase(
+            name="implement",
+            generation=2,
+            status="completed",
+            output_structured={"verdict": "PASS"},
+        )
+        assert phase_dot_class(phase) == "rework"
+
+
 # --- Ticket #250: Structured drill-down sections ---