sigh
This commit is contained in:
@@ -0,0 +1,61 @@
|
||||
{
|
||||
"track_id": "mma_tier_usage_reset_fix_20260610",
|
||||
"name": "Fix mma_tier_usage reset + 2 pre-existing controller bugs (2026-06-10)",
|
||||
"created_at": "2026-06-10",
|
||||
"status": "spec",
|
||||
"priority": "A",
|
||||
"blocked_by": [],
|
||||
"blocks": [],
|
||||
"inherits_from": [
|
||||
"conductor/tracks/workspace_path_finalize_20260609/"
|
||||
],
|
||||
"supersedes": [],
|
||||
"domain": "AppController (test infrastructure)",
|
||||
"scope_summary": "Four surgical fixes in src/app_controller.py: (FR1) pre-populate mma_tier_usage on reset (matches __init__ defaults) so _flush_to_project doesn't crash with KeyError; (FR2) make _flush_to_project defensive against missing 'model' key; (FR3) re-add self.context_preset_manager = ContextPresetManager() init that was lost in 72f8f466; (FR4) remove 'persona_manager' from _LAZY_MANAGER_DEFAULTS in __getattr__ because the comment is wrong (returning None makes hasattr() return True, not False).",
|
||||
"estimated_effort": "1.5 hours",
|
||||
"phases": 1,
|
||||
"verification_criteria": [
|
||||
"src/app_controller.py:3409 pre-populates mma_tier_usage with the full default shape (input, output, provider, model, tool_preset for all 4 tiers)",
|
||||
"src/app_controller.py:2639 uses d.get('model') instead of d['model']",
|
||||
"src/app_controller.py:__init__ contains self.context_preset_manager = ContextPresetManager()",
|
||||
"src/app_controller.py:1266-1275 does NOT contain 'persona_manager' in _LAZY_MANAGER_DEFAULTS",
|
||||
"A new unit test in tests/test_mma_tier_usage_reset_fix.py verifies the post-reset flush does not raise KeyError",
|
||||
"tests/test_reset_session_clears_mma_and_rag.py (3 tests) still pass",
|
||||
"tests/test_context_presets_manager.py::test_app_controller_save_load passes",
|
||||
"tests/test_project_switch_persona_preset.py::test_load_active_project_creates_persona_manager passes",
|
||||
"tests/test_project_switch_persona_preset.py::test_load_context_preset_missing_raises_keyerror passes",
|
||||
"All 4 tests in tests/test_extended_sims.py pass in batch (test_context_sim_live, test_ai_settings_sim_live, test_tools_sim_live, test_execution_sim_live)",
|
||||
"Tier-1 batch: 5/5 pass",
|
||||
"Tier-2 batch: 5/5 pass",
|
||||
"Tier-3 batch: 0 new failures vs 33d02bb1 baseline"
|
||||
],
|
||||
"out_of_scope": [
|
||||
"Refactoring _switch_project to use a state machine",
|
||||
"Removing the recursive re-switch in _do_project_switch's finally",
|
||||
"Removing the other 5 names from _LAZY_MANAGER_DEFAULTS (context_preset_manager, tool_preset_manager, preset_manager, vendor_state, perf_monitor) — only persona_manager is removed in this track",
|
||||
"Modifying the 3 tests in tests/test_reset_session_clears_mma_and_rag.py",
|
||||
"Modifying tests/test_context_presets_manager.py::test_app_controller_save_load",
|
||||
"Modifying tests/test_project_switch_persona_preset.py::test_load_active_project_creates_persona_manager",
|
||||
"Modifying tests/test_project_switch_persona_preset.py::test_load_context_preset_missing_raises_keyerror",
|
||||
"Refactoring simulation/sim_base.py or simulation/sim_context.py",
|
||||
"Adding new audit scripts",
|
||||
"Doc updates",
|
||||
"Follow-up tracks",
|
||||
"Any 'while we're at it' refactors"
|
||||
],
|
||||
"risks": [
|
||||
{
|
||||
"risk": "The pre-populated default values drift from the __init__ values over time (someone changes one but not the other)",
|
||||
"mitigation": "Add a comment in the reset code pointing to the __init__ shape; both sites should be updated together. Out of scope for this track to extract a shared constant."
|
||||
},
|
||||
{
|
||||
"risk": "Defense-in-depth change at line 2639 silently drops 'model' from the saved project, causing the next load to lose data",
|
||||
"mitigation": "The d.get('model') fallback writes None when the key is missing, which is a better failure mode than a crash. The test_extended_sims tests use gemini_cli (not affected). A test asserts the saved value matches the pre-populated default."
|
||||
},
|
||||
{
|
||||
"risk": "Removing 'persona_manager' from _LAZY_MANAGER_DEFAULTS breaks code that does getattr(ctrl, 'persona_manager', None) or relies on the lazy fallback",
|
||||
"mitigation": "The track verifies in the full batch run. If any other test fails due to the change, file a follow-up. The minimal change is to remove only 'persona_manager' (the one the failing test asserts on)."
|
||||
}
|
||||
],
|
||||
"tier_2_supervision_required_for": []
|
||||
}
|
||||
@@ -0,0 +1,583 @@
|
||||
# `mma_tier_usage` Reset Fix — Implementation Plan
|
||||
|
||||
> **For Tier 3 workers:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
|
||||
>
|
||||
> **Scope is exactly 4 surgical edits in `src/app_controller.py` + 2 new regression tests. Do not refactor anything else. Do not add new tests beyond the 2 in this plan. Do not update docs. Do not file follow-up tracks. Execute exactly what is here, then stop.**
|
||||
|
||||
**Goal:** Fix 3 pre-existing bugs in `src/app_controller.py` that surface during the test suite:
|
||||
- **FR1+FR2:** Restore the pre-`fe240db4` contract that `_flush_to_project` requires (every `mma_tier_usage[tier]` entry has a `model` key), and harden `_flush_to_project` so it does not crash if a future code path produces a partial entry.
|
||||
- **FR3:** Re-add the `self.context_preset_manager = ContextPresetManager()` init line that was lost in `72f8f466`. Without it, `save_context_preset` and `load_context_preset` crash.
|
||||
- **FR4:** Remove `persona_manager` from `_LAZY_MANAGER_DEFAULTS` in `__getattr__` (the comment is wrong; `__getattr__` returning None makes `hasattr()` return True, breaking `test_load_active_project_creates_persona_manager`).
|
||||
|
||||
**Architecture:** Four surgical edits in `src/app_controller.py`. No new modules, no new helpers, no API changes.
|
||||
|
||||
**Tech Stack:** Python 3.11+, pytest.
|
||||
|
||||
**HARD CONSTRAINTS (from `AGENTS.md` and `conductor/edit_workflow.md`):**
|
||||
- **NEVER** use `git checkout -- <file>`, `git restore`, `git reset`, or any other form of pre-fix replay (including scratch reproduction scripts that simulate the pre-fix state). The user explicitly banned all of these. They destroyed user in-progress work twice. Step 3.1.4 is intentionally a no-op; the 3rd regression test's docstring explains the pre-fix failure mode in prose as a substitute.
|
||||
- **1-space indent, CRLF, type hints.** Per project conventions.
|
||||
|
||||
---
|
||||
|
||||
## Pre-Phase 0: Checkpoint
|
||||
|
||||
- [ ] **Step 0.1: Pre-edit checkpoint**
|
||||
```powershell
|
||||
cd C:\projects\manual_slop; git add . && git commit -m "wip: pre-mma-tier-usage-reset-fix" --allow-empty
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Phase 1: Apply FR1 (pre-populate `mma_tier_usage` on reset)
|
||||
|
||||
Focus: Restore the pre-`fe240db4` shape of `mma_tier_usage` in `_handle_reset_session`.
|
||||
|
||||
### Task 1.1: Read the current state of `_handle_reset_session`
|
||||
|
||||
- [ ] **Step 1.1.1: Read the exact lines**
|
||||
Use `manual-slop_get_file_slice` to read `src/app_controller.py:3407-3411`. Confirm the current shape is `{'Tier 1': {}, 'Tier 2': {}, 'Tier 3': {}, 'Tier 4': {}}` (empty dicts) on line 3409, with the comment `# Reset mma_tier_usage to pre-populated default (prior tests pollute it)` on line 3408.
|
||||
|
||||
### Task 1.2: Apply the edit
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/app_controller.py:3409` (the empty-dict reset)
|
||||
|
||||
- [ ] **Step 1.2.1: Replace the empty-dict reset with the pre-populated default**
|
||||
|
||||
Change FROM:
|
||||
```python
|
||||
# Reset mma_tier_usage to pre-populated default (prior tests pollute it)
|
||||
self.mma_tier_usage = {'Tier 1': {}, 'Tier 2': {}, 'Tier 3': {}, 'Tier 4': {}}
|
||||
```
|
||||
|
||||
Change TO:
|
||||
```python
|
||||
# Reset mma_tier_usage to the same shape as __init__ (line 952-957). Prior
|
||||
# tests pollute it; downstream consumers like _flush_to_project require
|
||||
# every tier entry to have 'model' / 'provider' / 'tool_preset' keys. The
|
||||
# pre-populated defaults (input=0, output=0, provider='gemini', model=
|
||||
# tier default, tool_preset=None) restore the contract without retaining
|
||||
# any polluted model names or token counts from a prior session.
|
||||
self.mma_tier_usage = {
|
||||
"Tier 1": {"input": 0, "output": 0, "provider": "gemini", "model": "gemini-3.1-pro-preview", "tool_preset": None},
|
||||
"Tier 2": {"input": 0, "output": 0, "provider": "gemini", "model": "gemini-3-flash-preview", "tool_preset": None},
|
||||
"Tier 3": {"input": 0, "output": 0, "provider": "gemini", "model": "gemini-2.5-flash-lite", "tool_preset": None},
|
||||
"Tier 4": {"input": 0, "output": 0, "provider": "gemini", "model": "gemini-2.5-flash-lite", "tool_preset": None},
|
||||
}
|
||||
```
|
||||
|
||||
Use `manual-slop_set_file_slice` with the exact start_line and end_line of the block. Verify the slice boundaries with `manual-slop_get_file_slice` first.
|
||||
|
||||
**CRITICAL — 1-space indent.** The dict values (the per-tier dicts) use 1-space indent. The outer dict has no indent. Match the existing project convention exactly.
|
||||
|
||||
**CRITICAL — Do NOT use empty dicts.** Empty dicts cause the test to fail. The whole point of this fix is to pre-populate.
|
||||
|
||||
- [ ] **Step 1.2.2: Verify syntax**
|
||||
```powershell
|
||||
cd C:\projects\manual_slop; uv run python -c "import ast; ast.parse(open('src/app_controller.py').read()); print('OK')"
|
||||
```
|
||||
|
||||
- [ ] **Step 1.2.3: Verify the import is still valid**
|
||||
```powershell
|
||||
cd C:\projects\manual_slop; uv run python -c "from src.app_controller import AppController; print('import OK')"
|
||||
```
|
||||
|
||||
### Task 1.3: Commit FR1
|
||||
|
||||
- [ ] **Step 1.3.1: Commit the FR1 change**
|
||||
```powershell
|
||||
cd C:\projects\manual_slop; git add src/app_controller.py
|
||||
git commit -m "fix(controller): pre-populate mma_tier_usage on reset (restore _flush_to_project contract)"
|
||||
$h = git log -1 --format='%H'
|
||||
git notes add -m "Reverts fe240db4's empty-dict reset to the pre-populated default (matching __init__ at line 952-957). The empty-dict reset broke _flush_to_project at line 2639, which does d['model'] and raised KeyError. The crash then caused _do_project_switch's finally block to re-queue the switch infinitely, which is why test_context_sim_live saw the 'switching to: temp_livecontextsim (stale ui - ops disabled)' status for 60+ seconds. 1 file changed, ~10 lines." $h
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Phase 2: Apply FR2 (defensive `_flush_to_project`)
|
||||
|
||||
Focus: Make `_flush_to_project` not crash if a future code path produces a partial `mma_tier_usage[tier]` entry.
|
||||
|
||||
### Task 2.1: Read the current state of `_flush_to_project`
|
||||
|
||||
- [ ] **Step 2.1.1: Read the exact line**
|
||||
Use `manual-slop_get_file_slice` to read `src/app_controller.py:2638-2640`. Confirm line 2639 is:
|
||||
```python
|
||||
mma_sec["tier_models"] = {t: {"model": d["model"], "provider": d.get("provider", "gemini"), "tool_preset": d.get("tool_preset")} for t, d in self.mma_tier_usage.items()}
|
||||
```
|
||||
|
||||
### Task 2.2: Apply the edit
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/app_controller.py:2639`
|
||||
|
||||
- [ ] **Step 2.2.1: Replace `d["model"]` with `d.get("model")`**
|
||||
|
||||
Change FROM:
|
||||
```python
|
||||
mma_sec["tier_models"] = {t: {"model": d["model"], "provider": d.get("provider", "gemini"), "tool_preset": d.get("tool_preset")} for t, d in self.mma_tier_usage.items()}
|
||||
```
|
||||
|
||||
Change TO:
|
||||
```python
|
||||
mma_sec["tier_models"] = {t: {"model": d.get("model"), "provider": d.get("provider", "gemini"), "tool_preset": d.get("tool_preset")} for t, d in self.mma_tier_usage.items()}
|
||||
```
|
||||
|
||||
Use `manual-slop_set_file_slice` with the exact start_line and end_line.
|
||||
|
||||
**CRITICAL — Do not change `d.get("provider", ...)` or `d.get("tool_preset")`.** Only `d["model"]` becomes `d.get("model")`.
|
||||
|
||||
- [ ] **Step 2.2.2: Verify syntax**
|
||||
```powershell
|
||||
cd C:\projects\manual_slop; uv run python -c "import ast; ast.parse(open('src/app_controller.py').read()); print('OK')"
|
||||
```
|
||||
|
||||
- [ ] **Step 2.2.3: Verify the import is still valid**
|
||||
```powershell
|
||||
cd C:\projects\manual_slop; uv run python -c "from src.app_controller import AppController; print('import OK')"
|
||||
```
|
||||
|
||||
### Task 2.3: Commit FR2
|
||||
|
||||
- [ ] **Step 2.3.1: Commit the FR2 change**
|
||||
```powershell
|
||||
cd C:\projects\manual_slop; git add src/app_controller.py
|
||||
git commit -m "fix(controller): _flush_to_project defensive against missing 'model' key"
|
||||
$h = git log -1 --format='%H'
|
||||
git notes add -m "Defense in depth. d['model'] is replaced with d.get('model') so a future code path that produces a partial mma_tier_usage[tier] dict (e.g. _handle_mma_state_update at line 484-497 does controller.mma_tier_usage[tier] = data) doesn't crash the project save. The other .get() calls (provider, tool_preset) were already defensive; this aligns the model lookup. 1 file changed, 1 line." $h
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Phase 3: Apply FR3 (re-add `context_preset_manager` init)
|
||||
|
||||
Focus: Restore the `self.context_preset_manager = ContextPresetManager()` init line that was lost in `72f8f466`.
|
||||
|
||||
### Task 3.1: Read the current state of `__init__`
|
||||
|
||||
- [ ] **Step 3.1.1: Read the exact lines around the insertion point**
|
||||
Use `manual-slop_get_file_slice` to read `src/app_controller.py:1182-1186`. Confirm the current shape is:
|
||||
```python
|
||||
})
|
||||
self.perf_monitor = performance_monitor.get_monitor()
|
||||
```
|
||||
|
||||
### Task 3.2: Apply the edit
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/app_controller.py` (insert one line between line 1183 and 1185)
|
||||
|
||||
- [ ] **Step 3.2.1: Insert the `context_preset_manager` init**
|
||||
|
||||
Change FROM:
|
||||
```python
|
||||
})
|
||||
self.perf_monitor = performance_monitor.get_monitor()
|
||||
```
|
||||
|
||||
Change TO:
|
||||
```python
|
||||
})
|
||||
self.context_preset_manager = ContextPresetManager()
|
||||
self.perf_monitor = performance_monitor.get_monitor()
|
||||
```
|
||||
|
||||
Use `manual-slop_set_file_slice` with the exact start_line and end_line of the 2-line block (the `})` close brace and the `self.perf_monitor` line). Replace with the 3-line block above.
|
||||
|
||||
**CRITICAL — Use exactly 1-space indent.** The `})` line has no indent (it's a closing brace at the module level). The new `self.context_preset_manager` line has 1 space. The `self.perf_monitor` line has 1 space. Match the surrounding style exactly.
|
||||
|
||||
**CRITICAL — Use the exact same spacing and double-space alignment** as the `c039fdbb` version: `self.context_preset_manager = ContextPresetManager()` (2 spaces before the `=`). The 2-space alignment matches the `self.perf_monitor = ...` and `self._perf_profiling_enabled = ...` lines around it.
|
||||
|
||||
- [ ] **Step 3.2.2: Verify syntax**
|
||||
```powershell
|
||||
cd C:\projects\manual_slop; uv run python -c "import ast; ast.parse(open('src/app_controller.py').read()); print('OK')"
|
||||
```
|
||||
|
||||
- [ ] **Step 3.2.3: Verify the import is still valid**
|
||||
```powershell
|
||||
cd C:\projects\manual_slop; uv run python -c "from src.app_controller import AppController; ctrl = AppController(); print('context_preset_manager:', type(ctrl.context_preset_manager).__name__)"
|
||||
```
|
||||
Expected output: `context_preset_manager: ContextPresetManager`
|
||||
|
||||
- [ ] **Step 3.2.4: Verify `hasattr` semantics on a bare AppController**
|
||||
The bug we're fixing requires `context_preset_manager` to be set so `save_context_preset` and `load_context_preset` work. But we still want `__getattr__` to handle OTHER missing attrs. Verify with:
|
||||
```powershell
|
||||
cd C:\projects\manual_slop; uv run python -c "from src.app_controller import AppController; ctrl = AppController(); print('has context_preset_manager:', hasattr(ctrl, 'context_preset_manager'))"
|
||||
```
|
||||
Expected: `has context_preset_manager: True`
|
||||
|
||||
### Task 3.3: Commit FR3
|
||||
|
||||
- [ ] **Step 3.3.1: Commit the FR3 change**
|
||||
```powershell
|
||||
cd C:\projects\manual_slop; git add src/app_controller.py
|
||||
git commit -m "fix(controller): re-add self.context_preset_manager init (lost in 72f8f466)"
|
||||
$h = git log -1 --format='%H'
|
||||
git notes add -m "Re-adds the self.context_preset_manager = ContextPresetManager() line that was in c039fdbb but accidentally dropped during a hand-edited refactor of the _settable_fields block in 72f8f466. Without this init, save_context_preset and load_context_preset crash with AttributeError: 'NoneType' object has no attribute 'save_preset' (or 'load_all'). The ContextPresetManager import was already at the top of the file (line 41), so no new import is needed. 1 file changed, 1 line." $h
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Phase 4: Apply FR4 (remove `persona_manager` from `_LAZY_MANAGER_DEFAULTS`)
|
||||
|
||||
Focus: Make `hasattr(ctrl, "persona_manager")` return False for a fresh `AppController()` so the regression test `test_load_active_project_creates_persona_manager` passes.
|
||||
|
||||
### Task 4.1: Read the current state of `_LAZY_MANAGER_DEFAULTS`
|
||||
|
||||
- [ ] **Step 4.1.1: Read the exact lines**
|
||||
Use `manual-slop_get_file_slice` to read `src/app_controller.py:1260-1281`. Confirm the current shape is:
|
||||
```python
|
||||
_LAZY_MANAGER_DEFAULTS = {
|
||||
"context_preset_manager",
|
||||
"persona_manager",
|
||||
"tool_preset_manager",
|
||||
"preset_manager",
|
||||
"vendor_state",
|
||||
"perf_monitor",
|
||||
}
|
||||
```
|
||||
|
||||
### Task 4.2: Apply the edit
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/app_controller.py:1267` (the `"persona_manager"` line in `_LAZY_MANAGER_DEFAULTS`)
|
||||
|
||||
- [ ] **Step 4.2.1: Remove `"persona_manager"` from the set**
|
||||
|
||||
Change FROM:
|
||||
```python
|
||||
_LAZY_MANAGER_DEFAULTS = {
|
||||
"context_preset_manager",
|
||||
"persona_manager",
|
||||
"tool_preset_manager",
|
||||
"preset_manager",
|
||||
"vendor_state",
|
||||
"perf_monitor",
|
||||
}
|
||||
```
|
||||
|
||||
Change TO:
|
||||
```python
|
||||
_LAZY_MANAGER_DEFAULTS = {
|
||||
"context_preset_manager",
|
||||
"tool_preset_manager",
|
||||
"preset_manager",
|
||||
"vendor_state",
|
||||
"perf_monitor",
|
||||
}
|
||||
```
|
||||
|
||||
Use `manual-slop_set_file_slice` with the exact start_line and end_line of the block.
|
||||
|
||||
**CRITICAL — Keep the other 5 names.** Only `"persona_manager"` is removed in this FR. The other 5 may have lazy-default callers that need verification in the batch run. Removing them is a follow-up.
|
||||
|
||||
- [ ] **Step 4.2.2: Update the misleading comment above the set**
|
||||
|
||||
Change FROM:
|
||||
```python
|
||||
# Manager attributes that are initialized by init_state() but are absent
|
||||
# on a bare AppController() (which some tests construct). Return None
|
||||
# for these so test code that references them without calling init_state
|
||||
# does not crash. hasattr() still returns False for non-mocked access
|
||||
# paths because callers wrap in try/except for AttributeError when they
|
||||
# need to distinguish "lazy" from "absent".
|
||||
```
|
||||
|
||||
Change TO:
|
||||
```python
|
||||
# Manager attributes that are initialized by init_state() but are absent
|
||||
# on a bare AppController() (which some tests construct). Return None
|
||||
# for these so test code that references them without calling init_state
|
||||
# does not crash. NOTE: callers that need to distinguish "lazy" from
|
||||
# "absent" must use try/except AttributeError explicitly; hasattr()
|
||||
# returns True because __getattr__ returns None (a valid attribute
|
||||
# value).
|
||||
```
|
||||
|
||||
Use `manual-slop_set_file_slice` with the exact start_line and end_line of the comment block.
|
||||
|
||||
- [ ] **Step 4.2.3: Verify syntax**
|
||||
```powershell
|
||||
cd C:\projects\manual_slop; uv run python -c "import ast; ast.parse(open('src/app_controller.py').read()); print('OK')"
|
||||
```
|
||||
|
||||
- [ ] **Step 4.2.4: Verify the import is still valid**
|
||||
```powershell
|
||||
cd C:\projects\manual_slop; uv run python -c "from src.app_controller import AppController; ctrl = AppController(); print('has persona_manager:', hasattr(ctrl, 'persona_manager'))"
|
||||
```
|
||||
Expected: `has persona_manager: False`
|
||||
|
||||
- [ ] **Step 4.2.5: Verify `_load_active_project` still sets `persona_manager`**
|
||||
The fix only changes `__getattr__` behavior for missing attrs. After `_load_active_project()` is called, `persona_manager` should be a real `PersonaManager` instance.
|
||||
```powershell
|
||||
cd C:\projects\manual_slop; uv run python -c "from src.app_controller import AppController; ctrl = AppController(); ctrl.active_project_path = 'tests/artifacts/temp_livecontextsim.toml'; ctrl._load_active_project(); print('has persona_manager after load:', hasattr(ctrl, 'persona_manager')); print('type:', type(ctrl.persona_manager).__name__)"
|
||||
```
|
||||
Expected: `has persona_manager after load: True` and `type: PersonaManager` (or similar — the test only requires `hasattr` to be True after `_load_active_project`).
|
||||
|
||||
If the actual `temp_livecontextsim.toml` file doesn't exist, that's OK — `_load_active_project` may log a warning but should still set `persona_manager`. If the test fails because the file doesn't exist, skip this verification step.
|
||||
|
||||
### Task 4.3: Commit FR4
|
||||
|
||||
- [ ] **Step 4.3.1: Commit the FR4 change**
|
||||
```powershell
|
||||
cd C:\projects\manual_slop; git add src/app_controller.py
|
||||
git commit -m "fix(controller): remove 'persona_manager' from _LAZY_MANAGER_DEFAULTS"
|
||||
$h = git log -1 --format='%H'
|
||||
git notes add -m "Removes 'persona_manager' from the _LAZY_MANAGER_DEFAULTS set in __getattr__. The original code returned None for these attrs, but the accompanying comment claimed hasattr() returns False (which is wrong — __getattr__ returning None makes hasattr() return True). The test test_load_active_project_creates_persona_manager asserts not hasattr(ctrl, 'persona_manager') for a fresh controller, which is the correct Python semantics. The other 5 names in the set are kept; they may have lazy-default callers that need verification in the batch run. 1 file changed, comment + 1 line." $h
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Phase 5: Add 4 regression tests
|
||||
|
||||
Focus: Unit tests that prove the fixes prevent the original failures. Two for FR1+FR2 (post-reset flush), one for FR3 (context_preset_manager is callable), one for FR4 (persona_manager hasattr semantics).
|
||||
|
||||
### Task 5.1: Write the regression tests
|
||||
|
||||
**Files:**
|
||||
- Create: `tests/test_mma_tier_usage_reset_fix.py`
|
||||
|
||||
- [ ] **Step 5.1.1: Write the test file**
|
||||
Create `tests/test_mma_tier_usage_reset_fix.py` with the following content:
|
||||
```python
|
||||
"""Regression tests for 3 pre-existing bugs in AppController.
|
||||
|
||||
Bug 1: _handle_reset_session zeroes mma_tier_usage to empty dicts; the downstream
|
||||
_flush_to_project crashes with KeyError: 'model'. (Commits fe240db4 introduced.)
|
||||
Bug 2: __init__ does not set self.context_preset_manager; save_context_preset
|
||||
and load_context_preset crash. (Lost in 72f8f466.)
|
||||
Bug 3: __getattr__ returns None for 'persona_manager', making hasattr() return
|
||||
True (the accompanying comment claims False, which is wrong).
|
||||
|
||||
The integration symptom of Bug 1 was test_context_sim_live polling ai_status
|
||||
for 60s and seeing the constant 'switching to: temp_livecontextsim (stale ui -
|
||||
ops disabled)' string (older runs) or 'error: \\'model\\'' (newer runs after
|
||||
sim_context.py added an 'error in s' early-break check).
|
||||
|
||||
These tests exercise the exact code paths that were crashing, in isolation,
|
||||
to prove the fixes prevent the original failures.
|
||||
|
||||
The tests do NOT require the live_gui fixture. They use a real AppController()
|
||||
with a tmp_path for the project file, matching the pattern in
|
||||
tests/test_handle_reset_session_clears_project.py.
|
||||
"""
|
||||
import pytest
|
||||
import tomllib
|
||||
from pathlib import Path
|
||||
|
||||
from src.app_controller import AppController
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def controller(tmp_path: Path) -> AppController:
|
||||
"""Build a real AppController with a writable project file."""
|
||||
proj_path = tmp_path / "test_project.toml"
|
||||
proj_path.write_text("[project]\nname = 'TestProject'\n")
|
||||
ctrl = AppController()
|
||||
ctrl.active_project_path = str(proj_path)
|
||||
yield ctrl
|
||||
|
||||
|
||||
def test_reset_session_makes_flush_to_project_not_crash(controller: AppController) -> None:
|
||||
"""Bug 1 fix: After _handle_reset_session, _flush_to_project must not raise KeyError.
|
||||
|
||||
Pre-fix: the reset zeroes mma_tier_usage to empty dicts; _flush_to_project
|
||||
crashes on d['model']. Post-fix: the reset pre-populates the dicts (matching
|
||||
__init__ defaults), and _flush_to_project uses d.get('model') as a defensive
|
||||
fallback. This test asserts the round-trip works.
|
||||
"""
|
||||
for tier in ("Tier 1", "Tier 2", "Tier 3", "Tier 4"):
|
||||
assert "model" in controller.mma_tier_usage[tier], (
|
||||
f"precondition failed: tier {tier} has no 'model' key in __init__"
|
||||
)
|
||||
controller._handle_reset_session()
|
||||
for tier in ("Tier 1", "Tier 2", "Tier 3", "Tier 4"):
|
||||
assert "model" in controller.mma_tier_usage[tier], (
|
||||
f"_handle_reset_session stripped 'model' from {tier}: "
|
||||
f"{controller.mma_tier_usage[tier]!r}"
|
||||
)
|
||||
assert "provider" in controller.mma_tier_usage[tier], (
|
||||
f"_handle_reset_session stripped 'provider' from {tier}: "
|
||||
f"{controller.mma_tier_usage[tier]!r}"
|
||||
)
|
||||
controller._flush_to_project()
|
||||
assert Path(controller.active_project_path).exists()
|
||||
|
||||
|
||||
def test_flush_to_project_is_defensive_against_partial_tier_dict(controller: AppController) -> None:
|
||||
"""Bug 1 fix (defense in depth): _flush_to_project must not raise KeyError on partial dicts.
|
||||
|
||||
This is the defense-in-depth test for the d.get('model') change. Simulates
|
||||
a code path (like _handle_mma_state_update at line 484-497) that replaces
|
||||
the entire mma_tier_usage[tier] entry with a partial dict.
|
||||
"""
|
||||
controller.mma_tier_usage["Tier 3"] = {"input": 0, "output": 0, "provider": "gemini"}
|
||||
controller._flush_to_project()
|
||||
with open(controller.active_project_path, "rb") as f:
|
||||
saved = tomllib.load(f)
|
||||
tier_models = saved.get("mma", {}).get("tier_models", {})
|
||||
assert "Tier 3" in tier_models, f"Tier 3 missing from saved tier_models: {tier_models!r}"
|
||||
assert tier_models["Tier 3"].get("model") in (None, ""), (
|
||||
f"Expected None or empty model for the partial-dict case, got "
|
||||
f"{tier_models['Tier 3'].get('model')!r}"
|
||||
)
|
||||
|
||||
|
||||
def test_context_preset_manager_is_initialized(controller: AppController) -> None:
|
||||
"""Bug 2 fix: self.context_preset_manager must be a ContextPresetManager, not None.
|
||||
|
||||
Pre-fix: __init__ did not set self.context_preset_manager; save_context_preset
|
||||
and load_context_preset both crashed with AttributeError. Post-fix: __init__
|
||||
sets it to ContextPresetManager() (the line was lost in 72f8f466 and re-added).
|
||||
"""
|
||||
assert controller.context_preset_manager is not None, (
|
||||
f"context_preset_manager is None; the __init__ line is missing"
|
||||
)
|
||||
from src.context_presets import ContextPresetManager
|
||||
assert isinstance(controller.context_preset_manager, ContextPresetManager), (
|
||||
f"context_preset_manager is {type(controller.context_preset_manager).__name__}, "
|
||||
f"expected ContextPresetManager"
|
||||
)
|
||||
|
||||
|
||||
def test_hasattr_persona_manager_returns_false_for_fresh_controller() -> None:
|
||||
"""Bug 3 fix: hasattr(ctrl, 'persona_manager') must be False for a fresh AppController.
|
||||
|
||||
Pre-fix: __getattr__ returned None for 'persona_manager' (in _LAZY_MANAGER_DEFAULTS),
|
||||
making hasattr() return True. The comment claimed hasattr() returns False but
|
||||
that's wrong. Post-fix: 'persona_manager' is removed from _LAZY_MANAGER_DEFAULTS,
|
||||
so __getattr__ raises AttributeError, so hasattr() returns False.
|
||||
"""
|
||||
ctrl = AppController()
|
||||
assert not hasattr(ctrl, "persona_manager"), (
|
||||
f"hasattr(ctrl, 'persona_manager') returned True for a fresh AppController. "
|
||||
f"__getattr__ likely still returns None for it. Check _LAZY_MANAGER_DEFAULTS "
|
||||
f"in src/app_controller.py."
|
||||
)
|
||||
```
|
||||
|
||||
**CRITICAL — 1-space indent for all function bodies.** The file-level content has no indent. The `def` lines have no indent. The function body lines have exactly 1 space.
|
||||
|
||||
- [ ] **Step 5.1.2: Verify syntax**
|
||||
```powershell
|
||||
cd C:\projects\manual_slop; uv run python -c "import ast; ast.parse(open('tests/test_mma_tier_usage_reset_fix.py').read()); print('OK')"
|
||||
```
|
||||
|
||||
- [ ] **Step 5.1.3: Run the 4 new tests**
|
||||
```powershell
|
||||
cd C:\projects\manual_slop; uv run pytest tests/test_mma_tier_usage_reset_fix.py -v --timeout=30
|
||||
```
|
||||
Expected: 4/4 pass.
|
||||
|
||||
- [ ] **Step 5.1.4: Skip pre-fix verification**
|
||||
|
||||
**DO NOT** attempt to verify the tests would fail pre-fix. The user has explicitly banned all forms of pre-fix replay (no `git checkout`, no `git restore`, no `git reset`, no scratch reproduction scripts that simulate the pre-fix state). The 4 tests in this file are the unit-test equivalent of the integration tests that exposed the bugs; reasoning in their docstrings explains the pre-fix failure mode in prose as a substitute for replay.
|
||||
|
||||
If you want extra confidence the test design is correct, READ the test, READ the bug location (lines 3409, 1183, 1267 in the current HEAD), and PREDICT the failure mode from the code. Do not run it against pre-fix state.
|
||||
|
||||
### Task 5.2: Commit the regression tests
|
||||
|
||||
- [ ] **Step 5.2.1: Commit the regression tests**
|
||||
```powershell
|
||||
cd C:\projects\manual_slop; git add tests/test_mma_tier_usage_reset_fix.py
|
||||
git commit -m "test(reset): regression for 3 pre-existing controller bugs"
|
||||
$h = git log -1 --format='%H'
|
||||
git notes add -m "4 tests in tests/test_mma_tier_usage_reset_fix.py: (1) test_reset_session_makes_flush_to_project_not_crash verifies the post-reset flush path works end-to-end; (2) test_flush_to_project_is_defensive_against_partial_tier_dict verifies the .get('model') defense in depth; (3) test_context_preset_manager_is_initialized verifies the FR3 fix (the __init__ line was lost in 72f8f466); (4) test_hasattr_persona_manager_returns_false_for_fresh_controller verifies the FR4 fix (the _LAZY_MANAGER_DEFAULTS comment was wrong). All fail pre-fix and pass post-fix. Tests do not require live_gui fixture." $h
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Phase 6: Run the full batch and verify
|
||||
|
||||
Focus: The moment of truth. The 4 sim tests in `test_extended_sims.py` now pass, the 3 previously-failing tier-1 tests now pass, Tier-2 still passes, no new tier-3 failures.
|
||||
|
||||
### Task 6.1: Verify the existing 3 tests in `test_reset_session_clears_mma_and_rag.py` still pass
|
||||
|
||||
- [ ] **Step 6.1.1: Run the regression tests from `fe240db4`**
|
||||
```powershell
|
||||
cd C:\projects\manual_slop; uv run pytest tests/test_reset_session_clears_mma_and_rag.py -v --timeout=60
|
||||
```
|
||||
Expected: 3/3 pass (the `fe240db4` regressions are not broken by the new fix).
|
||||
|
||||
### Task 6.2: Run the 3 previously-failing tier-1 tests + 4 sim tests
|
||||
|
||||
- [ ] **Step 6.2.1: Run the 3 previously-failing tier-1 tests**
|
||||
```powershell
|
||||
cd C:\projects\manual_slop; uv run pytest tests/test_context_presets_manager.py::test_app_controller_save_load tests/test_project_switch_persona_preset.py::test_load_active_project_creates_persona_manager tests/test_project_switch_persona_preset.py::test_load_context_preset_missing_raises_keyerror -v --timeout=60
|
||||
```
|
||||
Expected: 3/3 pass.
|
||||
|
||||
- [ ] **Step 6.2.2: Run the 4 sim tests**
|
||||
```powershell
|
||||
cd C:\projects\manual_slop; uv run pytest tests/test_extended_sims.py -v --timeout=300
|
||||
```
|
||||
Expected: 4/4 pass. **CRITICAL: This must be in batch mode** (i.e. as part of a larger run, not isolation). If the test is run in isolation, it may pass even without the fix because the io_pool is empty. Verify the run is the FULL pytest invocation of `test_extended_sims.py` (all 4 tests share a live_gui subprocess).
|
||||
|
||||
### Task 6.3: Run the full batch
|
||||
|
||||
- [ ] **Step 6.3.1: Run the full batched test suite**
|
||||
```powershell
|
||||
cd C:\projects\manual_slop; uv run .\scripts\run_tests_batched.py 2>&1 | Tee-Object -FilePath "tests/artifacts/post_mma_reset_fix_batch_20260610.log" | Select-Object -Last 50
|
||||
```
|
||||
Expected:
|
||||
- tier-1: 5/5 batches pass
|
||||
- tier-2: 5/5 batches pass
|
||||
- tier-3: 0 NEW failures vs the `33d02bb1` baseline (i.e. the 4 sim tests now pass; the 3 `fe240db4` regression tests still pass)
|
||||
|
||||
- [ ] **Step 6.3.2: If tier-3 has new failures, STOP and report**
|
||||
**DO NOT** try to fix new failures in this track. This track's scope is the 4 FRs above. New failures are out of scope — document them in the git note and move on.
|
||||
|
||||
### Task 6.4: Checkpoint commit
|
||||
|
||||
- [ ] **Step 6.4.1: Create the checkpoint commit**
|
||||
```powershell
|
||||
cd C:\projects\manual_slop; git add tests/artifacts/post_mma_reset_fix_batch_20260610.log
|
||||
git commit -m "conductor(checkpoint): Checkpoint end of Phase 6 (4 FRs + 4 regression tests)"
|
||||
$h = git log -1 --format='%H'
|
||||
git notes add -m "Final batch run log. tier-1 5/5, tier-2 5/5, tier-3 [count] failures (should be 0 new vs 33d02bb1). The 4 sim tests in test_extended_sims.py now pass because FR1+FR2 fix the mma_tier_usage reset. The 3 previously-failing tier-1 tests now pass because FR3 re-adds the context_preset_manager init and FR4 removes persona_manager from _LAZY_MANAGER_DEFAULTS." $h
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Final Verification
|
||||
|
||||
- [ ] All 5 commits in place (FR1, FR2, FR3, FR4, regression tests, checkpoint)
|
||||
- [ ] `src/app_controller.py:3409` pre-populates `mma_tier_usage` with the full default shape
|
||||
- [ ] `src/app_controller.py:2639` uses `d.get("model")` instead of `d["model"]`
|
||||
- [ ] `src/app_controller.py:__init__` contains `self.context_preset_manager = ContextPresetManager()`
|
||||
- [ ] `src/app_controller.py:1266-1275` does NOT contain `"persona_manager"` in `_LAZY_MANAGER_DEFAULTS`
|
||||
- [ ] 4 new regression tests in `tests/test_mma_tier_usage_reset_fix.py` pass
|
||||
- [ ] 3 existing tests in `tests/test_reset_session_clears_mma_and_rag.py` still pass
|
||||
- [ ] 3 previously-failing tier-1 tests now pass:
|
||||
- `tests/test_context_presets_manager.py::test_app_controller_save_load`
|
||||
- `tests/test_project_switch_persona_preset.py::test_load_active_project_creates_persona_manager`
|
||||
- `tests/test_project_switch_persona_preset.py::test_load_context_preset_missing_raises_keyerror`
|
||||
- [ ] 4 sim tests in `tests/test_extended_sims.py` pass in batch
|
||||
- [ ] Tier-1 batch: 5/5 pass
|
||||
- [ ] Tier-2 batch: 5/5 pass
|
||||
- [ ] Tier-3 batch: 0 new failures
|
||||
|
||||
## Track Done
|
||||
|
||||
After the 6 commits (FR1, FR2, FR3, FR4, regression tests, checkpoint) and the full batch verification, the track is DONE. **Do not:**
|
||||
- File follow-up tracks
|
||||
- Add scope
|
||||
- Refactor anything else
|
||||
- Update docs
|
||||
- Add more tests
|
||||
|
||||
**Do:**
|
||||
- Report the final state to the user
|
||||
- Mark the track as complete in `conductor/tracks.md`
|
||||
- Move on to whatever's next
|
||||
|
||||
---
|
||||
|
||||
## Execution Constraints
|
||||
|
||||
- **1-space indent, CRLF, type hints.** Per project conventions.
|
||||
- **1-line edits via `manual-slop_set_file_slice`.** Per `conductor/edit_workflow.md`.
|
||||
- **Verify syntax with `ast.parse` after each edit.**
|
||||
- **No diagnostic noise in production.** No `print()` statements added to `src/app_controller.py` for debugging.
|
||||
- **Per-task atomic commits.** Not batched.
|
||||
- **No "while we're at it" refactors.** This is a 4-line bug fix (2 surgical FRs on `_handle_reset_session`/`_flush_to_project`, 1 line in `__init__`, 1 line removal from `_LAZY_MANAGER_DEFAULTS`). Stay in scope.
|
||||
@@ -0,0 +1,261 @@
|
||||
# Track Specification: Fix `mma_tier_usage` reset breaking `_flush_to_project` + 2 pre-existing bugs (2026-06-10)
|
||||
|
||||
## Overview
|
||||
|
||||
This track fixes **3 distinct pre-existing bugs** in `src/app_controller.py` that surfaced during the 2026-06-10 batch run:
|
||||
|
||||
1. **`mma_tier_usage` reset to empty dicts** (introduced in `fe240db4` 2026-06-09). `_handle_reset_session` zeroes the per-tier dicts to `{}`, but `_flush_to_project` does `d["model"]` and crashes with `KeyError`. This crashes the project save AND triggers an infinite re-switch loop in `_do_project_switch`'s finally block. Symptom: `test_context_sim_live` sees `ai_status = "error: 'model'"` (or "switching to: ... (stale ui - ops disabled)" in older runs) and times out at 60s.
|
||||
|
||||
2. **`self.context_preset_manager` is never initialized in `__init__`** (accidentally lost in `72f8f466` 2026-06-10). The line `self.context_preset_manager = ContextPresetManager()` was in the codebase at `c039fdbb` (2026-06-09) and got dropped when `_settable_fields` block was hand-edited. `save_context_preset` and `load_context_preset` both dereference `self.context_preset_manager.save_preset(...)` and `self.context_preset_manager.load_all(...)` — both crash with `AttributeError: 'NoneType' object has no attribute 'save_preset'` (or `'load_all'`). Symptom: `tests/test_context_presets_manager.py::test_app_controller_save_load` and `tests/test_project_switch_persona_preset.py::test_load_context_preset_missing_raises_keyerror` fail in tier-1.
|
||||
|
||||
3. **`__getattr__` short-circuits manager attributes to None, breaking `hasattr()`** (added 2026-06-08 in `c039fdbb`'s neighborhood). The `_LAZY_MANAGER_DEFAULTS` set in `AppController.__getattr__` (src/app_controller.py:1266-1275) returns `None` for `context_preset_manager`, `persona_manager`, `tool_preset_manager`, `preset_manager`, `vendor_state`, `perf_monitor`. The code comment claims "hasattr() still returns False for non-mocked access paths" but this is wrong — `__getattr__` returning None makes `hasattr()` return True. Symptom: `tests/test_project_switch_persona_preset.py::test_load_active_project_creates_persona_manager` fails because it asserts `not hasattr(ctrl, "persona_manager")` for a fresh `AppController()`, but `__getattr__` returns None so `hasattr()` returns True.
|
||||
|
||||
The mma_tier_usage fix was the original ask. The 2 additional bugs surfaced when the user ran the full batch to verify the original fix. Including all 3 in this track is in-scope: they are all in the same file (`src/app_controller.py`), all pre-existing (not introduced by my changes), all block the test suite from going green, and all are 1-3 line surgical fixes.
|
||||
|
||||
## Bug 1 in detail: `mma_tier_usage` reset
|
||||
|
||||
`_handle_reset_session` (src/app_controller.py:3358) was changed in commit `fe240db4` to reset `mma_tier_usage` to `{'Tier 1': {}, 'Tier 2': {}, 'Tier 3': {}, 'Tier 4': {}}` — empty dicts. The downstream consumer `_flush_to_project` (line 2639) does `d["model"]` and crashes with `KeyError: 'model'` when iterating over the per-tier dicts.
|
||||
|
||||
This is the root cause of `test_context_sim_live` (and the 3 sibling sims) failing. The test sees the `ai_status` of `"error: 'model'"` (after the sim_context.py polling loop added an `"error" in s` check) because:
|
||||
|
||||
1. The test clicks `btn_reset` → `_handle_reset_session` zeroes `mma_tier_usage` to empty dicts.
|
||||
2. The test clicks `btn_project_new_automated` → `_switch_project(path)` is called → sets `in_progress=True`, submits `_do_project_switch` to the io_pool, sets `ai_status = "switching to: ... (stale ui - ops disabled)"`.
|
||||
3. The test clicks `btn_project_save` → `_cb_project_save` calls `_flush_to_project()` on the main render thread → CRASHES with `KeyError: 'model'`. The exception is silently swallowed by `_process_pending_gui_tasks`'s try/except.
|
||||
4. **Concurrently** on the io_pool: `_do_project_switch` runs → calls `self._flush_to_project()` FIRST → CRASHES with the same `KeyError` → `finally` block runs → `in_progress=False` → `pending == active_project_path` is false (we never got to update `active_project_path`) → `_switch_project(pending)` is called recursively → resubmits → `in_progress=True` again → `_do_project_switch` crashes again → infinite re-switch loop.
|
||||
5. After 60+ seconds of the re-switch loop, eventually some other worker call reaches `_handle_md_only` (the test's actual target). It crashes the same way, but the `except Exception as e: self.ai_status = f"error: {e}"` in `_handle_md_only`'s worker (line 3560) catches it and sets `ai_status = "error: 'model'"`.
|
||||
6. Test polls `ai_status` and sees `"error: 'model'"`. The `"error" in s` branch in the sim polling loop (added to `sim_context.py` in the working tree) breaks early. The assertion fails with the message: `Expected 'md written' in status, got error: 'model'`.
|
||||
|
||||
The fix restores the pre-`fe240db4` behavior of `_handle_reset_session`: pre-populate `mma_tier_usage` with the full default values (input, output, provider, model, tool_preset) so that downstream consumers like `_flush_to_project` don't crash on missing keys.
|
||||
|
||||
The 3 regression tests in `tests/test_reset_session_clears_mma_and_rag.py` (added in the same `fe240db4` commit) check that the polluted `'model' = 'polluted'` value is cleared. They pass with the pre-populated defaults because `'gemini-3.1-pro-preview' != 'polluted'`. The goal of "no stale pollution" is preserved.
|
||||
|
||||
## Bug 2 in detail: missing `context_preset_manager` init
|
||||
|
||||
`git show c039fdbb:src/app_controller.py` shows the line was present at that commit:
|
||||
```python
|
||||
self.context_preset_manager = ContextPresetManager()
|
||||
```
|
||||
right after the `_settable_fields` block and before `self.perf_monitor = ...`. `git show HEAD:src/app_controller.py` (after `72f8f466`) shows the line is gone. The diff between `c039fdbb` and `72f8f466` confirms it was the one line dropped:
|
||||
```
|
||||
-self.context_preset_manager = ContextPresetManager()
|
||||
```
|
||||
during a hand-edited refactor of the `_settable_fields` block.
|
||||
|
||||
The fix is to re-add the line at the same position in `__init__`.
|
||||
|
||||
## Bug 3 in detail: `__getattr__` returns None for manager attrs
|
||||
|
||||
The `__getattr__` at src/app_controller.py:1226-1281 has a `_LAZY_MANAGER_DEFAULTS` set (lines 1266-1275) that includes `persona_manager`, `context_preset_manager`, `tool_preset_manager`, `preset_manager`, `vendor_state`, `perf_monitor`. When the controller is constructed without calling `init_state()` (some tests do this), accessing these attributes goes through `__getattr__` which returns `None`.
|
||||
|
||||
The comment on the set says:
|
||||
> "hasattr() still returns False for non-mocked access paths because callers wrap in try/except for AttributeError when they need to distinguish 'lazy' from 'absent'."
|
||||
|
||||
This is **wrong**. `__getattr__` returning `None` makes `hasattr(obj, name)` return `True` (because `None` is a valid attribute value). The test `test_load_active_project_creates_persona_manager` is written correctly per Python semantics — it asserts that before `_load_active_project()` is called, the controller should not have `persona_manager`. But because `__getattr__` returns `None`, `hasattr(ctrl, "persona_manager")` is `True`, and the assertion fails.
|
||||
|
||||
The fix: remove `persona_manager` (and the other lazily-managed attrs) from `_LAZY_MANAGER_DEFAULTS`, so `__getattr__` raises `AttributeError` for them. Callers that want the lazy default can use `getattr(ctrl, "persona_manager", None)`. The comment should also be removed or updated to reflect the actual Python semantics.
|
||||
|
||||
`context_preset_manager` is also in this set, so removing it from `_LAZY_MANAGER_DEFAULTS` is necessary regardless (Bug 2's fix re-adds the init, so the lazy fallback is no longer needed for that one). For the other 5 names (`persona_manager`, `tool_preset_manager`, `preset_manager`, `vendor_state`, `perf_monitor`), the lazy fallback may or may not be load-bearing for other tests. The conservative fix is to remove `persona_manager` specifically (the one the test asserts on) and verify the other 5 don't have callers relying on the lazy default.
|
||||
|
||||
Actually, looking at the test that's failing more carefully:
|
||||
- `test_load_active_project_creates_persona_manager` only asserts `not hasattr(ctrl, "persona_manager")` BEFORE `_load_active_project()`.
|
||||
- The test in the same file `test_switch_project_preserves_global_preset` (line 150) explicitly sets `ctrl.persona_manager = PersonaManager(...)` BEFORE calling `_refresh_from_project()`. This works fine because `setattr` doesn't go through `__getattr__`.
|
||||
- The test in the same file `test_load_context_preset_missing_raises_keyerror` (line 181) doesn't touch `persona_manager`.
|
||||
|
||||
The minimal fix is to remove `persona_manager` from `_LAZY_MANAGER_DEFAULTS`. The other 5 names can stay (they have similar semantics; whether other tests depend on the lazy default needs to be verified in the batch run). The track will verify no regressions in the batch.
|
||||
|
||||
## Current State Audit (as of `33d02bb1`)
|
||||
|
||||
### Already Implemented (DO NOT re-implement)
|
||||
|
||||
- `_handle_reset_session` (src/app_controller.py:3358) clears project state, MMA state, RAG state. Pre-populated `mma_tier_usage` defaults in `__init__` (line 952-957). 3 regression tests in `tests/test_reset_session_clears_mma_and_rag.py` verify the polluted state is cleared.
|
||||
- `simulation/sim_base.py` `setup()` (line 78-99) waits for the project switch to complete via `wait_for_project_switch(expected_path=..., timeout=30.0)`.
|
||||
- `simulation/sim_context.py` `run()` (line 17-30) waits for the project switch to complete again with `wait_for_project_switch(timeout=15.0)` before clicking `btn_md_only`. The polling loop also breaks early on `"error" in status` to surface terminal errors.
|
||||
- `src/api_hooks.py` exposes `/api/project_switch_status` (line 2493) and `/api/gui/state` (line 309). The latter is the fallback used by `get_project_switch_status` in `api_hook_client.py:362-384` when the dedicated endpoint is missing.
|
||||
- `src/app_controller.py:_switch_project` (line 2830) is non-blocking; submits `_do_project_switch` to `submit_io` (line 2303 → `_io_pool`).
|
||||
- `src/app_controller.py:_do_project_switch` (line 2789) is the async worker. Its `try`/`finally` structure (line 2792-2822) sets `in_progress = False` in the `finally` and recursively re-queues via `_switch_project(pending)` if `pending != active_project_path`. The recursion is the infinite loop when the worker fails before setting `active_project_path`.
|
||||
|
||||
### Bugs
|
||||
|
||||
**Bug 1: Empty `mma_tier_usage` reset.** `src/app_controller.py:3409` (introduced in commit `fe240db4`):
|
||||
|
||||
```python
|
||||
# Reset mma_tier_usage to pre-populated default (prior tests pollute it)
|
||||
self.mma_tier_usage = {'Tier 1': {}, 'Tier 2': {}, 'Tier 3': {}, 'Tier 4': {}}
|
||||
```
|
||||
|
||||
Comment says "pre-populated default" but the dicts are empty. `_flush_to_project` (line 2639) does:
|
||||
|
||||
```python
|
||||
mma_sec["tier_models"] = {t: {"model": d["model"], "provider": d.get("provider", "gemini"), "tool_preset": d.get("tool_preset")} for t, d in self.mma_tier_usage.items()}
|
||||
```
|
||||
|
||||
`d["model"]` raises `KeyError` when `d = {}`.
|
||||
|
||||
**Bug 2: Missing `context_preset_manager` init.** `src/app_controller.py:__init__` does not set `self.context_preset_manager`. The line `self.context_preset_manager = ContextPresetManager()` was in the codebase at commit `c039fdbb` (2026-06-09) but was dropped during a hand-edited refactor in `72f8f466` (2026-06-10). `save_context_preset` and `load_context_preset` both dereference `self.context_preset_manager` which is `None` (via `__getattr__`'s `_LAZY_MANAGER_DEFAULTS` short-circuit, see Bug 3) — both crash with `AttributeError`.
|
||||
|
||||
**Bug 3: `__getattr__` short-circuit breaks `hasattr()`.** `src/app_controller.py:1266-1281` has:
|
||||
|
||||
```python
|
||||
_LAZY_MANAGER_DEFAULTS = {
|
||||
"context_preset_manager",
|
||||
"persona_manager",
|
||||
"tool_preset_manager",
|
||||
"preset_manager",
|
||||
"vendor_state",
|
||||
"perf_monitor",
|
||||
}
|
||||
if name in _LAZY_MANAGER_DEFAULTS:
|
||||
return None
|
||||
```
|
||||
|
||||
The accompanying comment claims `hasattr()` still returns False for these, which is **wrong** — `__getattr__` returning `None` makes `hasattr()` return `True`. Test `test_load_active_project_creates_persona_manager` asserts `not hasattr(ctrl, "persona_manager")` for a fresh controller and fails.
|
||||
|
||||
### Gaps to Fill (This Track's Scope)
|
||||
|
||||
- **Gap 1 (Bug 1): `_handle_reset_session` should pre-populate `mma_tier_usage` with the full default shape** (matching `__init__` at line 952-957), not empty dicts. This restores the pre-`fe240db4` contract that downstream consumers rely on.
|
||||
- **Gap 2 (Bug 1): `_flush_to_project` should be defensive** against missing `model` keys (use `.get("model", default)` instead of `["model"]`). Other code paths can produce partial `mma_tier_usage` entries (e.g. `_handle_mma_state_update` at line 484-497 does `controller.mma_tier_usage[tier] = data` with whatever data the caller sends). Defense in depth.
|
||||
- **Gap 3 (Bug 2): Re-add `self.context_preset_manager = ContextPresetManager()` in `__init__`** at the original position (after the `_settable_fields` block, before `self.perf_monitor = ...`).
|
||||
- **Gap 4 (Bug 3): Remove `persona_manager` from `_LAZY_MANAGER_DEFAULTS`** in `__getattr__`. The other 5 names stay (they may have lazy-default callers; verify in batch). Also fix or remove the misleading comment.
|
||||
|
||||
## Goals
|
||||
|
||||
1. **Goal A: `test_context_sim_live` passes in batch.** The sim tests in `tests/test_extended_sims.py` (4 of them) all pass. Specifically the test that was failing with `assert "md written" in status, f"Expected 'md written' in status, got {status}"` no longer times out.
|
||||
2. **Goal B: The 3 regression tests in `tests/test_reset_session_clears_mma_and_rag.py` still pass.** They check that polluted `tier_usage` data is cleared; pre-populated defaults are not pollution.
|
||||
3. **Goal C: `test_app_controller_save_load` passes.** Tier-1 test in `tests/test_context_presets_manager.py` that calls `controller.save_context_preset(preset)` and expects no crash.
|
||||
4. **Goal D: `test_load_context_preset_missing_raises_keyerror` passes.** Tier-1 test in `tests/test_project_switch_persona_preset.py` that calls `controller.load_context_preset("NonexistentPreset")` and expects `KeyError` (which requires `self.context_preset_manager.load_all` to be callable).
|
||||
5. **Goal E: `test_load_active_project_creates_persona_manager` passes.** Tier-1 test that asserts `not hasattr(ctrl, "persona_manager")` for a fresh controller.
|
||||
6. **Goal F: No new failures in tier-1, tier-2, or tier-3 batches.** Match the `33d02bb1` baseline or improve on it.
|
||||
|
||||
### Non-Goals
|
||||
|
||||
- Refactoring `_switch_project` or `_do_project_switch` to use a state machine.
|
||||
- Removing the `try/finally` recursive re-switch in `_do_project_switch` (that's a separate architectural concern; the contract is "if a switch fails, re-queue it", which is a valid design).
|
||||
- Modifying the 3 regression tests in `tests/test_reset_session_clears_mma_and_rag.py`.
|
||||
- Modifying `tests/test_context_presets_manager.py::test_app_controller_save_load`, `tests/test_project_switch_persona_preset.py::test_load_active_project_creates_persona_manager`, or `tests/test_project_switch_persona_preset.py::test_load_context_preset_missing_raises_keyerror` (the test code is correct; the production code is wrong).
|
||||
- Modifying `simulation/sim_base.py` or `simulation/sim_context.py`.
|
||||
- Adding new audit scripts.
|
||||
- Updating `docs/`.
|
||||
- Filing follow-up tracks.
|
||||
- Any "while we're at it" refactors.
|
||||
|
||||
## Functional Requirements
|
||||
|
||||
### FR1. Pre-populate `mma_tier_usage` on reset
|
||||
|
||||
**Where:** `src/app_controller.py:3409`
|
||||
|
||||
**What:** Replace the empty-dict reset with the full pre-populated default (matching the shape in `__init__` at line 952-957). The full shape is:
|
||||
```python
|
||||
{
|
||||
"Tier 1": {"input": 0, "output": 0, "provider": "gemini", "model": "gemini-3.1-pro-preview", "tool_preset": None},
|
||||
"Tier 2": {"input": 0, "output": 0, "provider": "gemini", "model": "gemini-3-flash-preview", "tool_preset": None},
|
||||
"Tier 3": {"input": 0, "output": 0, "provider": "gemini", "model": "gemini-2.5-flash-lite", "tool_preset": None},
|
||||
"Tier 4": {"input": 0, "output": 0, "provider": "gemini", "model": "gemini-2.5-flash-lite", "tool_preset": None},
|
||||
}
|
||||
```
|
||||
|
||||
**Why this shape:** It's the same shape `__init__` uses (line 952-957), so the controller's `mma_tier_usage` invariant is preserved across the reset boundary.
|
||||
|
||||
**Acceptance:**
|
||||
- `tests/test_reset_session_clears_mma_and_rag.py::test_reset_session_clears_mma_tier_usage` still passes (the assertion `tier1.get('model') != 'polluted'` holds because `'gemini-3.1-pro-preview' != 'polluted'`).
|
||||
- `tests/test_reset_session_clears_mma_and_rag.py::test_reset_session_clears_mma_status` still passes (untouched by the change).
|
||||
- `tests/test_reset_session_clears_mma_and_rag.py::test_reset_session_clears_active_tier` still passes (untouched by the change).
|
||||
- `tests/test_extended_sims.py::test_context_sim_live` passes.
|
||||
- `tests/test_extended_sims.py::test_ai_settings_sim_live`, `test_tools_sim_live`, `test_execution_sim_live` pass.
|
||||
|
||||
### FR2. Make `_flush_to_project` defensive against missing `model`
|
||||
|
||||
**Where:** `src/app_controller.py:2639`
|
||||
|
||||
**What:** Change `d["model"]` to `d.get("model")` (or `d.get("model", "")`). The rest of the dict comprehension already uses `.get()` for `provider` and `tool_preset`; `model` is the only one that does a hard `[]` lookup.
|
||||
|
||||
**Why:** Defense in depth. Other code paths can produce partial `mma_tier_usage[tier]` dicts (e.g. `_handle_mma_state_update` at line 484-497 replaces the entry with whatever the caller sends). Even with FR1, future regressions that produce empty/partial dicts will not crash the project save.
|
||||
|
||||
**Acceptance:**
|
||||
- `mma_sec["tier_models"]` is written successfully even if some tier's `mma_tier_usage[tier]` is missing the `model` key. The resulting TOML field would be `model = ""` (or the default value), not a crash.
|
||||
- No existing tests break.
|
||||
|
||||
### FR3. Re-add `self.context_preset_manager = ContextPresetManager()` to `__init__`
|
||||
|
||||
**Where:** `src/app_controller.py:__init__` — between line 1183 (end of `_settable_fields` block) and line 1185 (`self.perf_monitor = ...`)
|
||||
|
||||
**What:** Insert the line `self.context_preset_manager = ContextPresetManager()` at the same position it occupied in commit `c039fdbb` (immediately before `self.perf_monitor = performance_monitor.get_monitor()`).
|
||||
|
||||
**Why:** `save_context_preset` (line 3019) and `load_context_preset` (line 3023) both dereference `self.context_preset_manager`. The init line was lost in `72f8f466`. Without it, both methods crash with `AttributeError: 'NoneType' object has no attribute 'save_preset'`.
|
||||
|
||||
**Acceptance:**
|
||||
- `tests/test_context_presets_manager.py::test_app_controller_save_load` passes (it calls `controller.save_context_preset(preset)` and asserts the project is updated).
|
||||
- `tests/test_project_switch_persona_preset.py::test_load_context_preset_missing_raises_keyerror` passes (it calls `controller.load_context_preset("NonexistentPreset")` and expects `KeyError`; the KeyError can only be raised if `self.context_preset_manager.load_all(self.project)` is callable).
|
||||
- No existing tests break.
|
||||
|
||||
### FR4. Remove `persona_manager` from `_LAZY_MANAGER_DEFAULTS` in `__getattr__`
|
||||
|
||||
**Where:** `src/app_controller.py:1266-1275` (the `_LAZY_MANAGER_DEFAULTS` set)
|
||||
|
||||
**What:** Remove the string `"persona_manager"` from the set. The other 5 names stay (verify in batch). Also fix or remove the misleading comment that says "hasattr() still returns False for non-mocked access paths because callers wrap in try/except for AttributeError when they need to distinguish 'lazy' from 'absent'" — this is incorrect.
|
||||
|
||||
**Why:** `__getattr__` returning `None` makes `hasattr()` return `True`. The test `test_load_active_project_creates_persona_manager` asserts `not hasattr(ctrl, "persona_manager")` for a fresh controller, which is the correct Python-semantics check. The comment justifying the lazy default is wrong.
|
||||
|
||||
**Acceptance:**
|
||||
- `tests/test_project_switch_persona_preset.py::test_load_active_project_creates_persona_manager` passes (the assertion `not hasattr(ctrl, "persona_manager")` holds for a fresh controller).
|
||||
- After `_load_active_project()` is called, `hasattr(ctrl, "persona_manager")` is True and `ctrl.persona_manager` is a `PersonaManager` instance.
|
||||
- No existing tests break. (The 5 other names in `_LAZY_MANAGER_DEFAULTS` may have lazy-default callers — verify in the batch run.)
|
||||
|
||||
## Non-Functional Requirements
|
||||
|
||||
- **NFR1: 1 import, no new functions, ~10 line changes total.** Surgical. Two file edits in `src/app_controller.py`.
|
||||
- **NFR2: No regressions.** Tier-1 and tier-2 batch results must match the `33d02bb1` baseline.
|
||||
- **NFR3: 2 atomic commits.** One per FR. Not batched.
|
||||
- **NFR4: 1-space indent, CRLF, type hints.** Per project conventions.
|
||||
- **NFR5: 1 regression test added.** A unit test that proves `KeyError: 'model'` no longer occurs in the post-reset flush path. The test must NOT be a copy of the existing 3 tests in `tests/test_reset_session_clears_mma_and_rag.py`; it must be a NEW test that exercises the specific code path that was crashing.
|
||||
|
||||
## Architecture Reference
|
||||
|
||||
- **`src/app_controller.py:952-957`** — `mma_tier_usage` default shape in `__init__`. This is the shape FR1 must match.
|
||||
- **`src/app_controller.py:1183-1185`** — `__init__` end of `_settable_fields` block and start of `self.perf_monitor = ...`. FR3 inserts the missing `context_preset_manager` init between these.
|
||||
- **`src/app_controller.py:1266-1281`** — `_LAZY_MANAGER_DEFAULTS` set and its consumer in `__getattr__`. FR4.
|
||||
- **`src/app_controller.py:2639`** — `_flush_to_project` line that crashes. FR2.
|
||||
- **`src/app_controller.py:3019-3023`** — `save_context_preset` and `load_context_preset`. FR3 ensures these have a non-None `context_preset_manager` to dereference.
|
||||
- **`src/app_controller.py:3358-3409`** — `_handle_reset_session`. FR1.
|
||||
- **`src/app_controller.py:2789-2822`** — `_do_project_switch`. NOT changed in this track; the recursive re-switch is a valid design; the bug is the upstream `_flush_to_project` crash, not the re-switch.
|
||||
- **`src/app_controller.py:2830-2848`** — `_switch_project`. NOT changed.
|
||||
- **`tests/test_reset_session_clears_mma_and_rag.py`** — 3 regression tests from `fe240db4`. Must continue to pass.
|
||||
- **`tests/test_extended_sims.py`** — 4 sim tests that have been failing. FR1+FR2 unblock them.
|
||||
- **`tests/test_context_presets_manager.py::test_app_controller_save_load`** — tier-1 test that fails due to Bug 2. FR3 unblocks it.
|
||||
- **`tests/test_project_switch_persona_preset.py::test_load_active_project_creates_persona_manager`** — tier-1 test that fails due to Bug 3. FR4 unblocks it.
|
||||
- **`tests/test_project_switch_persona_preset.py::test_load_context_preset_missing_raises_keyerror`** — tier-1 test that fails due to Bug 2. FR3 unblocks it.
|
||||
|
||||
## Out of Scope
|
||||
|
||||
- Refactoring `_switch_project` to use a state machine
|
||||
- Removing the recursive re-switch in `_do_project_switch`'s `finally`
|
||||
- Modifying the 3 tests in `tests/test_reset_session_clears_mma_and_rag.py`
|
||||
- Modifying `tests/test_context_presets_manager.py::test_app_controller_save_load`
|
||||
- Modifying `tests/test_project_switch_persona_preset.py::test_load_active_project_creates_persona_manager`
|
||||
- Modifying `tests/test_project_switch_persona_preset.py::test_load_context_preset_missing_raises_keyerror`
|
||||
- Refactoring `simulation/sim_base.py` or `simulation/sim_context.py`
|
||||
- Removing the other 5 names (`context_preset_manager`, `tool_preset_manager`, `preset_manager`, `vendor_state`, `perf_monitor`) from `_LAZY_MANAGER_DEFAULTS` — only `persona_manager` is removed in FR4. Verify the others in the batch; if any of them break, file a follow-up.
|
||||
- Adding new audit scripts
|
||||
- Doc updates
|
||||
- Follow-up tracks
|
||||
- Any "while we're at it" refactors
|
||||
|
||||
## Verification Criteria
|
||||
|
||||
1. ✅ `src/app_controller.py:3409` pre-populates `mma_tier_usage` with the full default shape (model, provider, tool_preset, input, output for all 4 tiers).
|
||||
2. ✅ `src/app_controller.py:2639` uses `d.get("model")` (or equivalent) instead of `d["model"]`.
|
||||
3. ✅ `src/app_controller.py:__init__` contains `self.context_preset_manager = ContextPresetManager()` between the `_settable_fields` block and `self.perf_monitor = ...`.
|
||||
4. ✅ `src/app_controller.py:1266-1275` does NOT contain `"persona_manager"` in `_LAZY_MANAGER_DEFAULTS`. The misleading comment is fixed or removed.
|
||||
5. ✅ A new unit test in `tests/test_mma_tier_usage_reset_fix.py` verifies the post-reset flush doesn't crash.
|
||||
6. ✅ `tests/test_reset_session_clears_mma_and_rag.py` (3 tests) still pass.
|
||||
7. ✅ `tests/test_extended_sims.py::test_context_sim_live` passes in batch.
|
||||
8. ✅ `tests/test_extended_sims.py::test_ai_settings_sim_live` passes in batch.
|
||||
9. ✅ `tests/test_extended_sims.py::test_tools_sim_live` passes in batch.
|
||||
10. ✅ `tests/test_extended_sims.py::test_execution_sim_live` passes in batch.
|
||||
11. ✅ `tests/test_context_presets_manager.py::test_app_controller_save_load` passes.
|
||||
12. ✅ `tests/test_project_switch_persona_preset.py::test_load_active_project_creates_persona_manager` passes.
|
||||
13. ✅ `tests/test_project_switch_persona_preset.py::test_load_context_preset_missing_raises_keyerror` passes.
|
||||
14. ✅ Tier-1 batch: 5/5 pass.
|
||||
15. ✅ Tier-2 batch: 5/5 pass.
|
||||
16. ✅ Tier-3 batch: 0 new failures vs `33d02bb1` baseline.
|
||||
17. ✅ 4 atomic commits (one per FR).
|
||||
@@ -0,0 +1,47 @@
|
||||
# Track state for mma_tier_usage_reset_fix_20260610
|
||||
# Updated by executing agent as tasks complete
|
||||
|
||||
[meta]
|
||||
track_id = "mma_tier_usage_reset_fix_20260610"
|
||||
name = "Fix mma_tier_usage reset + 2 pre-existing controller bugs (2026-06-10)"
|
||||
status = "active"
|
||||
current_phase = 0
|
||||
last_updated = "2026-06-10"
|
||||
|
||||
[blocked_by]
|
||||
# No blockers.
|
||||
|
||||
[blocks]
|
||||
# This track blocks nothing.
|
||||
|
||||
[phases]
|
||||
phase_1 = { status = "pending", checkpointsha = "", name = "Apply 4-line fix in app_controller.py and add 2 regression tests" }
|
||||
|
||||
[tasks]
|
||||
t1_1 = { status = "pending", commit_sha = "", description = "Pre-edit checkpoint" }
|
||||
t1_2 = { status = "pending", commit_sha = "", description = "FR1: Pre-populate mma_tier_usage in _handle_reset_session" }
|
||||
t1_3 = { status = "pending", commit_sha = "", description = "FR2: Make _flush_to_project defensive against missing model key" }
|
||||
t1_4 = { status = "pending", commit_sha = "", description = "FR3: Re-add self.context_preset_manager = ContextPresetManager() in __init__" }
|
||||
t1_5 = { status = "pending", commit_sha = "", description = "FR4: Remove 'persona_manager' from _LAZY_MANAGER_DEFAULTS in __getattr__" }
|
||||
t1_6 = { status = "pending", commit_sha = "", description = "Add 2 regression tests in tests/test_mma_tier_usage_reset_fix.py" }
|
||||
t1_7 = { status = "pending", commit_sha = "", description = "Verify the existing 3 tests in test_reset_session_clears_mma_and_rag.py still pass" }
|
||||
t1_8 = { status = "pending", commit_sha = "", description = "Run the 3 previously-failing tier-1 tests + 4 sim tests in test_extended_sims.py" }
|
||||
t1_9 = { status = "pending", commit_sha = "", description = "Run the full batch and verify no new failures" }
|
||||
t1_10 = { status = "pending", commit_sha = "", description = "Checkpoint commit" }
|
||||
|
||||
[verification]
|
||||
mma_tier_usage_prepopulated = false
|
||||
flush_to_project_defensive = false
|
||||
context_preset_manager_init_restored = false
|
||||
persona_manager_removed_from_lazy_defaults = false
|
||||
regression_tests_pass = false
|
||||
reset_clears_mma_tests_pass = false
|
||||
three_failing_tier1_tests_pass = false
|
||||
extended_sims_pass = false
|
||||
full_batch_passes = false
|
||||
|
||||
[baseline_capture]
|
||||
# Captured from the 2026-06-10 batch run
|
||||
tier_1_status = "FAIL (3 tests: test_app_controller_save_load, test_load_active_project_creates_persona_manager, test_load_context_preset_missing_raises_keyerror)"
|
||||
tier_2_status = "PASS (5/5 batches)"
|
||||
tier_3_status = "FAIL on test_extended_sims.py::test_context_sim_live (4 sim tests)"
|
||||
Reference in New Issue
Block a user