From 66618258129ea625a926622cb6cacd8c7104b1cf Mon Sep 17 00:00:00 2001 From: smb Date: Fri, 4 Sep 2026 20:45:20 +0200 Subject: [PATCH 1/2] fix: treat a confirmed dialog as confirmed QMessageBox.question returns a plain int, not a StandardButton member: on PySide6 6.10.3 pressing Yes yields 16384, and `16384 is StandardButton.Yes` is False while `==` is True. Four call sites compared the answer with `is`, so every confirmation in the application was read as a refusal. The dialog appeared, the user pressed Yes, and nothing happened at all: no removal, no download, no message, no error. Affected: removing one model weight, Delete outdated, Re-download outdated, and deleting a pinned cut set. All four now compare with equality, and the one that reads least obviously carries a comment saying why, so it does not get tidied back into an identity check. The suite could not have caught this. Every test monkeypatched QMessageBox.question to return the canonical enum member, for which identity does hold, so the stub asserted the opposite of what Qt does. The stubs now return int(StandardButton.Yes), which is what Qt returns, and one test drives a real QMessageBox to completion by finding the active modal window and clicking its YesRole button. That test needs no stub at all and guards the whole class rather than these four instances. Closes #37 Co-Authored-By: Claude Opus 5 (1M context) --- src/matteloop/ui/model_manager/_controller.py | 17 ++- src/matteloop/ui/workspace_controller.py | 2 +- tests/ui/test_model_manager.py | 107 +++++++++++++++--- 3 files changed, 98 insertions(+), 28 deletions(-) diff --git a/src/matteloop/ui/model_manager/_controller.py b/src/matteloop/ui/model_manager/_controller.py index 15bd60b..cadf648 100644 --- a/src/matteloop/ui/model_manager/_controller.py +++ b/src/matteloop/ui/model_manager/_controller.py @@ -139,8 +139,6 @@ def _removal_block_reason(self, entry: ModelEntry) -> str | None: return "Cannot remove a model while a job is running." if self._manager is None: return "Model removal is unavailable in this runtime." - if self._manager.active_id == entry.model_id: - return "Cannot remove the model used by the active session." return None def _download_requested(self, value: object) -> None: @@ -196,7 +194,9 @@ def _delete_outdated_requested(self) -> None: QMessageBox.StandardButton.Yes | QMessageBox.StandardButton.No, QMessageBox.StandardButton.No, ) - if answer is not QMessageBox.StandardButton.Yes: + # QMessageBox.question returns an int, so identity with StandardButton + # does not hold. + if answer != QMessageBox.StandardButton.Yes: return self._start_obsolete_removal(manager) @@ -213,10 +213,7 @@ def _redownload_outdated_requested(self) -> None: return if self._remove_thread is not None: return - if ( - _confirm_redownload(self.dialog, entries) - is not QMessageBox.StandardButton.Yes - ): + if _confirm_redownload(self.dialog, entries) != QMessageBox.StandardButton.Yes: return cancel = CancellationState() self._batch = {entry.model_id: entry for entry in entries} @@ -373,7 +370,9 @@ def _obsolete_removal_failed(self, error: object) -> None: def _remove_requested(self, value: object) -> None: if not isinstance(value, ModelEntry) or not value.cached: return - if self._removal_block_reason(value) is not None: + reason = self._removal_block_reason(value) + if reason is not None: + self.dialog.set_message(reason) return if self._remove_thread is not None: return @@ -385,7 +384,7 @@ def _remove_requested(self, value: object) -> None: QMessageBox.StandardButton.Yes | QMessageBox.StandardButton.No, QMessageBox.StandardButton.No, ) - if answer is not QMessageBox.StandardButton.Yes: + if answer != QMessageBox.StandardButton.Yes: return manager = self._manager if manager is None: diff --git a/src/matteloop/ui/workspace_controller.py b/src/matteloop/ui/workspace_controller.py index c75ad14..5868cda 100644 --- a/src/matteloop/ui/workspace_controller.py +++ b/src/matteloop/ui/workspace_controller.py @@ -85,4 +85,4 @@ def _confirm_pinned_delete(self) -> bool: QMessageBox.StandardButton.Yes | QMessageBox.StandardButton.No, QMessageBox.StandardButton.No, ) - return answer is QMessageBox.StandardButton.Yes + return answer == QMessageBox.StandardButton.Yes diff --git a/tests/ui/test_model_manager.py b/tests/ui/test_model_manager.py index 4863c81..ece7f63 100644 --- a/tests/ui/test_model_manager.py +++ b/tests/ui/test_model_manager.py @@ -5,9 +5,9 @@ from threading import Event, get_ident import pytest -from PySide6.QtCore import QUrl +from PySide6.QtCore import QTimer, QUrl from PySide6.QtGui import QDesktopServices -from PySide6.QtWidgets import QMessageBox +from PySide6.QtWidgets import QApplication, QMessageBox from matteloop.core.errors import AppError, ErrorCode from matteloop.core.parameters import ParameterState @@ -192,7 +192,7 @@ def test_model_manager_removes_only_confirmed_selected_weight_in_background( monkeypatch.setattr( QMessageBox, "question", - lambda *_args, **_kwargs: QMessageBox.StandardButton.Yes, + lambda *_args, **_kwargs: int(QMessageBox.StandardButton.Yes), ) reported: list[str] = [] monkeypatch.setattr( @@ -224,7 +224,73 @@ def test_model_manager_removes_only_confirmed_selected_weight_in_background( controller.close() -def test_model_manager_blocks_active_and_running_job_removal( +def test_model_manager_removes_weight_after_real_confirmation( + tmp_path: Path, qtbot +) -> None: + target = _model_path(tmp_path, "u2netp") + target.parent.mkdir(parents=True) + target.write_bytes(b"cached-weight") + manager = FakeModelRemovalService(targets={"u2netp": target}) + controller = ModelManagerController( + ReducerStore(AppState(model_available=True)), + catalog=ModelCatalog.load_resource(), + cache_root=tmp_path, + manager=manager, + ) + qtbot.addWidget(controller.dialog) + controller.open() + controller.dialog.model_list.setCurrentRow( + next( + index + for index, entry in enumerate(controller.dialog.entries) + if entry.model_id == "u2netp" + ) + ) + + def click_active_modal_button() -> None: + app = QApplication.instance() + assert isinstance(app, QApplication) + modal = app.activeModalWidget() + if not isinstance(modal, QMessageBox): + return + yes_button = next( + ( + button + for button in modal.buttons() + if modal.buttonRole(button) == QMessageBox.ButtonRole.YesRole + ), + None, + ) + if yes_button is not None: + yes_button.click() + return + if manager.removed != ["u2netp"]: + return + ok_button = next( + ( + button + for button in modal.buttons() + if modal.buttonRole(button) == QMessageBox.ButtonRole.AcceptRole + ), + None, + ) + if ok_button is not None: + ok_button.click() + + modal_timer = QTimer(controller.dialog) + modal_timer.setInterval(10) + modal_timer.timeout.connect(click_active_modal_button) + modal_timer.start() + try: + controller.dialog.remove_button.click() + qtbot.waitUntil(lambda: manager.removed == ["u2netp"], timeout=5000) + qtbot.waitUntil(lambda: not target.exists(), timeout=5000) + finally: + modal_timer.stop() + controller.close() + + +def test_model_manager_allows_active_and_blocks_running_job_removal( tmp_path: Path, qtbot ) -> None: target = _model_path(tmp_path, "u2netp") @@ -247,8 +313,7 @@ def test_model_manager_blocks_active_and_running_job_removal( ) ) - assert controller.dialog.remove_button.isEnabled() is False - assert "active session" in controller.dialog.remove_button.toolTip() + assert controller.dialog.remove_button.isEnabled() is True running = ReducerStore( AppState( @@ -283,6 +348,12 @@ def test_model_manager_blocks_active_and_running_job_removal( assert running_controller.dialog.remove_button.isEnabled() is False assert "job is running" in running_controller.dialog.remove_button.toolTip() + selected = running_controller.dialog.selected_entry + assert selected is not None + running_controller._remove_requested(selected) + assert running_controller.dialog._message.text() == ( + "Cannot remove a model while a job is running." + ) controller.close() running_controller.close() @@ -529,7 +600,7 @@ def test_model_manager_reports_and_deletes_outdated_copy_alongside_current_weigh QMessageBox, "question", lambda _parent, title, message, *_args: ( - confirmation.append((title, message)) or QMessageBox.StandardButton.Yes + confirmation.append((title, message)) or int(QMessageBox.StandardButton.Yes) ), ) controller.dialog.delete_outdated_button.click() @@ -570,7 +641,7 @@ def test_model_manager_deletes_outdated_directory_without_touching_other_cache( monkeypatch.setattr( QMessageBox, "question", - lambda *_args, **_kwargs: QMessageBox.StandardButton.Yes, + lambda *_args, **_kwargs: int(QMessageBox.StandardButton.Yes), ) controller.dialog.delete_outdated_button.click() @@ -644,7 +715,7 @@ def test_model_manager_confirms_the_download_size_before_re_downloading_outdated QMessageBox, "question", lambda _parent, title, body, *_args: ( - confirmation.append((title, body)) or QMessageBox.StandardButton.No + confirmation.append((title, body)) or int(QMessageBox.StandardButton.No) ), ) @@ -691,7 +762,7 @@ def test_model_manager_re_downloads_each_outdated_weight_then_deletes_its_old_co monkeypatch.setattr( QMessageBox, "question", - lambda *_args, **_kwargs: QMessageBox.StandardButton.Yes, + lambda *_args, **_kwargs: int(QMessageBox.StandardButton.Yes), ) controller.dialog.redownload_outdated_button.click() @@ -757,7 +828,7 @@ def record(message: str) -> None: monkeypatch.setattr( QMessageBox, "question", - lambda *_args, **_kwargs: QMessageBox.StandardButton.Yes, + lambda *_args, **_kwargs: int(QMessageBox.StandardButton.Yes), ) controller.dialog.redownload_outdated_button.click() @@ -792,7 +863,7 @@ def test_model_manager_cancel_stops_the_batch_and_keeps_the_rest_outdated( monkeypatch.setattr( QMessageBox, "question", - lambda *_args, **_kwargs: QMessageBox.StandardButton.Yes, + lambda *_args, **_kwargs: int(QMessageBox.StandardButton.Yes), ) controller.dialog.redownload_outdated_button.click() @@ -840,7 +911,7 @@ def test_model_manager_escape_cancels_a_running_download_instead_of_hiding_the_d monkeypatch.setattr( QMessageBox, "question", - lambda *_args, **_kwargs: QMessageBox.StandardButton.Yes, + lambda *_args, **_kwargs: int(QMessageBox.StandardButton.Yes), ) controller.dialog.redownload_outdated_button.click() qtbot.waitUntil(lambda: manager.in_flight, timeout=5000) @@ -877,7 +948,7 @@ def test_model_manager_ignores_dismissal_while_a_removal_runs( monkeypatch.setattr( QMessageBox, "question", - lambda *_args, **_kwargs: QMessageBox.StandardButton.Yes, + lambda *_args, **_kwargs: int(QMessageBox.StandardButton.Yes), ) controller.dialog.delete_outdated_button.click() qtbot.waitUntil(lambda: manager.obsolete_calls == 1, timeout=5000) @@ -923,7 +994,7 @@ def test_model_manager_stops_the_batch_at_the_first_failed_download( monkeypatch.setattr( QMessageBox, "question", - lambda *_args, **_kwargs: QMessageBox.StandardButton.Yes, + lambda *_args, **_kwargs: int(QMessageBox.StandardButton.Yes), ) controller.dialog.redownload_outdated_button.click() @@ -974,7 +1045,7 @@ def test_model_manager_close_cancels_the_running_download_before_waiting( monkeypatch.setattr( QMessageBox, "question", - lambda *_args, **_kwargs: QMessageBox.StandardButton.Yes, + lambda *_args, **_kwargs: int(QMessageBox.StandardButton.Yes), ) controller.dialog.redownload_outdated_button.click() qtbot.waitUntil(lambda: manager.in_flight, timeout=5000) @@ -1089,7 +1160,7 @@ def test_model_manager_refuses_re_download_while_a_job_runs( monkeypatch.setattr( QMessageBox, "question", - lambda *_args, **_kwargs: QMessageBox.StandardButton.Yes, + lambda *_args, **_kwargs: int(QMessageBox.StandardButton.Yes), ) controller.dialog.redownload_outdated_button.click() @@ -1128,7 +1199,7 @@ def fail_after_external_removal() -> int: monkeypatch.setattr( QMessageBox, "question", - lambda *_args, **_kwargs: QMessageBox.StandardButton.Yes, + lambda *_args, **_kwargs: int(QMessageBox.StandardButton.Yes), ) controller.dialog.delete_outdated_button.click() From cee3d30daf5f9fd67ed2ad00dc3a951177f2db2b Mon Sep 17 00:00:00 2001 From: smb Date: Fri, 4 Sep 2026 20:45:20 +0200 Subject: [PATCH 2/2] fix: stop refusing to delete a weight the child merely loaded once A model prepared for one preview stayed the session's active model for the rest of the run: _active_spec is cleared only in close(), which runs at application shutdown. Removing that weight raised MODEL_IN_USE and the button stayed dead, so the default model could never be deleted without first preparing a different one. Nothing in the interface said so. The guard was stricter than the thing it protects. The segmentation child builds its session from the model bytes and holds no handle on the file afterwards, and prepare() returns the cached result for the same model without reading the file again. Deleting the active weight breaks nothing that is running; the next switch away and back downloads it, which the maintainer accepted as the consequence. Removal now refuses only while the model is in the cleanup set, the post-failure state where the child is not known to be healthy, and the error says that rather than naming an active session. The manager's active-model check is gone from the dialog's guard too; the refusal while a job runs is what covers a preparation actually in flight. A blocked click now puts the reason in the status line instead of returning in silence, as the neighbouring actions already do. jobs/models/session.py shrinks from 439 to 437 lines; the baseline records it. Closes #36 Co-Authored-By: Claude Opus 5 (1M context) --- scripts/guardrails-baseline.json | 2 +- src/matteloop/jobs/models/session.py | 6 ++---- tests/jobs/models/test_session.py | 19 ++++++++----------- 3 files changed, 11 insertions(+), 16 deletions(-) diff --git a/scripts/guardrails-baseline.json b/scripts/guardrails-baseline.json index c0f07f5..eda301b 100644 --- a/scripts/guardrails-baseline.json +++ b/scripts/guardrails-baseline.json @@ -20,7 +20,7 @@ "long_functions": 2 }, "src/matteloop/jobs/models/session.py": { - "lines": 439, + "lines": 437, "long_functions": 1 }, "src/matteloop/jobs/render.py": { diff --git a/src/matteloop/jobs/models/session.py b/src/matteloop/jobs/models/session.py index 37d777b..865aacd 100644 --- a/src/matteloop/jobs/models/session.py +++ b/src/matteloop/jobs/models/session.py @@ -220,14 +220,12 @@ def fetch( def remove(self, model_id: str) -> bool: with self._lock: spec = self._catalog.get(model_id) - if (self._active_spec is not None and self._active_spec.id == spec.id) or ( - spec.id in self._cleanup_ids - ): + if spec.id in self._cleanup_ids: raise AppError( ErrorCode.MODEL_IN_USE, "model-cache", "error.model.in-use", - f"model {spec.id!r} owns the active segmentation session", + f"model {spec.id!r} is pending segmentation cleanup", "close-or-switch-model", ) self._require_open() diff --git a/tests/jobs/models/test_session.py b/tests/jobs/models/test_session.py index 3ae7c66..8324a29 100644 --- a/tests/jobs/models/test_session.py +++ b/tests/jobs/models/test_session.py @@ -413,20 +413,17 @@ def test_task9_rejects_custom_options_and_unimplemented_prompts( assert exc.value.code is ErrorCode.MODEL_PREPARATION_INVALID -def test_remove_rejects_active_model_then_removes_exact_inactive_artifact( - tmp_path: Path, -) -> None: +def test_remove_active_and_inactive_model_weights(tmp_path: Path) -> None: manager, _downloader, _clients, _events = _manager(tmp_path) - result = manager.prepare("u2net", {}) - assert result.artifact_path is not None + inactive = manager.prepare("u2net", {}) + active = manager.prepare("u2netp", {}) + assert inactive.artifact_path is not None + assert active.artifact_path is not None - with pytest.raises(AppError) as exc: - manager.remove("u2net") - assert exc.value.code is ErrorCode.MODEL_IN_USE - - manager.prepare("u2netp", {}) assert manager.remove("u2net") is True - assert not result.artifact_path.exists() + assert not inactive.artifact_path.exists() + assert manager.remove("u2netp") is True + assert not active.artifact_path.exists() assert manager.remove("u2net") is False