Skip to content

Commit ca05df4

Browse files
authored
Consolidate whole-file update_lib migration guards (#9578)
* Prevent copy-lib from replacing Python test trees Preflight direct and expanded copy destinations, rejecting Python test paths before any replacement. Reject traversal components after the Lib path marker while retaining exact data-file copies. Assisted-by: OpenAI Codex:model version not exposed * Consolidate explicit marker retirement in update_lib Move the generic skip-retirement capability out of the Unicode runtime change so whole-file migration tooling has one review location. Keep fail-closed decorator matching and remaining-marker preservation. Assisted-by: OpenAI Codex:model version not exposed
1 parent a2b2e98 commit ca05df4

4 files changed

Lines changed: 81 additions & 9 deletions

File tree

‎scripts/update_lib/cmd_copy_lib.py‎

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,14 @@ def _copy_single(
5555
shutil.copy2(src_path, lib_path)
5656

5757

58+
def _reject_python_tests(src_path: pathlib.Path, relative: pathlib.Path) -> None:
59+
is_test_tree = len(relative.parts) >= 2 and relative.parts[1].casefold() == "test"
60+
if is_test_tree and (src_path.is_dir() or src_path.suffix.casefold() == ".py"):
61+
raise ValueError(
62+
"Use migrate for Python tests; copy-lib would replace Lib/test"
63+
)
64+
65+
5866
def copy_lib(
5967
src_path: pathlib.Path,
6068
verbose: bool = True,
@@ -77,11 +85,13 @@ def copy_lib(
7785
raise ValueError(f"Path must contain '/Lib/' (got: {src_path})")
7886
if not src_path.exists():
7987
raise FileNotFoundError(f"Source path does not exist: {src_path}")
88+
relative = parse_lib_path(src_path)
89+
_reject_python_tests(src_path, relative)
8090

8191
# Data files name an exact asset, not the containing top-level module.
8292
# Expanding Lib/test/data.txt as "test" would replace the entire test tree.
8393
if src_path.is_file() and src_path.suffix != ".py":
84-
_copy_single(src_path, parse_lib_path(src_path), verbose)
94+
_copy_single(src_path, relative, verbose)
8595
return
8696

8797
cpython_prefix, after_lib = path_str.split("/Lib/", 1)
@@ -93,11 +103,12 @@ def copy_lib(
93103
# Get all paths to copy from DEPENDENCIES table
94104
all_src_paths = get_lib_paths(name, cpython_prefix)
95105

96-
# Copy each file
97-
for src in all_src_paths:
98-
if src.exists():
99-
lib_path = parse_lib_path(src)
100-
_copy_single(src, lib_path, verbose)
106+
# Validate the entire expansion before removing any destination.
107+
plan = [(src, parse_lib_path(src)) for src in all_src_paths if src.exists()]
108+
for src, lib_path in plan:
109+
_reject_python_tests(src, lib_path)
110+
for src, lib_path in plan:
111+
_copy_single(src, lib_path, verbose)
101112

102113

103114
def main(argv: list[str] | None = None) -> int:

‎scripts/update_lib/file_utils.py‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,10 @@ def parse_lib_path(path: pathlib.Path | str) -> pathlib.Path:
7373
raise ValueError(f"Path must contain '/Lib/' or '\\Lib\\' (got: {path})")
7474

7575
idx = path_str.index(lib_marker)
76-
return pathlib.Path(path_str[idx + 1 :])
76+
relative = pathlib.Path(path_str[idx + 1 :])
77+
if ".." in relative.parts:
78+
raise ValueError(f"Path must not escape or traverse within Lib/ (got: {path})")
79+
return relative
7780

7881

7982
def is_lib_path(path: pathlib.Path) -> bool:

‎scripts/update_lib/patch_spec.py‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -596,9 +596,12 @@ def _conditional_retirements(document, old_content):
596596
"Original conditional decorator does not match exactly once"
597597
)
598598
marker = PatchSpec.try_from_ast_node(matches[0], old_content.splitlines())
599-
if marker is None or marker.ut_method != UtMethod.ExpectedFailureIf:
599+
if marker is None or marker.ut_method not in (
600+
UtMethod.ExpectedFailureIf,
601+
UtMethod.Skip,
602+
):
600603
raise ValueError(
601-
"Only explicit expectedFailureIf retirements are supported"
604+
"Only explicit expectedFailureIf and skip retirements are supported"
602605
)
603606
result[key] = marker
604607
return result

‎scripts/update_lib/tests/test_migrate.py‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -884,6 +884,61 @@ def test_other_guards_and_original_source_required(self):
884884
with self.assertRaises(ValueError):
885885
self.select(None, current, document)
886886

887+
def test_explicit_skip_retirement_preserves_other_markers(self):
888+
decorator = ' @unittest.skip("TODO: RUSTPYTHON; known limitation")\n'
889+
old = OVERRIDE_OLD.replace(
890+
" @unittest.expectedFailure # TODO: RUSTPYTHON; old failure\n",
891+
decorator,
892+
)
893+
894+
def authorize(document):
895+
document["retire_conditional"] = [
896+
{
897+
"class": "Example",
898+
"method": "test_known",
899+
"decorator": decorator,
900+
}
901+
]
902+
903+
with patch(__name__ + ".OVERRIDE_OLD", old):
904+
status, result = MarkerOverrideTests().invoke(authorize)
905+
self.assertEqual(status, 0)
906+
self.assertNotIn("known limitation", result)
907+
self.assertIn('unittest.skip("TODO: RUSTPYTHON; retained guard")', result)
908+
status, result = MarkerOverrideTests().invoke()
909+
self.assertEqual(status, 1)
910+
self.assertEqual(result, old)
911+
912+
def test_skip_retirement_fails_closed(self):
913+
self.decorator = ' @unittest.skip("TODO: RUSTPYTHON; known limitation")\n'
914+
for change in (
915+
"text",
916+
"duplicate",
917+
"retained",
918+
"duplicate_source",
919+
"skipIf",
920+
"skipUnless",
921+
):
922+
old, current, document = self.fixture()
923+
if change == "text":
924+
document["retire_conditional"][0]["decorator"] = self.decorator.replace(
925+
'"', "'"
926+
)
927+
elif change == "duplicate":
928+
document["retire_conditional"] *= 2
929+
elif change == "retained":
930+
document["patches"] = patches_to_json(current)
931+
elif change == "duplicate_source":
932+
old = old.replace(self.decorator, self.decorator * 2)
933+
current = extract_patches(old)
934+
else:
935+
replacement = self.decorator.replace("skip(", change + "(True, ")
936+
old = old.replace(self.decorator, replacement)
937+
current = extract_patches(old)
938+
document["retire_conditional"][0]["decorator"] = replacement
939+
with self.subTest(change=change), self.assertRaises(ValueError):
940+
self.select(old, current, document)
941+
887942
def test_cli_conditional_retirement_preserves_remaining_markers(self):
888943
old = OVERRIDE_OLD.replace(
889944
" @unittest.expectedFailure # TODO: RUSTPYTHON; old failure\n",

0 commit comments

Comments
 (0)