fix: wrap yield in try-finally in UsethisConfig.set() and change_cwd() (#1834)

Copilot · nathanjmcdougall · web-flow · commit e6862cdfd9a7 · 2026-04-07T16:14:12.000+12:00
* Initial plan * fix: wrap yield in try-finally in UsethisConfig.set() and change_cwd(), update skill, add tests Agent-Logs-Url: https://github.com/usethis-python/usethis-python/sessions/429f9231-8554-40bd-b168-73c3f152f417 Co-authored-by: nathanjmcdougall <18602289+nathanjmcdougall@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: nathanjmcdougall <18602289+nathanjmcdougall@users.noreply.github.com>
diff --git a/.agents/skills/usethis-python-code/SKILL.md b/.agents/skills/usethis-python-code/SKILL.md
@@ -4,7 +4,7 @@ description: Guidelines for Python code design decisions such as when to share v
 compatibility: usethis, Python
 license: MIT
 metadata:
-  version: "1.8"
+  version: "1.9"
 ---
 
 # Python Code Guidelines
@@ -165,6 +165,7 @@ with _preserved(lock_path):
 
 - **Separate backup and restore helpers.** Splitting setup and teardown into two functions forces every caller to remember both calls and wire up `try`/`finally` correctly. A context manager removes this burden.
 - **Forgetting `finally` in the caller.** Without a context manager, it is easy to forget the `finally` block, leaving state unrestored if an exception occurs. A context manager guarantees cleanup.
+- **Placing cleanup after `yield` without `try`/`finally` in a `@contextmanager`.** With `@contextmanager`, cleanup code placed after `yield` does NOT run if an exception is raised inside the `with` block. The exception is re-raised at the `yield` point, causing the generator to terminate without reaching the cleanup code. Always wrap `yield` in `try`/`finally` to guarantee cleanup runs on both normal exit and exceptions.
 
 ## Ordering functions: the step-down rule
 
diff --git a/src/usethis/_config.py b/src/usethis/_config.py
@@ -63,7 +63,7 @@ def copy(self) -> UsethisConfig:
         return replace(self)
 
     @contextmanager
-    def set(  # noqa: PLR0913
+    def set(  # noqa: PLR0912, PLR0913
         self,
         *,
         offline: bool | None = None,
@@ -115,8 +115,10 @@ def set(  # noqa: PLR0913
         if isinstance(project_dir, str):
             project_dir = Path(project_dir)
         self.project_dir = project_dir
-        yield
-        self._restore(old)
+        try:
+            yield
+        finally:
+            self._restore(old)
 
     def _restore(self, other: UsethisConfig) -> None:
         """Restore all attributes from another configuration instance."""
diff --git a/src/usethis/_test.py b/src/usethis/_test.py
@@ -29,9 +29,11 @@ def change_cwd(new_dir: Path) -> Generator[None, None, None]:
     """
     old_dir = Path.cwd()
     os.chdir(new_dir)
-    with usethis_config.set(project_dir=new_dir):
-        yield
-    os.chdir(old_dir)
+    try:
+        with usethis_config.set(project_dir=new_dir):
+            yield
+    finally:
+        os.chdir(old_dir)
 
 
 def is_offline() -> bool:
diff --git a/tests/usethis/test_config.py b/tests/usethis/test_config.py
@@ -103,6 +103,54 @@ def test_set_overrides_cwd(self, tmp_path: Path):
             # Assert
             assert project_dir == Path("42 Wallaby Way, Sydney")
 
+    class TestSetRestoresOnException:
+        def test_restores_quiet_on_exception(self):
+            # Arrange
+            config = UsethisConfig()
+            assert config.quiet is False
+
+            # Act
+            with pytest.raises(RuntimeError), config.set(quiet=True):
+                raise RuntimeError
+
+            # Assert
+            assert config.quiet is False
+
+        def test_restores_all_fields_on_exception(self, tmp_path: Path):
+            # Arrange
+            config = UsethisConfig()
+            old = config.copy()
+
+            # Act
+            with (
+                pytest.raises(RuntimeError),
+                config.set(
+                    offline=True,
+                    quiet=True,
+                    frozen=True,
+                    alert_only=True,
+                    instruct_only=True,
+                    backend=BackendEnum.none,
+                    build_backend=BuildBackendEnum.uv,
+                    disable_pre_commit=True,
+                    subprocess_verbose=True,
+                    project_dir=tmp_path,
+                ),
+            ):
+                raise RuntimeError
+
+            # Assert
+            assert config.offline == old.offline
+            assert config.quiet == old.quiet
+            assert config.frozen == old.frozen
+            assert config.alert_only == old.alert_only
+            assert config.instruct_only == old.instruct_only
+            assert config.backend == old.backend
+            assert config.build_backend == old.build_backend
+            assert config.disable_pre_commit == old.disable_pre_commit
+            assert config.subprocess_verbose == old.subprocess_verbose
+            assert config.project_dir == old.project_dir
+
     class TestDisableUVSubprocess:
         def test_raises_error_when_disabled(self):
             # Act & Assert
