ed
7bcb5a8c07
Eliminates 22 call sites that bypassed the AppController state owner
and read/wrote config.toml directly. AppController is now the single
source of truth for self.config; gui_2.py, commands.py, etc. go
through controller.save_config() / controller.load_config().
Production changes:
- src/models.py: rename load_config -> _load_config_from_disk,
save_config -> _save_config_to_disk (private I/O primitives)
- src/app_controller.py: add public load_config()/save_config() methods
that own the state. Update 3 internal call sites and 3 ConductorEngine
call sites to pass max_workers from self.config
- src/multi_agent_conductor.py: ConductorEngine.__init__ now takes
max_workers as a parameter (caller responsibility, not I/O primitive)
- src/external_editor.py: get_default_launcher() takes config as a
parameter; gui_2.py:1311,4776 pass app.config
- src/gui_2.py: 17 sites of models.save_config(X.config) replaced with
X.save_config() (delegates via __getattr__ to controller)
- src/commands.py: save_all() uses app.save_config()
Test changes (route through controller, not I/O primitive):
- tests/conftest.py: mock_app and app_instance fixtures now patch
AppController.load_config/save_config instead of models I/O primitives
- 18 other test files: patches renamed from models._save_config_to_disk
to AppController.save_config (and same for load_config)
- tests/test_app_controller_mcp.py: use SLOP_CONFIG env var instead of
patching removed CONFIG_PATH module constant
- tests/test_parallel_execution.py: pass max_workers=2 explicitly to
ConductorEngine (caller no longer reads config)
- tests/test_gui_paths.py: add save_config=MagicMock() to MockApp;
assert on controller method, not I/O primitive
- tests/test_models_no_top_level_tomli_w.py: still calls private
_save_config_to_disk directly (the only allowed exception; tests
the lazy-load behavior of the primitive itself)
New files:
- scripts/audit_no_models_config_io.py: enforces the rule (--strict,
--json modes; AST-based docstring detection to avoid false positives)
- conductor/code_styleguides/config_state_owner.md: documents the rule
Verification:
- 67 targeted tests pass
- scripts/audit_no_models_config_io.py --strict returns 0
This is the architectural cleanup that surfaced during the
audit_architectural_cheats_20260607 review. Closes the smoke-gun
CONFIG_PATH module constant (already done in 0c7ebf22) AND the
free-function models.load_config/save_config smell.
[conductor(checkpoint): config-iO-refactor-20260607]
63 lines
2.5 KiB
Python
63 lines
2.5 KiB
Python
import pytest
|
|
from src.models import Persona, Ticket, WorkerContext
|
|
from src import multi_agent_conductor
|
|
from src import app_controller
|
|
from src import aggregate
|
|
from pathlib import Path
|
|
from unittest.mock import MagicMock, patch
|
|
|
|
def test_persona_aggregation_strategy():
|
|
p = Persona(name="test_persona", aggregation_strategy="summarize")
|
|
assert p.aggregation_strategy == "summarize"
|
|
|
|
d = p.to_dict()
|
|
assert d.get("aggregation_strategy") == "summarize"
|
|
|
|
p2 = Persona.from_dict("test2", d)
|
|
assert p2.aggregation_strategy == "summarize"
|
|
|
|
@patch("src.aggregate.build_markdown_from_items")
|
|
def test_app_controller_do_generate_uses_persona_strategy(mock_build):
|
|
mock_build.return_value = "fake_md"
|
|
app = app_controller.AppController()
|
|
app.personas = {}
|
|
p = Persona(name="p1", aggregation_strategy="full")
|
|
app.personas["p1"] = p
|
|
app.ui_active_persona = "p1"
|
|
|
|
with patch("src.project_manager.flat_config", return_value={"project": {"name": "test"}, "output": {"output_dir": "."}, "files": {"paths": [], "base_dir": "."}}):
|
|
with patch("src.aggregate.build_file_items", return_value=[]):
|
|
with patch("pathlib.Path.mkdir"):
|
|
with patch("pathlib.Path.write_text"):
|
|
with patch.object(app, "_flush_to_project"):
|
|
with patch.object(app, "_flush_to_config"):
|
|
with patch("src.app_controller.AppController.save_config"):
|
|
full_md, path, file_items, stable_md, disc = app._do_generate()
|
|
|
|
# Verify aggregate.run and build_markdown_no_history received aggregation_strategy="full"
|
|
# Actually mock_build captures the inner calls of build_markdown_no_history
|
|
call_kwargs = mock_build.call_args[1]
|
|
assert call_kwargs.get("aggregation_strategy") == "full"
|
|
|
|
@patch("src.summarize.summarise_file")
|
|
@patch("src.multi_agent_conductor.ai_client.send")
|
|
def test_run_worker_lifecycle_uses_strategy(mock_send, mock_summarise, tmp_path):
|
|
mock_send.return_value = "fake response"
|
|
mock_summarise.return_value = "fake summary"
|
|
|
|
test_file = tmp_path / "test.py"
|
|
test_file.write_text("def test():\n pass")
|
|
|
|
ticket = Ticket(id="1", description="test")
|
|
context = WorkerContext(ticket_id="1", model_name="test", persona_id="test_persona")
|
|
|
|
p = Persona(name="test_persona", aggregation_strategy="summarize")
|
|
|
|
with patch("src.personas.PersonaManager.load_all", return_value={"test_persona": p}):
|
|
multi_agent_conductor.run_worker_lifecycle(
|
|
ticket, context, context_files=[str(test_file)]
|
|
)
|
|
|
|
# Should have called summarise_file because of the "summarize" strategy
|
|
mock_summarise.assert_called_once()
|