From 0d6dd8dbaba146759e3f17b1532cb9c6a0638ad0 Mon Sep 17 00:00:00 2001 From: Ed_ Date: Fri, 5 Jun 2026 19:05:35 -0400 Subject: [PATCH] docs(spec): design for live-gui fragility fixes (272-file suite: 269/272 -> 272/272) --- ...6-06-05-live-gui-fragility-fixes-design.md | 236 ++++++++++++++++++ 1 file changed, 236 insertions(+) create mode 100644 docs/superpowers/specs/2026-06-05-live-gui-fragility-fixes-design.md diff --git a/docs/superpowers/specs/2026-06-05-live-gui-fragility-fixes-design.md b/docs/superpowers/specs/2026-06-05-live-gui-fragility-fixes-design.md new file mode 100644 index 00000000..b74a9b59 --- /dev/null +++ b/docs/superpowers/specs/2026-06-05-live-gui-fragility-fixes-design.md @@ -0,0 +1,236 @@ +# Live-GUI Fragility Fixes — Design + +**Date:** 2026-06-05 +**Status:** Draft +**Track follow-up to:** regression_fixes_20260605 +**Scope:** Fix 3 failing live_gui tests discovered in the 2026-06-05 batched test run, harden the defer-not-catch pattern doc, restore 100% pass rate on the 272-file test suite. + +## 1. Background + +### Test run results (2026-06-05, batched via `scripts/run_tests_batched.py`) + +- **272 test files, 68 batches, 269/272 passing (98.9%).** +- 3 failing tests, all in `live_gui` (session-scoped fixture) or `integration` marker category. +- 0 failing tests in any other category (unit, headless, mock_app, simulation). + +### Failures (3) + +| Test | File | Symptom | Root cause | +|---|---|---|---| +| `test_auto_switch_sim` | `tests/test_auto_switch_sim.py:47` | `assert False == True` after triggering tier-3 auto-switch | Category A: profile save raises TypeError → no profile saved → load is no-op | +| `test_workspace_profiles_restoration` | `tests/test_workspace_profiles_sim.py:81` | `assert False is True` after `load_workspace_profile` | Category A: same as above | +| `test_no_extraneous_pop_when_prior_session_renders` | `tests/test_prior_session_no_pop_imbalance.py:135` | `TypeError: cannot unpack non-iterable NoneType object` at `src/gui_2.py:2333` | Category B: test mock setup for `imscope.window` returns non-iterable, but production code expects `(opened, visible)` tuple | + +### Root cause analysis (Category A — both profile failures) + +A regression introduced by commit `d7487af4` ("fix(gui_2): defer save_ini_settings on first capture to avoid early-render crash"). That commit added a defer-not-catch guard in `_capture_workspace_profile` (`src/gui_2.py:601-606`): + +```python +def _capture_workspace_profile(self, name: str) -> models.WorkspaceProfile: + if not getattr(self, "_ini_capture_ready", False): + self._ini_capture_ready = True + ini = b"" # <-- BUG: bytes, not str + else: + try: + ini = imgui.save_ini_settings_to_memory() # returns str + except Exception: + ini = b"" # <-- BUG: same + ... +``` + +The bug: `ini = b""` is a `bytes` literal, but the `WorkspaceProfile` dataclass declares `ini_content: str` (`src/models.py:799`), AND `tomli_w` (the TOML serializer) raises `TypeError: Object of type 'bytes' is not TOML serializable`. + +Verified empirically: +```python +>>> import tomli_w +>>> tomli_w.dump({"ini_content": b""}, io.BytesIO()) +TypeError: Object of type 'bytes' is not TOML serializable +``` + +Trace path for the failure: +1. Test: `set_value('ui_separate_tier1', True)` → field is `True` in app state. +2. Test: `push_event("custom_callback", {"callback": "save_workspace_profile", ...})`. +3. GUI: `_process_pending_gui_tasks` → `_cb_save_workspace_profile` (`src/app_controller.py:2870`). +4. App: `_capture_workspace_profile(name)` → returns `WorkspaceProfile(..., ini_content=b"", ...)`. +5. `workspace_manager.save_profile(profile)` → `profile.to_dict()` → `{"ini_content": b"", ...}`. +6. `_save_file` → `tomli_w.dump(data, f)` → **TypeError raised**. +7. Exception propagates; profile is **NOT saved to disk**; `workspace_profiles` is **NOT reloaded**; `self._app.workspace_profiles` is **NOT updated**. +8. Test: `set_value('ui_separate_tier1', False)` → field is `False`. +9. Test: `push_event("custom_callback", {"callback": "load_workspace_profile", ...})`. +10. App: `_cb_load_workspace_profile(name)` → `if name in self.workspace_profiles:` → `False` (save failed) → **does nothing**. +11. Test: `assert get_value('ui_separate_tier1') is True` → **fails** (still `False`). + +The original pre-defer code (`ini = imgui.save_ini_settings_to_memory()`) returned a `str` that round-tripped through TOML successfully; tests passed. The defer fix introduced a type-incompatible sentinel value that broke the serialization contract. + +The 1-line fix: change `ini = b""` to `ini = ""` (and add a defensive str-coerce for the non-defer path). + +### Root cause analysis (Category B — prior session test) + +The test mocks `imscope.window(...)` to return a `MagicMock()` whose `__enter__` returns the bare mock. Production code at `src/gui_2.py:2333` does `with imscope.window(...) as (opened, visible):` which expects a 2-tuple. The test's setup (lines ~70-80) sets `__enter__` for many imscope context managers to return non-iterable `MagicMock()` but for `popup_modal` (line ~91) correctly returns `(True, None)`. The `imscope.window` setup is missing the tuple-return — purely a test-authoring bug. + +## 2. Goals + +1. **Restore 100% pass rate on the 272-file test suite** (no regressions in any other test). +2. **Preserve the defer-not-catch safety property** of commit `d7487af4` (avoid C-level crash on early-render C calls). +3. **Harden the defer-not-catch documentation** to call out the str/bytes type contract (avoid future regressions of the same kind). +4. **Tighten the test-authoring contract** for the prior session test: mock imscope context managers with the correct return shape. + +## 3. Non-Goals + +- Not refactoring the workspace profile save/load architecture. +- Not adding wait-for-ready semantics to the test framework (deferred to a separate live_gui harden track; tracked as backlog item 0 in `conductor/tracks.md`). +- Not fixing the broader test fragility / session-state issues (deferred). +- Not addressing `sloppy.py` startup latency (separate track, also backlog). + +## 4. Design + +### Change 1: Fix `ini = b""` → `ini = ""` in `_capture_workspace_profile` + +**Files:** +- Modify: `src/gui_2.py:601-606` (the defer branch) +- Modify: `src/gui_2.py:606-609` (the non-defer branch's `except` handler) + +**Approach:** Change `ini = b""` to `ini = ""` in both places. The pre-fix code returned a `str`; we're restoring that contract. Additionally, defensively coerce the non-defer result: `ini = imgui.save_ini_settings_to_memory()` returns a `str` per `imgui-bundle` docs, but to be safe against future imgui-bundle changes, wrap it: `ini = str(imgui.save_ini_settings_to_memory() or "")`. + +```python +def _capture_workspace_profile(self, name: str) -> models.WorkspaceProfile: + if not getattr(self, "_ini_capture_ready", False): + self._ini_capture_ready = True + ini = "" + else: + try: + ini = str(imgui.save_ini_settings_to_memory() or "") + except Exception: + ini = "" + panel_states = { ... } + return models.WorkspaceProfile(...) +``` + +**Why:** `WorkspaceProfile.ini_content: str` (`src/models.py:799`); `tomli_w` rejects `bytes`. `imgui.load_ini_settings_from_memory(ini_data: str, ...)` also expects `str`. Restoring the `str` contract is the minimal fix. + +**Alternatives considered:** +- A2 — Use `imgui.save_ini_settings_to_disk(path)` then read the file. **Rejected**: adds a side-effect path that's not idempotent; tests can pollute the test artifacts dir. +- A3 — Force a frame render in `__init__` so the first call is safe. **Rejected**: changes init semantics; interacts badly with hot-reload (`src/hot_reloader.py`); may regress startup latency (the very thing the new sloppy.py startup track is meant to address). + +### Change 2: Fix the prior session test mock + +**Files:** +- Modify: `tests/test_prior_session_no_pop_imbalance.py` (the imscope.window mock setup) + +**Approach:** Add the tuple-return to `imscope.window`'s `__enter__` mock, matching the pattern already used for `popup_modal` at line 91: + +```python +mock_imscope.window.return_value.__enter__ = MagicMock(return_value=(True, True)) +mock_imscope.window.return_value.__exit__ = MagicMock(side_effect=_scope_exit) +``` + +**Why:** The test's `imscope.window` setup is the only one missing the tuple-return; all other imscope context managers that production code expects to unpack as tuples already have it. This is a 2-line test-only fix. + +### Change 3: Add a regression test for the ini_content type contract + +**Files:** +- Create: `tests/test_workspace_profile_serialization.py` + +**Approach:** Add a unit test that verifies a `WorkspaceProfile` with `ini_content=""` (empty str) round-trips through TOML via `to_dict` → `tomli_w.dump` → `tomllib.load` → `from_dict` without raising. This is the contract that the defer fix violated. + +```python +def test_workspace_profile_empty_ini_content_roundtrips(): + from src.models import WorkspaceProfile + profile = WorkspaceProfile(name="t", ini_content="", show_windows={"A": True}, panel_states={"x": 1}) + d = profile.to_dict() + import io, tomli_w, tomllib + buf = io.BytesIO() + tomli_w.dump({profile.name: d}, buf) # this is what save_profile does + buf.seek(0) + back = tomllib.load(buf) + loaded = WorkspaceProfile.from_dict("t", back["t"]) + assert loaded.ini_content == "" + assert loaded.show_windows == {"A": True} + assert loaded.panel_states == {"x": 1} +``` + +**Why:** This test would have caught the `d7487af4` regression. It encodes the type contract for future contributors. It's a pure unit test, no live_gui, runs in <1s. + +### Change 4: Harden the defer-not-catch doc + +**Files:** +- Modify: `docs/guide_gui_2.md` "Workspace Profile Defer-Not-Catch" section +- Modify: `docs/guide_testing.md` "Early-Render C-Level Crashes" section +- Modify: `conductor/workflow.md` "Defer-Not-Catch Pattern for Native Crashes" section + +**Approach:** Add a note: "When implementing a defer-not-catch guard for a return value, **ensure the sentinel value matches the type contract of the downstream consumer**. For `WorkspaceProfile.ini_content: str`, the sentinel must be `""` (str), not `b""` (bytes) — TOML serialization rejects bytes." + +**Why:** Future contributors applying the defer-not-catch pattern should not silently introduce type-incompatible sentinels. + +## 5. Data Flow + +### Before (buggy) +``` +set_value(True) → app.ui_separate_tier1 = True +save_workspace_profile → _capture_workspace_profile → ini=b"" (bytes) + → to_dict() → {"ini_content": b""} + → tomli_w.dump → TypeError + → profile NOT saved +set_value(False) → app.ui_separate_tier1 = False +load_workspace_profile → name not in workspace_profiles → no-op +assert get_value is True → FAILS (still False) +``` + +### After (fixed) +``` +set_value(True) → app.ui_separate_tier1 = True +save_workspace_profile → _capture_workspace_profile → ini="" (str) + → to_dict() → {"ini_content": ""} + → tomli_w.dump → OK + → profile saved +set_value(False) → app.ui_separate_tier1 = False +load_workspace_profile → name in workspace_profiles → _apply_workspace_profile + → setattr(self, "ui_separate_tier1", True) +assert get_value is True → PASSES +``` + +## 6. Error Handling + +- The defer branch and the `except` branch both set `ini = ""`. Empty string is a valid `str` and is safe for `tomli_w`, for the dataclass, and for `imgui.load_ini_settings_from_memory("")` (which is a no-op that lets ImGui use its defaults). +- No new exceptions are introduced. The `TypeError` from the buggy `b""` goes away because the type is now `str`. +- The new regression test (`test_workspace_profile_serialization.py`) is itself a forward-looking guard: if a future change reintroduces a bytes sentinel, the test will fail with a clear message. + +## 7. Testing Strategy + +### New tests +- `tests/test_workspace_profile_serialization.py::test_workspace_profile_empty_ini_content_roundtrips` — pure unit test, <1s, encodes the str contract. + +### Existing tests that should now pass +- `tests/test_auto_switch_sim::test_auto_switch_sim` — saves+loads workspace profile. +- `tests/test_workspace_profiles_sim::test_workspace_profiles_restoration` — saves+loads workspace profile. +- `tests/test_prior_session_no_pop_imbalance::test_no_extraneous_pop_when_prior_session_renders` — mock setup fix. + +### Regression check +- Re-run the full batched test suite (`scripts/run_tests_batched.py`) after the fixes; expect 272/272 pass. +- Re-run targeted batches of theme tests (`test_theme*`, `test_log_pruner*`, `test_view_presets*`, `test_gui_progress*`, `test_gui_phase4*`) to verify the prior doc-track fixes still pass. + +## 8. Risk Assessment + +| Risk | Likelihood | Impact | Mitigation | +|---|---|---|---| +| The `str()` coercion in the non-defer branch changes behavior | Low | Low | `imgui.save_ini_settings_to_memory()` is documented to return `str`; the coercion is defensive only. The `or ""` handles a `None` return (which `imgui-bundle` does not produce but we don't want to crash on). | +| The new unit test depends on `tomli_w` semantics that change | Very low | Low | `tomli_w` is a stable dep; the test would only break if `bytes` becomes serializable, which would be a major version change. | +| The mock fix in the prior session test changes other behavior | Low | Low | The fix only adds the missing tuple-return; existing mocks for other imscope context managers are untouched. | +| Removing the `b""` sentinel causes the early-render C crash to return | Very low | High | The `try/except Exception` around `imgui.save_ini_settings_to_memory()` is preserved; the flag-based defer is preserved. Only the type of the sentinel changes. | + +## 9. Out of Scope (Tracked Separately) + +- **live_gui session-state contract** (test-authoring rigor, wait-for-ready pattern) — see [docs/guide_testing.md#authoring-robust-live_gui-tests-dont-assume-clean-state] (added in this session). This is a doc-only change; tests will be hardened over time as they break. +- **sloppy.py startup latency** — new backlog item 0 in `conductor/tracks.md`, planned via superpowers writing-plans skill in a future session. +- **Other live_gui tests still flagged as fragile in the regression-fixes plan** (MMA engine state transitions, RAG status timing) — these were in the deferred category of the `regression_fixes_20260605` plan; not addressed by this design. + +## 10. References + +- Commit `d7487af4` — the defer-not-catch fix that introduced the `b""` sentinel. +- `src/gui_2.py:601-606` — current defer code. +- `src/models.py:797-823` — `WorkspaceProfile` dataclass with `ini_content: str`. +- `src/workspace_manager.py:48-58` — `save_profile` that calls `to_dict` then `tomli_w.dump`. +- `docs/guide_gui_2.md#workspace-profile-defer-not-catch` — the defer-not-catch section to harden. +- `docs/guide_testing.md#known-gotchas-2026-06-05` — the early-render C-crash section to harden. +- `conductor/tracks.md` — `regression_fixes_20260605` and `multi_themes_20260604` entries. +- `conductor/tracks.md` — new backlog item 0 (sloppy.py startup speedup).