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]
167 lines
6.2 KiB
Python
167 lines
6.2 KiB
Python
"""Audit script: ensure no production code in src/ calls the models I/O primitives directly.
|
|
|
|
Architecture rule: AppController owns the config I/O. The
|
|
models._load_config_from_disk and models._save_config_to_disk
|
|
functions are private file I/O primitives. Direct callers in src/
|
|
are an architectural smell (bypassing the controller state owner).
|
|
|
|
The only allowed call sites are inside AppController itself.
|
|
|
|
Usage:
|
|
python scripts/audit_no_models_config_io.py # human-readable report
|
|
python scripts/audit_no_models_config_io.py --json # JSON output for CI
|
|
python scripts/audit_no_models_config_io.py --strict # exit 1 on violations
|
|
"""
|
|
from __future__ import annotations
|
|
import argparse
|
|
import json
|
|
import os
|
|
import re
|
|
import sys
|
|
from pathlib import Path
|
|
|
|
# Patterns that are architectural smells in production code.
|
|
# These are the I/O primitives; only AppController should call them.
|
|
FORBIDDEN_PATTERNS = [
|
|
(re.compile(r"\bmodels\._load_config_from_disk\s*\("), "models._load_config_from_disk"),
|
|
(re.compile(r"\bmodels\._save_config_to_disk\s*\("), "models._save_config_to_disk"),
|
|
]
|
|
|
|
# The OLD public names. After the rename these should not exist anywhere.
|
|
LEGACY_NAMES = [
|
|
(re.compile(r"\bmodels\.load_config\s*\("), "models.load_config"),
|
|
(re.compile(r"\bmodels\.save_config\s*\("), "models.save_config"),
|
|
]
|
|
|
|
# Files where these calls are LEGITIMATE.
|
|
ALLOWED_FILES = {
|
|
"src/app_controller.py", # the only public owner of the I/O
|
|
"src/models.py", # the module that defines them
|
|
"tests/test_models_no_top_level_tomli_w.py", # tests lazy-load behavior
|
|
}
|
|
|
|
# Source roots to scan
|
|
SOURCE_ROOTS = ["src"]
|
|
|
|
|
|
def find_violations() -> list[dict[str, object]]:
|
|
"""Scan src/ for direct calls to the forbidden config I/O primitives."""
|
|
violations: list[dict[str, object]] = []
|
|
for root in SOURCE_ROOTS:
|
|
if not os.path.isdir(root):
|
|
continue
|
|
for dirpath, _dirs, files in os.walk(root):
|
|
for fname in files:
|
|
if not fname.endswith(".py"):
|
|
continue
|
|
path = os.path.join(dirpath, fname)
|
|
# Normalize to forward slashes for matching
|
|
norm = path.replace(os.sep, "/")
|
|
if norm in ALLOWED_FILES:
|
|
continue
|
|
with open(path, encoding="utf-8", errors="replace") as f:
|
|
src = f.read()
|
|
docstring_lines = _docstring_lines(src)
|
|
for lineno, line in enumerate(src.splitlines(), start=1):
|
|
if lineno in docstring_lines:
|
|
continue
|
|
stripped = line.lstrip()
|
|
if stripped.startswith("#"):
|
|
continue
|
|
for pattern, name in FORBIDDEN_PATTERNS:
|
|
if pattern.search(line):
|
|
violations.append({
|
|
"file": path,
|
|
"line": lineno,
|
|
"pattern": name,
|
|
"text": line.rstrip(),
|
|
"severity": "error",
|
|
})
|
|
for pattern, name in LEGACY_NAMES:
|
|
if pattern.search(line):
|
|
violations.append({
|
|
"file": path,
|
|
"line": lineno,
|
|
"pattern": name,
|
|
"text": line.rstrip(),
|
|
"severity": "error",
|
|
})
|
|
return violations
|
|
|
|
|
|
def _docstring_lines(src: str) -> set[int]:
|
|
"""Return a set of 1-based line numbers that are inside a docstring.
|
|
|
|
Uses the AST to find module/class/function docstrings, then expands
|
|
the string node's line range. Multi-line strings are included in
|
|
full so any code-looking text inside them is ignored.
|
|
"""
|
|
import ast
|
|
lines: set[int] = set()
|
|
try:
|
|
tree = ast.parse(src)
|
|
except SyntaxError:
|
|
return lines
|
|
for node in ast.walk(tree):
|
|
if not isinstance(node, (ast.Module, ast.FunctionDef, ast.AsyncFunctionDef,
|
|
ast.ClassDef)):
|
|
continue
|
|
doc = ast.get_docstring(node, clean=False)
|
|
if not doc or not node.body:
|
|
continue
|
|
first = node.body[0]
|
|
if not isinstance(first, ast.Expr) or not isinstance(first.value, ast.Constant):
|
|
continue
|
|
if not isinstance(first.value.value, str):
|
|
continue
|
|
start = first.lineno
|
|
end = getattr(first, "end_lineno", None) or start
|
|
for ln in range(start, end + 1):
|
|
lines.add(ln)
|
|
return lines
|
|
|
|
|
|
def main() -> int:
|
|
parser = argparse.ArgumentParser(
|
|
description="Audit for direct calls to models config I/O primitives in src/"
|
|
)
|
|
parser.add_argument("--json", action="store_true", help="JSON output for CI")
|
|
parser.add_argument("--strict", action="store_true", help="Exit 1 on any violation")
|
|
args = parser.parse_args()
|
|
|
|
violations = find_violations()
|
|
|
|
if args.json:
|
|
print(json.dumps({"violations": violations, "count": len(violations)}, indent=2))
|
|
else:
|
|
print("=" * 70)
|
|
print("Architectural audit: models config I/O usage in src/")
|
|
print("=" * 70)
|
|
print()
|
|
print("Rule: AppController owns config I/O. Direct calls to")
|
|
print(" - models._load_config_from_disk(...)")
|
|
print(" - models._save_config_to_disk(...)")
|
|
print(" - models.load_config(...) (legacy)")
|
|
print(" - models.save_config(...) (legacy)")
|
|
print("from outside AppController are architectural smells.")
|
|
print()
|
|
print(f"Allowed call sites: {sorted(ALLOWED_FILES)}")
|
|
print()
|
|
if not violations:
|
|
print("OK - no violations found.")
|
|
else:
|
|
print(f"Found {len(violations)} violation(s):")
|
|
print()
|
|
for v in violations:
|
|
print(f" {v['file']}:{v['line']}: {v['pattern']}")
|
|
print(f" {v['text']}")
|
|
print()
|
|
|
|
if args.strict and violations:
|
|
return 1
|
|
return 0
|
|
|
|
|
|
if __name__ == "__main__":
|
|
sys.exit(main())
|