Fix #397: pass encoding="utf-8" to read_text() in tests/ (#402)

* Fix #397: pass encoding="utf-8" to read_text() in tests/

Path.read_text() with no arguments uses locale.getpreferredencoding().
On Windows that's cp1252, which raises UnicodeDecodeError on the
verbatim Korean string in plugin/addons/godot_ai/utils/uv_cache_cleanup.gd.
test_plugin_self_update_safety.py rglob-walks the plugin tree and
crashed 4 tests on Windows; CI is Linux-only (issue #35) so the hazard
was silent until a developer ran pytest on Windows.

Adds encoding="utf-8" to all 87 bare .read_text() calls across
tests/unit/, plus a new test_read_text_encoding.py lint that fails
when any future bare call lands.

Closes #397

* Apply /simplify findings to test_read_text_encoding.py

- Trim oversized module docstring to its WHY (cp1252 trap on Windows)
- Trim assert message to one line; the offender list is the actionable part
- Fast-path skip files where "read_text" doesn't appear before AST parse
  (~50% lint runtime saved on this tree per /simplify efficiency review)
- Add four detector self-tests (positive, negative, unrelated-name,
  multi-offender) so a future refactor can't silently turn the lint into a
  no-op — mirrors pattern in test_no_direct_undo_redo_in_gdscript_tests.py
  and test_gdscript_no_adjacent_string_concat.py

* Document AST lint blind spots from /review nit

Note in the helper docstring that the matcher catches only the keyword
form. Positional `read_text("utf-8")` and `**kwargs` splat are opaque to
AST and would falsely trip the lint — codebase convention is the keyword
form, so the match stays exact.

---------

Co-authored-by: Claude <noreply@anthropic.com>

Author: dsarno

tests/unit/test_audit_data_loss_safeguards.py

@@ -37,7 +37,7 @@
 
 
 def test_runner_declares_install_status_enum() -> None:
-    source = RUNNER_PATH.read_text()
+    source = RUNNER_PATH.read_text(encoding="utf-8")
     assert "enum InstallStatus { OK, FAILED_CLEAN, FAILED_MIXED }" in source, (
         "Three-state install outcome is the contract callers depend on. "
         "FAILED_CLEAN means rollback restored the prior state; FAILED_MIXED "
@@ -48,7 +48,7 @@ def test_runner_declares_install_status_enum() -> None:
 
 
 def test_runner_tracks_paths_written_for_cross_batch_rollback() -> None:
-    source = RUNNER_PATH.read_text()
+    source = RUNNER_PATH.read_text(encoding="utf-8")
     assert "var _paths_written = []" in source, (
         "Per-file install records must accumulate across both `_new_file_paths` "
         "and `_existing_file_paths` batches so a failure in the second batch "
@@ -60,7 +60,7 @@ def test_runner_tracks_paths_written_for_cross_batch_rollback() -> None:
 
 
 def test_install_zip_paths_returns_install_status_and_drives_rollback() -> None:
-    source = RUNNER_PATH.read_text()
+    source = RUNNER_PATH.read_text(encoding="utf-8")
     paths_block = get_func_block(source, "func _install_zip_paths(")
     assert "-> int:" in source[: source.index(paths_block) + len(paths_block)]
     assert "InstallStatus.OK" in paths_block, (
@@ -78,7 +78,7 @@ def test_install_zip_paths_returns_install_status_and_drives_rollback() -> None:
 
 
 def test_install_zip_file_returns_dictionary_record_with_backup_metadata() -> None:
-    source = RUNNER_PATH.read_text()
+    source = RUNNER_PATH.read_text(encoding="utf-8")
     install_block = get_func_block(source, "func _install_zip_file(")
     assert "-> Dictionary:" in source[: source.index(install_block) + len(install_block)]
     # Backup is taken via COPY (not rename) so the original stays in place
@@ -94,7 +94,7 @@ def test_install_zip_file_returns_dictionary_record_with_backup_metadata() -> No
 
 
 def test_install_zip_file_does_not_remove_target_before_rename_attempt() -> None:
-    source = RUNNER_PATH.read_text()
+    source = RUNNER_PATH.read_text(encoding="utf-8")
     install_block = get_func_block(source, "func _install_zip_file(")
     # The first rename attempt must precede any DirAccess.remove_absolute(target_path).
     # The remove-then-rename pattern only appears INSIDE the
@@ -112,7 +112,7 @@ def test_install_zip_file_does_not_remove_target_before_rename_attempt() -> None
 
 
 def test_rollback_returns_failed_mixed_when_any_restore_fails() -> None:
-    source = RUNNER_PATH.read_text()
+    source = RUNNER_PATH.read_text(encoding="utf-8")
     rollback_block = get_func_block(source, "func _rollback_paths_written(")
     assert "-> int:" in source[: source.index(rollback_block) + len(rollback_block)]
     assert "InstallStatus.FAILED_MIXED" in rollback_block, (
@@ -138,17 +138,15 @@ def test_inner_install_restore_failure_surfaces_failed_mixed() -> None:
     its conditional set in `_install_zip_file`, and its consumption in
     `_rollback_paths_written`."""
 
-    source = RUNNER_PATH.read_text()
+    source = RUNNER_PATH.read_text(encoding="utf-8")
     # Member declaration with the protective comment.
     assert "var _restore_failed := false" in source
 
     # `_install_zip_file` must only delete the backup when the restore
     # copy actually succeeded. The pattern is: a guarded copy_absolute
     # call whose return is checked, and an `else: _restore_failed = true`.
     install_block = get_func_block(source, "func _install_zip_file(")
-    assert (
-        "DirAccess.copy_absolute(backup_path, target_path) == OK" in install_block
-    ), (
+    assert "DirAccess.copy_absolute(backup_path, target_path) == OK" in install_block, (
         "Inner restore must check the copy result before treating the "
         "restore as complete. Without this check, a failed copy followed "
         "by an unconditional backup delete strands the file and produces "
@@ -170,7 +168,7 @@ def test_inner_install_restore_failure_surfaces_failed_mixed() -> None:
 
 
 def test_handle_install_failure_refuses_to_reenable_on_mixed_state() -> None:
-    source = RUNNER_PATH.read_text()
+    source = RUNNER_PATH.read_text(encoding="utf-8")
     handler_block = get_func_block(source, "func _handle_install_failure(")
     assert "InstallStatus.FAILED_MIXED" in handler_block
     # The MIXED branch must NOT call set_plugin_enabled(true). The caller
@@ -189,7 +187,7 @@ def test_handle_install_failure_refuses_to_reenable_on_mixed_state() -> None:
 
 
 def test_extract_and_scan_routes_failure_through_handle_install_failure() -> None:
-    source = RUNNER_PATH.read_text()
+    source = RUNNER_PATH.read_text(encoding="utf-8")
     extract_block = get_func_block(source, "func _extract_and_scan() -> void:")
     assert "_install_zip_paths(_new_file_paths)" in extract_block
     assert "_handle_install_failure(status)" in extract_block, (
@@ -213,7 +211,7 @@ def test_extract_and_scan_routes_failure_through_handle_install_failure() -> Non
 def test_atomic_write_does_not_remove_target_before_swap() -> None:
     """The bug pattern: remove(path) then retry rename. Must not return."""
 
-    source = ATOMIC_WRITE_PATH.read_text()
+    source = ATOMIC_WRITE_PATH.read_text(encoding="utf-8")
     write_block = get_func_block(source, "static func write(")
     # The dangerous sequence was: rename failed -> remove(path) -> rename retry.
     # In the new code, remove(path) only happens AFTER a successful copy
@@ -237,7 +235,7 @@ def test_atomic_write_does_not_remove_target_before_swap() -> None:
 
 
 def test_atomic_write_uses_copy_then_verify_as_rename_fallback() -> None:
-    source = ATOMIC_WRITE_PATH.read_text()
+    source = ATOMIC_WRITE_PATH.read_text(encoding="utf-8")
     write_block = get_func_block(source, "static func write(")
     assert "DirAccess.copy_absolute(tmp_path, path)" in write_block, (
         "Overwrite-copy is the safe fallback when rename-over-existing is "
@@ -252,7 +250,7 @@ def test_atomic_write_uses_copy_then_verify_as_rename_fallback() -> None:
 
 
 def test_atomic_write_restores_from_backup_when_swap_fails() -> None:
-    source = ATOMIC_WRITE_PATH.read_text()
+    source = ATOMIC_WRITE_PATH.read_text(encoding="utf-8")
     write_block = get_func_block(source, "static func write(")
     assert "DirAccess.copy_absolute(path, backup_path)" in write_block, (
         "Snapshot the prior file via copy_absolute BEFORE attempting the "
@@ -272,7 +270,7 @@ def test_atomic_write_restore_does_not_remove_path_before_copy() -> None:
     only. `copy_absolute` overwrites by default, so the pre-remove was
     unnecessary AND introduced a window where `path` could disappear."""
 
-    source = ATOMIC_WRITE_PATH.read_text()
+    source = ATOMIC_WRITE_PATH.read_text(encoding="utf-8")
     write_block = get_func_block(source, "static func write(")
 
     # Locate the `if backup_made:` branch and assert it does NOT contain
@@ -296,7 +294,7 @@ def test_atomic_write_restore_does_not_remove_path_before_copy() -> None:
 
 def test_atomic_write_size_verification_uses_utf8_byte_count() -> None:
     """`store_string` writes UTF-8 bytes — verify against to_utf8_buffer().size()."""
-    source = ATOMIC_WRITE_PATH.read_text()
+    source = ATOMIC_WRITE_PATH.read_text(encoding="utf-8")
     verify_block = get_func_block(source, "static func _written_size_matches(")
     assert "content.to_utf8_buffer().size()" in verify_block, (
         "String.length() returns char count which diverges from the byte "
@@ -314,7 +312,7 @@ def test_atomic_write_clears_partial_new_file_when_no_original_existed() -> None
     "destination is in its pre-call state on failure" — for a brand-new path
     that means nothing should be on disk after the function returns false."""
 
-    source = ATOMIC_WRITE_PATH.read_text()
+    source = ATOMIC_WRITE_PATH.read_text(encoding="utf-8")
     write_block = get_func_block(source, "static func write(")
     assert "elif not had_original and FileAccess.file_exists(path):" in write_block, (
         "Failure path must clear partial bytes when no original existed. "

tests/unit/test_camera_current_contract.py

@@ -21,7 +21,7 @@ def test_current_switch_undo_captures_logical_or_effective_current() -> None:
     current", leaving the new camera active after undo.
     """
 
-    source = CAMERA_HANDLER_PATH.read_text()
+    source = CAMERA_HANDLER_PATH.read_text(encoding="utf-8")
     switch_block = get_func_block(
         source,
         "func _add_make_current_to_action(node: Node, type_str: String, scene_root: Node) -> void:",
@@ -31,13 +31,13 @@ def test_current_switch_undo_captures_logical_or_effective_current() -> None:
 
 
 def test_configure_current_false_uses_authoritative_current_guard() -> None:
-    source = CAMERA_HANDLER_PATH.read_text()
+    source = CAMERA_HANDLER_PATH.read_text(encoding="utf-8")
     configure_block = get_func_block(source, "func configure(params: Dictionary) -> Dictionary:")
     assert "var was_on: bool = _resolve_current(scene_root, node)" in configure_block
 
 
 def test_camera_reads_still_route_through_logical_current_resolvers() -> None:
-    source = CAMERA_HANDLER_PATH.read_text()
+    source = CAMERA_HANDLER_PATH.read_text(encoding="utf-8")
     get_block = get_func_block(source, "func get_camera(params: Dictionary) -> Dictionary:")
     list_block = get_func_block(source, "func list_cameras(_params: Dictionary) -> Dictionary:")
     assert "_resolve_current(scene_root, node)" in get_block

tests/unit/test_deferred_timeout_contract.py

@@ -11,12 +11,12 @@
 
 def test_deferred_timeout_error_code_registered_on_both_sides() -> None:
     assert ErrorCode.DEFERRED_TIMEOUT == "DEFERRED_TIMEOUT"
-    gd_error_codes = (PLUGIN_ROOT / "utils" / "error_codes.gd").read_text()
+    gd_error_codes = (PLUGIN_ROOT / "utils" / "error_codes.gd").read_text(encoding="utf-8")
     assert 'const DEFERRED_TIMEOUT := "DEFERRED_TIMEOUT"' in gd_error_codes
 
 
 def test_dispatcher_tracks_deferred_ids_and_emits_timeout_error() -> None:
-    source = (PLUGIN_ROOT / "dispatcher.gd").read_text()
+    source = (PLUGIN_ROOT / "dispatcher.gd").read_text(encoding="utf-8")
     assert "_pending_deferred" in source
     assert 'const ErrorCodes := preload("res://addons/godot_ai/utils/error_codes.gd")' in source
     assert "ErrorCodes.DEFERRED_TIMEOUT" in source

tests/unit/test_dock_cli_timeout.py

@@ -28,7 +28,7 @@
 def test_cli_strategy_routes_every_shell_out_through_mcpcliexec() -> None:
     """No bare OS.execute survives in _cli_strategy.gd."""
 
-    cli_source = (PLUGIN_ROOT / "clients" / "_cli_strategy.gd").read_text()
+    cli_source = (PLUGIN_ROOT / "clients" / "_cli_strategy.gd").read_text(encoding="utf-8")
 
     # The whole point of the refactor: every CLI invocation must go
     # through the bounded helper. A bare OS.execute slipping back in
@@ -50,7 +50,7 @@ def test_cli_strategy_routes_every_shell_out_through_mcpcliexec() -> None:
 def test_cli_exec_helper_uses_pipe_spawn_and_poll_kill() -> None:
     """The helper must spawn detached and kill on timeout — not a blocking OS.execute."""
 
-    helper_source = (PLUGIN_ROOT / "clients" / "_cli_exec.gd").read_text()
+    helper_source = (PLUGIN_ROOT / "clients" / "_cli_exec.gd").read_text(encoding="utf-8")
 
     # Pipe-based spawn returns a PID we can poll on. A blocking
     # OS.execute(..., true) here would just relocate the original hang.
@@ -67,7 +67,7 @@ def test_cli_exec_helper_uses_pipe_spawn_and_poll_kill() -> None:
 def test_cli_strategy_surfaces_timeout_in_configure_and_remove_messages() -> None:
     """A timeout must produce a user-actionable error, not a cryptic exit code."""
 
-    cli_source = (PLUGIN_ROOT / "clients" / "_cli_strategy.gd").read_text()
+    cli_source = (PLUGIN_ROOT / "clients" / "_cli_strategy.gd").read_text(encoding="utf-8")
 
     # The dock surfaces these strings in its row-error label and "Run
     # this manually" panel. Drift here means the user sees "exit code
@@ -84,7 +84,7 @@ def test_cli_strategy_surfaces_timeout_in_configure_and_remove_messages() -> Non
 def test_dock_dispatches_configure_and_remove_to_worker_thread() -> None:
     """Issue #239: the Configure / Remove buttons must not block main."""
 
-    dock_source = (PLUGIN_ROOT / "mcp_dock.gd").read_text()
+    dock_source = (PLUGIN_ROOT / "mcp_dock.gd").read_text(encoding="utf-8")
 
     # The dispatch funnel must exist and route the click into a worker.
     assert "func _dispatch_client_action(" in dock_source
@@ -114,8 +114,8 @@ def test_dock_dispatches_configure_and_remove_to_worker_thread() -> None:
 def test_dock_drains_action_workers_during_install_update_and_exit_tree() -> None:
     """Worker drain must cover both shutdown paths — same reason as the refresh worker."""
 
-    dock_source = (PLUGIN_ROOT / "mcp_dock.gd").read_text()
-    manager_source = (PLUGIN_ROOT / "utils" / "update_manager.gd").read_text()
+    dock_source = (PLUGIN_ROOT / "mcp_dock.gd").read_text(encoding="utf-8")
+    manager_source = (PLUGIN_ROOT / "utils" / "update_manager.gd").read_text(encoding="utf-8")
 
     # `_exit_tree` (dock teardown) must drain inline; the install-time
     # drain runs through `McpUpdateManager._drain_dock_workers()` which
@@ -141,7 +141,7 @@ def test_dock_drains_action_workers_during_install_update_and_exit_tree() -> Non
 def test_dock_action_dispatch_gates_on_self_update_in_progress() -> None:
     """The same gate the refresh worker honors must protect Configure / Remove."""
 
-    dock_source = (PLUGIN_ROOT / "mcp_dock.gd").read_text()
+    dock_source = (PLUGIN_ROOT / "mcp_dock.gd").read_text(encoding="utf-8")
     block = get_func_block(dock_source, "func _dispatch_client_action(")
     assert "_is_self_update_in_progress" in block, (
         "Configure / Remove dispatch must short-circuit during the "
@@ -155,7 +155,7 @@ def test_dock_action_dispatch_gates_on_self_update_in_progress() -> None:
 def test_status_refresh_apply_skips_rows_with_in_flight_action() -> None:
     """A concurrent refresh result must not stomp the 'Configuring…' badge."""
 
-    dock_source = (PLUGIN_ROOT / "mcp_dock.gd").read_text()
+    dock_source = (PLUGIN_ROOT / "mcp_dock.gd").read_text(encoding="utf-8")
     apply_block = get_func_block(dock_source, "func _apply_client_status_refresh_results(")
     assert "_client_action_threads.has(" in apply_block, (
         "Refresh-result apply must skip rows whose action worker is "