refactor(ticket): migrate Ticket consumers to direct field access (Phase 1)

TIER-2 READ AGENTS.md, conductor/workflow.md, conductor/edit_workflow.md,
conductor/tier2/githooks/forbidden-files.txt,
conductor/tracks/tier2_leak_prevention_20260620/spec.md,
conductor/code_styleguides/data_oriented_design.md,
conductor/code_styleguides/error_handling.md,
conductor/code_styleguides/type_aliases.md before Phase 1.

Phase 1 of metadata_promotion_20260624: migrate Ticket consumers from
t.get('key', default) / t['key'] to direct field access (t.id, t.status, etc.).

Changes:
- self.active_tickets: list[Metadata] -> list[models.Ticket]
- _deserialize_active_track_result populates self.active_tickets as Tickets
- _load_active_tickets (beads branch) constructs Ticket instances
- topological_sort signature: list[dict[str, Any]] -> list[Ticket]
- Migrated ~40 consumer sites in src/gui_2.py: _reorder_ticket,
  bulk_execute/skip/block, _cb_block_ticket, _cb_unblock_ticket,
  _dag_cycle_check_result, ticket queue rendering, DAG panel
- Migrated ~10 consumer sites in src/app_controller.py: _cb_ticket_retry,
  _cb_ticket_skip, approve_ticket, mutate_dag, _push_mma_state_update_result,
  completed count
- Removed legacy Ticket.get() compat method (Task 1.5)
- Added tests/test_metadata_promotion_phase1.py with 15 regression-guard tests
- Updated existing tests to construct Ticket instances instead of dicts

Verified: 1885 of 1910 unit tests pass (25 pre-existing failures unrelated
to Ticket migration; many are live_gui/sim tests that need a running GUI).
This commit is contained in:
ed
2026-06-25 18:20:45 -04:00
parent 9fdb7e0cc9
commit 0506c5da63
12 changed files with 358 additions and 171 deletions
+17 -16
View File
@@ -1,6 +1,7 @@
import unittest
from unittest.mock import patch
from src import conductor_tech_lead
from src.models import Ticket
from src.result_types import Result
import pytest
@@ -30,28 +31,28 @@ class TestConductorTechLead(unittest.TestCase):
class TestTopologicalSort(unittest.TestCase):
def test_topological_sort_linear(self) -> None:
tickets = [
{"id": "t2", "depends_on": ["t1"]},
{"id": "t1", "depends_on": []},
Ticket(id="t2", description="t2", depends_on=["t1"]),
Ticket(id="t1", description="t1", depends_on=[]),
]
sorted_tickets = conductor_tech_lead.topological_sort(tickets)
self.assertEqual(sorted_tickets[0]['id'], "t1")
self.assertEqual(sorted_tickets[1]['id'], "t2")
self.assertEqual(sorted_tickets[0].id, "t1")
self.assertEqual(sorted_tickets[1].id, "t2")
def test_topological_sort_complex(self) -> None:
tickets = [
{"id": "t3", "depends_on": ["t1", "t2"]},
{"id": "t1", "depends_on": []},
{"id": "t2", "depends_on": ["t1"]},
Ticket(id="t3", description="t3", depends_on=["t1", "t2"]),
Ticket(id="t1", description="t1", depends_on=[]),
Ticket(id="t2", description="t2", depends_on=["t1"]),
]
sorted_tickets = conductor_tech_lead.topological_sort(tickets)
self.assertEqual(sorted_tickets[0]['id'], "t1")
self.assertEqual(sorted_tickets[1]['id'], "t2")
self.assertEqual(sorted_tickets[2]['id'], "t3")
self.assertEqual(sorted_tickets[0].id, "t1")
self.assertEqual(sorted_tickets[1].id, "t2")
self.assertEqual(sorted_tickets[2].id, "t3")
def test_topological_sort_cycle(self) -> None:
tickets = [
{"id": "t1", "depends_on": ["t2"]},
{"id": "t2", "depends_on": ["t1"]},
Ticket(id="t1", description="t1", depends_on=["t2"]),
Ticket(id="t2", description="t2", depends_on=["t1"]),
]
with self.assertRaises(ValueError) as cm:
conductor_tech_lead.topological_sort(tickets)
@@ -65,7 +66,7 @@ class TestTopologicalSort(unittest.TestCase):
# If a ticket depends on something not in the list, we should handle it or let it fail.
# The TrackDAG silently ignores missing dependencies, causing cycle detection to trigger.
tickets = [
{"id": "t1", "depends_on": ["missing"]},
Ticket(id="t1", description="t1", depends_on=["missing"]),
]
# Currently this raises ValueError due to cycle detection on incomplete sort
with self.assertRaises(ValueError):
@@ -73,12 +74,12 @@ class TestTopologicalSort(unittest.TestCase):
def test_topological_sort_vlog(vlogger) -> None:
tickets = [
{"id": "t2", "depends_on": ["t1"]},
{"id": "t1", "depends_on": []},
Ticket(id="t2", description="t2", depends_on=["t1"]),
Ticket(id="t1", description="t1", depends_on=[]),
]
vlogger.log_state("Input Order", ["t2", "t1"], ["t2", "t1"])
sorted_tickets = conductor_tech_lead.topological_sort(tickets)
result_ids = [t['id'] for t in sorted_tickets]
result_ids = [t.id for t in sorted_tickets]
vlogger.log_state("Sorted Order", "N/A", result_ids)
assert result_ids == ["t1", "t2"]
vlogger.finalize("Topological Sort Verification", "PASS", "Linear dependencies correctly ordered.")
+5 -3
View File
@@ -2315,9 +2315,10 @@ def test_phase_10_l7271_dag_cycle_check_result_no_cycle():
opening the "Cycle Detected!" popup.
"""
from unittest.mock import MagicMock, patch
from src.models import Ticket
import src.gui_2 as gui2_mod
app = MagicMock()
app.active_tickets = [{"id": "T-001", "depends_on": []}]
app.active_tickets = [Ticket(id="T-001", description="T-001", depends_on=[])]
mock_dag = MagicMock()
mock_dag.has_cycle.return_value = False
with patch("src.dag_engine.TrackDAG", return_value=mock_dag):
@@ -2334,11 +2335,12 @@ def test_phase_10_l7271_dag_cycle_check_result_cycle_detected():
returns Result(data=True). The caller opens the "Cycle Detected!" popup.
"""
from unittest.mock import MagicMock, patch
from src.models import Ticket
import src.gui_2 as gui2_mod
app = MagicMock()
app.active_tickets = [
{"id": "T-001", "depends_on": ["T-002"]},
{"id": "T-002", "depends_on": ["T-001"]},
Ticket(id="T-001", description="T-001", depends_on=["T-002"]),
Ticket(id="T-002", description="T-002", depends_on=["T-001"]),
]
mock_dag = MagicMock()
mock_dag.has_cycle.return_value = True
+2 -2
View File
@@ -47,5 +47,5 @@ def test_load_active_tickets_from_beads(tmp_path: Path):
# 5. Verify active_tickets populated from Beads
assert len(ctrl.active_tickets) == 1
assert ctrl.active_tickets[0]["id"] == "bead-1"
assert ctrl.active_tickets[0]["description"] == "Description 1"
assert ctrl.active_tickets[0].id == "bead-1"
assert ctrl.active_tickets[0].description == "Description 1"
+2 -1
View File
@@ -1,5 +1,6 @@
import pytest
from unittest.mock import MagicMock, patch
from src import models
def test_gui_has_kill_button_method():
from src.gui_2 import App
@@ -36,7 +37,7 @@ def test_render_ticket_queue_table_columns():
from src.gui_2 import App, render_ticket_queue
app = App.__new__(App)
app.active_track = MagicMock()
app.active_tickets = [{"id": "T-001", "priority": "medium", "status": "in_progress", "description": "Test task"}]
app.active_tickets = [models.Ticket(id="T-001", description="Test task", priority="medium", status="in_progress")]
app.ui_selected_tickets = set()
app.ui_selected_ticket_id = None
app.controller = MagicMock()
+191
View File
@@ -0,0 +1,191 @@
"""
Phase 1 of metadata_promotion_20260624.
Verifies:
1. self.active_tickets load boundaries convert dicts to models.Ticket
2. conductor_tech_lead.topological_sort returns list[models.Ticket]
3. gui_2.py consumer sites use direct field access (not .get())
4. app_controller.py consumer sites use direct field access (not .get())
"""
import inspect
from unittest.mock import patch
from src.models import Ticket
class TestActiveTicketsType:
def test_active_tickets_annotation_is_list_of_ticket(self) -> None:
"""self.active_tickets type hint must be list[models.Ticket], not list[Metadata]."""
from src.app_controller import AppController
src_text = inspect.getsource(AppController.__init__)
assert "list[models.Ticket]" in src_text, (
"AppController.__init__ must declare self.active_tickets: list[models.Ticket]"
)
assert "list[Metadata]" not in src_text.split("self.active_tickets")[1].split("\n")[0], (
"AppController.__init__ must NOT declare self.active_tickets: list[Metadata]"
)
class TestActiveTicketsLoadBoundaries:
def test_load_at_data_converts_dicts_to_tickets(self) -> None:
"""_deserialize_active_track_result boundary must wrap dicts as models.Ticket."""
from src.app_controller import AppController
with patch.object(AppController, "load_config", return_value={
'ai': {'provider': 'gemini', 'model': 'gemini-2.5-flash-lite'},
'projects': {'paths': [], 'active': ''},
'gui': {'show_windows': {}},
}), patch.object(AppController, "save_config"), \
patch.object(AppController, "_prune_old_logs"), \
patch.object(AppController, "start_services"), \
patch.object(AppController, "_init_ai_and_hooks"):
ctrl = AppController.__new__(AppController)
ctrl.__init__()
at_data = {
"id": "track-x",
"title": "Track X",
"tickets": [
{"id": "T1", "description": "first", "status": "todo"},
{"id": "T2", "description": "second", "status": "todo"},
],
}
ctrl._deserialize_active_track_result(at_data)
assert ctrl.active_tickets, "load path should populate active_tickets"
for t in ctrl.active_tickets:
assert isinstance(t, Ticket), (
f"active_tickets must contain Ticket instances, got {type(t).__name__}: {t!r}"
)
def test_load_active_tickets_beads_branch_converts_dicts_to_tickets(self) -> None:
"""_load_active_tickets (beads branch) must wrap bead dicts as models.Ticket."""
from src.app_controller import AppController
from src.models import Ticket
ctrl = AppController.__new__(AppController)
ctrl._last_request_errors = []
ctrl.ui_project_execution_mode = "beads"
ctrl.ui_files_base_dir = None
class _Bead:
def __init__(self, bid: str, title: str, desc: str, status: str) -> None:
self.id = bid; self.title = title; self.description = desc; self.status = status
with patch.object(AppController, "_load_beads_from_path_result") as mock_load:
mock_load.return_value = (lambda: type("R", (), {"ok": True, "data": [
_Bead("B1", "T1", "first", "todo"), _Bead("B2", "T2", "second", "todo")
]})())
ctrl._load_active_tickets()
for t in ctrl.active_tickets:
assert isinstance(t, Ticket), (
f"beads branch must populate active_tickets with Ticket instances, got {type(t).__name__}"
)
class TestTopologicalSortReturnsTicketList:
def test_topological_sort_returns_ticket_instances(self) -> None:
"""conductor_tech_lead.topological_sort must return list[models.Ticket]."""
from src import conductor_tech_lead
sig = inspect.signature(conductor_tech_lead.topological_sort)
assert sig.return_annotation is not inspect.Signature.empty
assert "Ticket" in str(sig.return_annotation), (
f"topological_sort return annotation must reference Ticket, got {sig.return_annotation}"
)
class TestGuiConsumersDirectFieldAccess:
def test_reorder_ticket_uses_direct_field_access(self) -> None:
"""gui_2.App._reorder_ticket must use t.id / t.depends_on (not .get())."""
import inspect
from src import gui_2
src = inspect.getsource(gui_2.App._reorder_ticket)
assert "t.get(" not in src, (
"_reorder_ticket must not call t.get() — use t.id and t.depends_on directly"
)
def test_bulk_execute_uses_direct_field_access(self) -> None:
"""gui_2.App.bulk_execute must use t.id (not .get())."""
import inspect
from src import gui_2
src = inspect.getsource(gui_2.App.bulk_execute)
assert "t.get(" not in src, (
"bulk_execute must not call t.get() — use t.id directly"
)
def test_bulk_skip_uses_direct_field_access(self) -> None:
"""gui_2.App.bulk_skip must use t.id (not .get())."""
import inspect
from src import gui_2
src = inspect.getsource(gui_2.App.bulk_skip)
assert "t.get(" not in src, (
"bulk_skip must not call t.get() — use t.id directly"
)
def test_bulk_block_uses_direct_field_access(self) -> None:
"""gui_2.App.bulk_block must use t.id (not .get())."""
import inspect
from src import gui_2
src = inspect.getsource(gui_2.App.bulk_block)
assert "t.get(" not in src, (
"bulk_block must not call t.get() — use t.id directly"
)
def test_cb_block_ticket_uses_direct_field_access(self) -> None:
"""gui_2.App._cb_block_ticket must use direct field access (not .get())."""
import inspect
from src import gui_2
src = inspect.getsource(gui_2.App._cb_block_ticket)
assert "t.get(" not in src, (
"_cb_block_ticket must not call t.get() — use direct field access"
)
def test_cb_unblock_ticket_uses_direct_field_access(self) -> None:
"""gui_2.App._cb_unblock_ticket must use direct field access (not .get())."""
import inspect
from src import gui_2
src = inspect.getsource(gui_2.App._cb_unblock_ticket)
assert "t.get(" not in src, (
"_cb_unblock_ticket must not call t.get() — use direct field access"
)
def test_dag_cycle_check_uses_direct_field_access(self) -> None:
"""gui_2._dag_cycle_check_result must use t.id / t.depends_on (not .get())."""
import inspect
from src import gui_2
src = inspect.getsource(gui_2._dag_cycle_check_result)
assert "t.get(" not in src, (
"_dag_cycle_check_result must not call t.get() — use t.id and t.depends_on directly"
)
class TestAppControllerConsumersDirectFieldAccess:
def test_cb_ticket_retry_uses_direct_field_access(self) -> None:
"""app_controller._cb_ticket_retry must use t.id (not .get())."""
import inspect
from src import app_controller
src = inspect.getsource(app_controller.AppController._cb_ticket_retry)
assert "t.get(" not in src, (
"_cb_ticket_retry must not call t.get() — use t.id directly"
)
def test_cb_ticket_skip_uses_direct_field_access(self) -> None:
"""app_controller._cb_ticket_skip must use t.id (not .get())."""
import inspect
from src import app_controller
src = inspect.getsource(app_controller.AppController._cb_ticket_skip)
assert "t.get(" not in src, (
"_cb_ticket_skip must not call t.get() — use t.id directly"
)
def test_approve_ticket_uses_direct_field_access(self) -> None:
"""app_controller.approve_ticket must use t.id (not .get())."""
import inspect
from src import app_controller
src = inspect.getsource(app_controller.AppController.approve_ticket)
assert "t.get(" not in src, (
"approve_ticket must not call t.get() — use t.id directly"
)
def test_mutate_dag_uses_direct_field_access(self) -> None:
"""app_controller.mutate_dag must use t.id and t.depends_on (not .get())."""
import inspect
from src import app_controller
src = inspect.getsource(app_controller.AppController.mutate_dag)
assert "t.get(" not in src, (
"mutate_dag must not call t.get() — use t.id and t.depends_on directly"
)
+5 -4
View File
@@ -1,16 +1,17 @@
from src.gui_2 import App
from src.models import Ticket
def test_cb_ticket_retry(app_instance: App) -> None:
ticket_id = "test_ticket_1"
app_instance.active_tickets = [{"id": ticket_id, "status": "failed"}]
app_instance.active_tickets = [Ticket(id=ticket_id, description="test", status="failed")]
# Synchronous implementation does not use asyncio.run_coroutine_threadsafe
app_instance.controller._cb_ticket_retry(ticket_id)
# Verify status update
assert app_instance.active_tickets[0]['status'] == 'todo'
assert app_instance.active_tickets[0].status == 'todo'
def test_cb_ticket_skip(app_instance: App) -> None:
ticket_id = "test_ticket_2"
app_instance.active_tickets = [{"id": ticket_id, "status": "todo"}]
app_instance.active_tickets = [Ticket(id=ticket_id, description="test", status="todo")]
app_instance.controller._cb_ticket_skip(ticket_id)
# Verify status update
assert app_instance.active_tickets[0]['status'] == 'skipped'
assert app_instance.active_tickets[0].status == 'skipped'
+6 -6
View File
@@ -34,17 +34,17 @@ def test_generate_tickets() -> None:
def test_topological_sort() -> None:
tickets = [
{"id": "T2", "depends_on": ["T1"]},
{"id": "T1", "depends_on": []}
Ticket(id="T2", description="d2", depends_on=["T1"]),
Ticket(id="T1", description="d1", depends_on=[])
]
sorted_tickets = conductor_tech_lead.topological_sort(tickets)
assert sorted_tickets[0]["id"] == "T1"
assert sorted_tickets[1]["id"] == "T2"
assert sorted_tickets[0].id == "T1"
assert sorted_tickets[1].id == "T2"
def test_topological_sort_circular() -> None:
tickets = [
{"id": "T1", "depends_on": ["T2"]},
{"id": "T2", "depends_on": ["T1"]}
Ticket(id="T1", description="d1", depends_on=["T2"]),
Ticket(id="T2", description="d2", depends_on=["T1"])
]
with pytest.raises(ValueError, match="DAG Validation Error"):
conductor_tech_lead.topological_sort(tickets)
+27 -27
View File
@@ -40,70 +40,70 @@ def test_ticket_from_dict_default_priority():
class TestBulkOperations:
def test_bulk_execute(self, mock_app):
mock_app.active_tickets = [
{"id": "T1", "status": "todo"},
{"id": "T2", "status": "todo"},
{"id": "T3", "status": "todo"}
Ticket(id="T1", description="T1", status="todo"),
Ticket(id="T2", description="T2", status="todo"),
Ticket(id="T3", description="T3", status="todo")
]
mock_app.ui_selected_tickets = {"T1", "T3"}
with patch.object(mock_app.controller, "_push_mma_state_update") as mock_push:
mock_app.bulk_execute()
assert mock_app.active_tickets[0]["status"] == "in_progress"
assert mock_app.active_tickets[1]["status"] == "todo"
assert mock_app.active_tickets[2]["status"] == "in_progress"
assert mock_app.active_tickets[0].status == "in_progress"
assert mock_app.active_tickets[1].status == "todo"
assert mock_app.active_tickets[2].status == "in_progress"
mock_push.assert_called_once()
def test_bulk_skip(self, mock_app):
mock_app.active_tickets = [
{"id": "T1", "status": "todo"},
{"id": "T2", "status": "todo"}
Ticket(id="T1", description="T1", status="todo"),
Ticket(id="T2", description="T2", status="todo")
]
mock_app.ui_selected_tickets = {"T1"}
with patch.object(mock_app.controller, "_push_mma_state_update") as mock_push:
mock_app.bulk_skip()
assert mock_app.active_tickets[0]["status"] == "completed"
assert mock_app.active_tickets[1]["status"] == "todo"
assert mock_app.active_tickets[0].status == "completed"
assert mock_app.active_tickets[1].status == "todo"
mock_push.assert_called_once()
def test_bulk_block(self, mock_app):
mock_app.active_tickets = [
{"id": "T1", "status": "todo"},
{"id": "T2", "status": "todo"}
Ticket(id="T1", description="T1", status="todo"),
Ticket(id="T2", description="T2", status="todo")
]
mock_app.ui_selected_tickets = {"T1", "T2"}
with patch.object(mock_app.controller, "_push_mma_state_update") as mock_push:
mock_app.bulk_block()
assert mock_app.active_tickets[0]["status"] == "blocked"
assert mock_app.active_tickets[1]["status"] == "blocked"
assert mock_app.active_tickets[0].status == "blocked"
assert mock_app.active_tickets[1].status == "blocked"
mock_push.assert_called_once()
class TestReorder:
def test_reorder_ticket_valid(self, mock_app):
mock_app.active_tickets = [
{"id": "T1", "depends_on": []},
{"id": "T2", "depends_on": []},
{"id": "T3", "depends_on": ["T1"]}
Ticket(id="T1", description="T1", depends_on=[]),
Ticket(id="T2", description="T2", depends_on=[]),
Ticket(id="T3", description="T3", depends_on=["T1"])
]
with patch.object(mock_app.controller, "_push_mma_state_update") as mock_push:
# Move T1 to index 1: [T2, T1, T3]. T3 depends on T1. T1 index 1 < T3 index 2. VALID.
mock_app._reorder_ticket(0, 1)
assert mock_app.active_tickets[0]["id"] == "T2"
assert mock_app.active_tickets[1]["id"] == "T1"
assert mock_app.active_tickets[2]["id"] == "T3"
assert mock_app.active_tickets[0].id == "T2"
assert mock_app.active_tickets[1].id == "T1"
assert mock_app.active_tickets[2].id == "T3"
mock_push.assert_called_once()
def test_reorder_ticket_invalid(self, mock_app):
mock_app.active_tickets = [
{"id": "T1", "depends_on": []},
{"id": "T2", "depends_on": ["T1"]}
Ticket(id="T1", description="T1", depends_on=[]),
Ticket(id="T2", description="T2", depends_on=["T1"])
]
with patch.object(mock_app.controller, "_push_mma_state_update") as mock_push:
# Move T1 after T2: [T2, T1]. T2 depends on T1, but T1 is now at index 1 while T2 is at index 0.
# Violation: dependency T1 (index 1) is not before T2 (index 0).
mock_app._reorder_ticket(0, 1)
# Should NOT change
assert mock_app.active_tickets[0]["id"] == "T1"
assert mock_app.active_tickets[1]["id"] == "T2"
assert mock_app.active_tickets[0].id == "T1"
assert mock_app.active_tickets[1].id == "T2"
mock_push.assert_not_called()