fix(gui): correct __getattr__ to not silently return None for missing ui_ attrs
PR1 follow-up (the actual IM_ASSERT root cause fix).
The IM_ASSERT in 'MainDockSpace' was triggered by the
render_approve_script_modal function (gui_2.py:4895) calling
imgui.checkbox with a None value for app.ui_approve_modal_preview.
The chain of bugs:
1. AppController.__getattr__ returned None for ANY ui_ attribute
(line 1237-1238). This was intended as a safety net for ui_*
flags defined in __init__ but it was too généreux: it returned
None for ui_ attrs that were NEVER set.
2. The pattern in render_approve_script_modal:
if not hasattr(app, 'ui_approve_modal_preview'):
app.ui_approve_modal_preview = False
_, app.ui_approve_modal_preview = imgui.checkbox(..., app.ui_approve_modal_preview)
relied on hasattr() returning False for unset attrs to trigger
the initialization. But the App.__setattr__ checks
hasattr(self.controller, name) to decide where to route
assignments. The controller's __getattr__ returned None for
ui_approve_modal_preview, so hasattr() returned True. The
App.__setattr__ routed the assignment to the controller.
The controller's __getattr__ then returned None on read,
silently dropping the False value.
3. The next line called imgui.checkbox with None, which raised
a TypeError. The TypeError propagated out of
render_approve_script_modal without closing the modal,
leaving the ImGui scope stack unbalanced. The unbalanced
scope triggered IM_ASSERT(Missing End()) on the next frame.
Fix: AppController.__getattr__ now only returns None for an
EXPLICIT allowlist of ui_ attrs that are defined in __init__.
For any other missing attribute (including the case
'hasattr() should return False'), it raises AttributeError.
The App.__getattr__ was also fixed (per the test) to check
hasattr(controller, name) before delegating. This is defense in
depth in case other __getattr__ patterns are added.
Test verification (TDD red → green):
- 1/1 test_app_getattr_hasattr_bug PASSES (verifies hasattr
returns False for unset attrs via App.__getattr__)
- 1/1 test_app_controller_getattr_ui_bug PASSES (verifies hasattr
returns False for unset ui_ attrs on controller)
Live verification:
- 4 sims + test_live_workflow + 2 markdown tests: 7/7 PASS in 83.15s
- Previously failed at 200s+ with 'cannot schedule new futures after
shutdown' / 121s with 'GUI is degraded before test starts'
- Now passes cleanly. The IM_ASSERT no longer fires.
13/13 related unit tests pass (app_controller_* + app_run_* +
app_getattr_*). No regressions in 51/51 io_pool/warmup/sigint/etc.
unit tests.
This commit is contained in:
@@ -0,0 +1,94 @@
|
||||
# Progress Report: test_full_live_workflow IM_ASSERT Investigation (2026-06-08 PM)
|
||||
|
||||
**Supersedes:** `docs/reports/test_full_live_workflow_imgui_assert_20260608.md` (initial root cause)
|
||||
**Date:** 2026-06-08 PM
|
||||
**Status:** 3 PRs landed (PR1 audit, PR2 wrap+health, PR3 pre-flight check). PR4 (real fix) deferred. Investigation continues.
|
||||
**Related:** `conductor/todos/TODO_test_full_live_workflow_v2.md`
|
||||
|
||||
---
|
||||
|
||||
## TL;DR
|
||||
|
||||
PR2 (wrap + health endpoint) and PR3 (pre-flight health check) are landed. The pre-flight check now fails the test fast in **76s with a clear, actionable message** instead of **200s with a confusing 120s timeout**.
|
||||
|
||||
**The IM_ASSERT itself is still happening**, but it's no longer silently poisoning the test. The user can now see:
|
||||
- The exact RuntimeError: `IM_ASSERT( (0) && "Missing End()" ) --- imgui.cpp:11662`
|
||||
- The full traceback pointing to `src/gui_2.py:619` in `app.run`
|
||||
- A note that the new test cannot proceed with dirty state
|
||||
|
||||
**The actual IM_ASSERT trigger (which `begin()` is missing its `end()`) is still unidentified.** The static `check_imgui_scopes.py` audit found 3 false positives. The real bug needs targeted investigation.
|
||||
|
||||
---
|
||||
|
||||
## What Was Landed (3 PRs)
|
||||
|
||||
### PR1: Audit Findings (no commit, documented)
|
||||
- Ran `scripts/check_imgui_scopes.py` against `src/gui_2.py`
|
||||
- Found 3 "extra end*" calls at lines 2920, 3843, 5455
|
||||
- All 3 are **false positives**:
|
||||
- Line 2920 (`render_persona_editor_window`): the `if not is_embedded:` pattern around the matching `begin()` makes the audit script confused
|
||||
- Line 3843 (`render_discussion_entry`): try/except early return path makes the audit see 2 end_groups for 1 begin_group
|
||||
- Line 5455 (`render_tier_stream_panel`): same try/except pattern
|
||||
- The static audit cannot find the real bug because it doesn't understand control flow
|
||||
|
||||
### PR2: Wrap `immapp.run` + `/api/gui_health` endpoint
|
||||
**Commit:** `1c565da7`
|
||||
- `src/gui_2.py:618` — `immapp.run` is now wrapped in `try/except RuntimeError`. On IM_ASSERT:
|
||||
- Logs the error to stderr at ERROR level (NOT silent)
|
||||
- Records `_gui_degraded_reason` and `_last_imgui_assert` on the controller
|
||||
- Returns from `run()` so the hook server keeps serving
|
||||
- `src/app_controller.py` — new state attributes
|
||||
- `src/api_hooks.py` — new `/api/gui_health` endpoint
|
||||
- `src/api_hook_client.py` — new `get_gui_health()` method
|
||||
- 4 new unit tests + 1 live test, all passing
|
||||
|
||||
### PR3: Pre-flight health check in test_full_live_workflow
|
||||
**Commit:** `51ecace4`
|
||||
- `tests/test_live_workflow.py:43-57` — pre-flight check at start of test
|
||||
- If `client.get_gui_health()['healthy']` is False, fails fast with a clear message
|
||||
|
||||
**Verification (latest run, 2026-06-08 PM):**
|
||||
| Test | Before | After |
|
||||
|------|--------|-------|
|
||||
| `test_full_live_workflow` isolation | 11.5s PASS | 13.45s PASS |
|
||||
| Tier-3 batch (50 files) | 200s FAIL (timeout) | 164.9s FAIL (fast-fail in 76s) |
|
||||
| Failure message | `RuntimeError: cannot schedule new futures after shutdown` (after 120s) | `Failed: GUI is degraded before test starts. degraded_reason='immapp.run raised RuntimeError: IM_ASSERT(...)'. This is likely caused by a prior test in the same live_gui session crashing the GUI.` |
|
||||
|
||||
---
|
||||
|
||||
## What Is Still Needed (PR1 follow-up + PR4)
|
||||
|
||||
### PR1 follow-up: Find the actual IM_ASSERT trigger
|
||||
The `IM_ASSERT` in `MainDockSpace: Missing End()` says SOMETHING opened a `begin()` without a matching `end()`. After 4 sims have run their panel renders, the cumulative ImGui scope stack has an unbalanced entry. The offending render function is unknown.
|
||||
|
||||
**Candidates to investigate:**
|
||||
- `render_tier_stream_panel` (called by the 4 tier windows, line 5409)
|
||||
- `render_execution_panel` (the execution sim opens this)
|
||||
- `render_mma_modal` (MMA approval dialogs)
|
||||
- `render_files_and_media` (file context panel)
|
||||
- `render_persona_editor_window` (opened by ai_settings sim)
|
||||
- `render_topic_picker` (MMA topic selection modal)
|
||||
- `render_mma_dashboard` (the dashboard that shows the tier streams)
|
||||
- The mma_state_update / mma_stream rendering code
|
||||
|
||||
**The trigger is cumulative state corruption** — the bug doesn't fire on a single render. After 4 sims each opening different panels, the ImGui scope stack has a phantom `begin()` from one of them that never got its `end()`.
|
||||
|
||||
### PR4: Apply the fix
|
||||
Once PR1 follow-up identifies the offending function, fix it using the `imscope` context manager from `src/imgui_scopes.py` (per the `conductor/workflow.md` defer-not-catch pattern).
|
||||
|
||||
### Future Track Foundation
|
||||
A dedicated track for the broader test infrastructure improvements:
|
||||
- Per-test auto-respawn in `live_gui` fixture (avoiding dirty state in the first place)
|
||||
- Per-file fixture scope (more isolation)
|
||||
- The `IM_ASSERT` itself is a render function bug that should be tracked separately
|
||||
|
||||
---
|
||||
|
||||
## Next Steps
|
||||
|
||||
1. **Investigate the actual IM_ASSERT trigger** via targeted log analysis
|
||||
2. **Apply TDD fix** once identified
|
||||
3. **Verify** test_full_live_workflow passes in tier-3 batch
|
||||
4. **Write future track foundation** document
|
||||
|
||||
The investigation continues. The next step is to add targeted logging to find which render function is leaving an unbalanced scope.
|
||||
+24
-4
@@ -1231,10 +1231,30 @@ class AppController:
|
||||
"__reduce__", "__reduce_ex__", "__getnewargs__",
|
||||
):
|
||||
raise AttributeError(name)
|
||||
# Only return a default for the UI flag pattern (set in init_state).
|
||||
# Other lazy attributes (e.g. persona_manager set in
|
||||
# _load_active_project) should raise so hasattr() returns False.
|
||||
if name.startswith("ui_") or name == "rag_engine":
|
||||
# Only return a default for the explicit UI flag list (those defined
|
||||
# in __init__ as defaults). The previous implementation returned None for
|
||||
# ANY ui_ attribute, which broke hasattr() and the App.__setattr__
|
||||
# routing logic. For any other missing attribute, raise AttributeError
|
||||
# so hasattr() returns False correctly.
|
||||
_UI_FLAG_DEFAULTS = {
|
||||
"ui_active_tool_preset", "ui_active_bias_profile",
|
||||
"ui_ai_input", "ui_disc_new_name_input", "ui_disc_new_role_input",
|
||||
"ui_epic_input", "ui_new_track_name", "ui_new_track_desc",
|
||||
"ui_new_track_type", "ui_project_conductor_dir",
|
||||
"ui_conductor_setup_summary", "ui_last_script_text",
|
||||
"ui_last_script_output", "ui_new_ticket_id", "ui_new_ticket_desc",
|
||||
"ui_new_ticket_target", "ui_new_ticket_deps", "ui_output_dir",
|
||||
"ui_files_base_dir", "ui_shots_base_dir", "ui_project_git_dir",
|
||||
"ui_project_system_prompt", "ui_project_execution_mode",
|
||||
"ui_gemini_cli_path", "ui_word_wrap", "ui_auto_add_history",
|
||||
"ui_separate_message_panel", "ui_separate_response_panel",
|
||||
"ui_separate_tool_calls_panel", "ui_global_system_prompt",
|
||||
"ui_base_system_prompt", "ui_use_default_base_prompt",
|
||||
"ui_project_context_marker", "ui_agent_tools", "ui_manual_approve",
|
||||
"ui_disc_truncate_pairs", "ui_auto_scroll_comms",
|
||||
"ui_auto_scroll_tool_calls", "ui_focus_agent", "ui_active_persona",
|
||||
}
|
||||
if name in _UI_FLAG_DEFAULTS or name == "rag_engine":
|
||||
return None
|
||||
raise AttributeError(name)
|
||||
|
||||
|
||||
+3
-1
@@ -688,7 +688,9 @@ class App:
|
||||
def __getattr__(self, name: str) -> Any:
|
||||
if name == 'controller':
|
||||
raise AttributeError(name)
|
||||
return getattr(self.controller, name)
|
||||
if hasattr(self, 'controller') and hasattr(self.controller, name):
|
||||
return getattr(self.controller, name)
|
||||
raise AttributeError(name)
|
||||
|
||||
def __setattr__(self, name: str, value: Any) -> None:
|
||||
if name != 'controller' and hasattr(self, 'controller') and hasattr(self.controller, name):
|
||||
|
||||
@@ -0,0 +1,46 @@
|
||||
"""Regression tests for AppController.__getattr__ ui_* default bug.
|
||||
|
||||
The bug: `AppController.__getattr__` returns `None` for any attribute
|
||||
starting with `ui_` (e.g. `ui_separate_task_dag`, `ui_approve_modal_preview`).
|
||||
This makes `hasattr(ctrl, 'ui_X')` return True for ANY ui_X attribute,
|
||||
even if the attribute was never set on the instance.
|
||||
|
||||
Consequence: `App.__setattr__` checks `hasattr(self.controller, name)`
|
||||
to decide where to route assignments. With the controller's broken
|
||||
__getattr__, all `ui_*` assignments get routed to the controller.
|
||||
But the controller's `__getattr__` then returns None on read,
|
||||
silently dropping the assigned value.
|
||||
|
||||
This breaks the `if not hasattr(app, 'foo'): app.foo = False`
|
||||
pattern in render_approve_script_modal (gui_2.py:4894): the hasattr
|
||||
returns True (via the controller), so the assignment never runs,
|
||||
and the next line tries to call checkbox with None.
|
||||
|
||||
The fix: AppController.__getattr__ should only return None for
|
||||
attributes that have been set (in self.__dict__). For unset
|
||||
attributes, raise AttributeError so hasattr() returns False.
|
||||
"""
|
||||
import pytest
|
||||
import sys
|
||||
import os
|
||||
from typing import Any
|
||||
|
||||
ROOT = os.path.abspath(os.path.join(os.path.dirname(__file__), ".."))
|
||||
sys.path.insert(0, ROOT)
|
||||
|
||||
|
||||
def test_hasattr_returns_false_for_unset_ui_attribute(mock_app: Any) -> None:
|
||||
"""hasattr(ctrl, 'ui_test_attr_xyz') must return False when the
|
||||
attribute is not set on the instance. Otherwise the App.__setattr__
|
||||
routing logic misbehaves.
|
||||
"""
|
||||
app = mock_app
|
||||
ctrl = app.controller
|
||||
# Use a unique attribute name guaranteed not to be in init_state()
|
||||
attr_name = "ui_test_attr_xyz_unique_12345"
|
||||
# Force the attribute lookup
|
||||
assert hasattr(ctrl, attr_name) is False, (
|
||||
f"hasattr(ctrl, {attr_name!r}) returned True for an unset attribute. "
|
||||
"AppController.__getattr__ is returning None for any ui_* attribute, "
|
||||
"which breaks hasattr() and the App.__setattr__ routing logic."
|
||||
)
|
||||
@@ -0,0 +1,40 @@
|
||||
"""Regression tests for the App.__getattr__ passthrough bug.
|
||||
|
||||
The bug: `App.__getattr__` delegates unknown attributes to the
|
||||
controller via `getattr(self.controller, name)`. If the controller
|
||||
doesn't have the attribute either, `getattr` returns None (the default).
|
||||
|
||||
This breaks `hasattr(app, 'unknown_attr')`:
|
||||
- hasattr() checks the instance dict first
|
||||
- If not found, it calls __getattr__
|
||||
- __getattr__ returns None instead of raising AttributeError
|
||||
- hasattr() sees a non-exception result, returns True
|
||||
- Caller's `if not hasattr(...)` is False, never initializes the attr
|
||||
|
||||
The fix: __getattr__ should raise AttributeError for attributes that
|
||||
the controller also doesn't have, so hasattr() works correctly.
|
||||
"""
|
||||
import pytest
|
||||
import sys
|
||||
import os
|
||||
from typing import Any
|
||||
|
||||
ROOT = os.path.abspath(os.path.join(os.path.dirname(__file__), ".."))
|
||||
sys.path.insert(0, ROOT)
|
||||
|
||||
|
||||
def test_hasattr_returns_false_for_unset_attribute(mock_app: Any) -> None:
|
||||
"""hasattr(app, 'definitely_unique_test_attr_xyz123') must return False
|
||||
when neither App nor controller has the attribute. This is the exact
|
||||
pattern that fails in render_approve_script_modal (gui_2.py:4894):
|
||||
`if not hasattr(app, 'foo'): app.foo = False`.
|
||||
"""
|
||||
app = mock_app
|
||||
# Use a unique attribute name guaranteed not to exist
|
||||
attr_name = "definitely_unique_test_attr_xyz123"
|
||||
# Assert hasattr returns False
|
||||
assert hasattr(app, attr_name) is False, (
|
||||
f"hasattr returned True for {attr_name!r} which is not set on App "
|
||||
"or controller. This breaks the `if not hasattr: initialize` "
|
||||
"pattern in render_approve_script_modal (gui_2.py:4894)."
|
||||
)
|
||||
Reference in New Issue
Block a user