From b8a9f18b92acd98feaba16afa8f30ddb81757e3c Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Tue, 25 Aug 2026 13:23:04 -0600 Subject: [PATCH 01/21] rimport: Anchor relative CLI-arg paths to cwd inside the inputdata tree Fixes the headline bug: rimport always resolved relative --file / positional filenames against the inputdata root, ignoring cwd. Running `rimport test.nc` from an inputdata subdirectory would silently stage a same-named file from the root instead (or fail to find the file that's actually there). get_files_to_process() now takes a required inputdata_root parameter and eagerly anchors non-absolute file/items_to_process names to the resolved cwd whenever cwd is inside the inputdata tree (the root itself counts), with no fallback to the root on a miss. When cwd is outside the tree, names are left unchanged for normalize_paths to resolve against the root, exactly as before. normalize_paths and --list entry handling are untouched (list-relative anchoring is a separate task). The 4 existing unit tests exercising relative CLI-arg names now monkeypatch.chdir() to a directory genuinely outside their tmp_path-based inputdata root, since "cwd outside the tree" is now the condition their existing expectations depend on. Co-Authored-By: Claude Opus 5 (1M context) --- rimport | 32 +++- tests/rimport/test_cmdline.py | 182 +++++++++++++++++++++ tests/rimport/test_get_files_to_process.py | 179 +++++++++++++++++++- 3 files changed, 383 insertions(+), 10 deletions(-) diff --git a/rimport b/rimport index 1024907..266591f 100755 --- a/rimport +++ b/rimport @@ -331,22 +331,44 @@ def print_can_file_be_downloaded(file_can_be_downloaded: bool): logger.info("%sFile is not (yet) available for download.", INDENT) -def get_files_to_process(file: str, filelist: str, items_to_process: list): +def get_files_to_process( + file: str, filelist: str, items_to_process: list, inputdata_root: Path +): """Get list of files to process. Uses --file and/or --filelist arguments, as well as positional items_to_process if given. + Non-absolute `file` and `items_to_process` entries are CLI args: if cwd is inside the + inputdata tree (the root itself counts), they are anchored (eagerly, absolute) to cwd, + with no fallback to the root. If cwd is outside the tree, they are left unchanged for + normalize_paths to later resolve against `inputdata_root`, as before. `--list` entries are + untouched here; they keep resolving against the root (list-relative anchoring is handled + elsewhere). + Args: file (str): Single file to process. filelist (str): File containing list of files to process. items_to_process (list): List of files to process. + inputdata_root (Path): Root of the inputdata tree, used to decide whether cwd is inside + it and, if so, to anchor relative CLI-arg names against cwd. Returns: list: List of files to process int: Result code """ + root_resolved = Path(inputdata_root).expanduser().resolve() + cwd = Path.cwd().resolve() + cwd_inside = cwd.is_relative_to(root_resolved) + + def _anchor_cli(name): + if Path(name).is_absolute(): + return name + if cwd_inside: + return str(cwd / name) # strict: NO fallback to root + return name # legacy: normalize_paths joins onto root later + if file is not None: - files_to_process = [file] + files_to_process = [_anchor_cli(file)] else: files_to_process = [] @@ -362,7 +384,7 @@ def get_files_to_process(file: str, filelist: str, items_to_process: list): files_to_process.extend(files_in_list) if items_to_process: - files_to_process.extend(items_to_process) + files_to_process.extend(_anchor_cli(item) for item in items_to_process) if not files_to_process: logger.error("rimport: At least one of --file or --filelist is required") @@ -412,7 +434,9 @@ def main(argv: List[str] | None = None) -> int: return 2 # Determine the list of relative filenames to handle - files_to_process, status = get_files_to_process(args.file, args.filelist, args.items_to_process) + files_to_process, status = get_files_to_process( + args.file, args.filelist, args.items_to_process, root + ) if status: return status diff --git a/tests/rimport/test_cmdline.py b/tests/rimport/test_cmdline.py index be05c5d..ef9a236 100644 --- a/tests/rimport/test_cmdline.py +++ b/tests/rimport/test_cmdline.py @@ -506,6 +506,188 @@ def test_check_doesnt_copy_unpublished(self, rimport_script, test_env, rimport_e assert "Created symbolic link".lower() not in result.stdout.lower() assert "Error creating symlink".lower() not in result.stdout.lower() + def test_relative_file_from_inputdata_subdir_stages_that_file( + self, rimport_script, test_env, rimport_env + ): + """Test that a relative positional filename anchors to cwd when rimport is run from + inside an inputdata subdirectory, staging the file that is actually there rather than + a same-named decoy file at the inputdata root.""" + inputdata_root = test_env["inputdata_root"] + staging_root = test_env["staging_root"] + + subdir = inputdata_root / "lnd" / "clm2" + subdir.mkdir(parents=True) + + subdir_file = subdir / "test.nc" + subdir_file.write_text("subdir data") + + decoy_file = inputdata_root / "test.nc" + decoy_file.write_text("decoy data") + + # Run rimport with a relative positional filename, from inside the subdir + command = [ + sys.executable, + rimport_script, + "test.nc", + "-inputdata", + str(inputdata_root), + ] + + result = subprocess.run( + command, + capture_output=True, + text=True, + check=False, + env=rimport_env, + cwd=subdir, + ) + + # Verify success + assert result.returncode == 0, f"Command failed: {result.stderr}" + + # Verify the subdir file (not the decoy) was staged + staged_file = staging_root / "lnd" / "clm2" / "test.nc" + assert staged_file.exists() + assert staged_file.read_text() == "subdir data" + + # Verify the subdir file was relinked + assert subdir_file.is_symlink() + assert subdir_file.resolve() == staged_file + + # Verify the decoy at the inputdata root was left untouched + assert not decoy_file.is_symlink() + assert decoy_file.read_text() == "decoy data" + + # Verify nothing was staged at the root-anchored path + assert not (staging_root / "test.nc").exists() + + def test_relative_file_from_subdir_missing_errors_no_root_fallback( + self, rimport_script, test_env, rimport_env + ): + """Test that a relative positional filename run from an inputdata subdirectory errors + when the file isn't there, rather than falling back to a same-named file at the root.""" + inputdata_root = test_env["inputdata_root"] + staging_root = test_env["staging_root"] + + subdir = inputdata_root / "lnd" / "clm2" + subdir.mkdir(parents=True) + + root_file = inputdata_root / "test.nc" + root_file.write_text("root data") + + # Run rimport with a relative positional filename, from inside the subdir + command = [ + sys.executable, + rimport_script, + "test.nc", + "-inputdata", + str(inputdata_root), + ] + + result = subprocess.run( + command, + capture_output=True, + text=True, + check=False, + env=rimport_env, + cwd=subdir, + ) + + # Verify failure + assert result.returncode != 0, f"Command unexpectedly passed: {result.stdout}" + assert "source not found" in result.stderr + + # Verify the root file was left untouched + assert not root_file.is_symlink() + assert root_file.read_text() == "root data" + + # Verify nothing was staged + assert not any(staging_root.iterdir()) + + def test_relative_file_from_outside_tree_resolves_against_root( + self, rimport_script, test_env, rimport_env + ): + """Test that a relative positional filename still resolves against the inputdata root, + as before, when rimport is run from outside the inputdata tree.""" + inputdata_root = test_env["inputdata_root"] + staging_root = test_env["staging_root"] + tmp_path = test_env["tmp_path"] + + test_file = inputdata_root / "test.nc" + test_file.write_text("root data") + + # Run rimport with a relative positional filename, from outside the tree + command = [ + sys.executable, + rimport_script, + "test.nc", + "-inputdata", + str(inputdata_root), + ] + + result = subprocess.run( + command, + capture_output=True, + text=True, + check=False, + env=rimport_env, + cwd=tmp_path, + ) + + # Verify success + assert result.returncode == 0, f"Command failed: {result.stderr}" + + # Verify the file was staged, resolved against the inputdata root + staged_file = staging_root / "test.nc" + assert staged_file.exists() + assert staged_file.read_text() == "root data" + + # Verify file was relinked + assert test_file.is_symlink() + assert test_file.resolve() == staged_file + + def test_dotdot_escape_from_subdir_errors( + self, rimport_script, test_env, rimport_env + ): + """Test that a '..'-escaping relative filename, anchored lexically to cwd, is rejected + by stage_data's existing outside-the-root guardrail.""" + inputdata_root = test_env["inputdata_root"] + tmp_path = test_env["tmp_path"] + + subdir = inputdata_root / "lnd" + subdir.mkdir(parents=True) + + # File must exist, or the "source not found" check fires before the guardrail and the + # "not under inputdata root" message never appears. + outside_file = tmp_path / "outside.nc" + outside_file.write_text("outside data") + + # Run rimport with an escaping relative positional filename, from inside the subdir + command = [ + sys.executable, + rimport_script, + "../../outside.nc", + "-inputdata", + str(inputdata_root), + ] + + result = subprocess.run( + command, + capture_output=True, + text=True, + check=False, + env=rimport_env, + cwd=subdir, + ) + + # Verify failure + assert result.returncode != 0, f"Command unexpectedly passed: {result.stdout}" + assert "not under inputdata root" in result.stderr + + # Verify the outside file was left untouched + assert not outside_file.is_symlink() + assert outside_file.read_text() == "outside data" + def test_check_doesnt_relink_published(self, rimport_script, test_env, rimport_env): """Test that published file is not relinked if check is True.""" inputdata_root = test_env["inputdata_root"] diff --git a/tests/rimport/test_get_files_to_process.py b/tests/rimport/test_get_files_to_process.py index 6fc13b3..a101c03 100644 --- a/tests/rimport/test_get_files_to_process.py +++ b/tests/rimport/test_get_files_to_process.py @@ -5,6 +5,7 @@ import os import importlib.util from importlib.machinery import SourceFileLoader +from pathlib import Path # pylint: disable=too-many-arguments,too-many-positional-arguments @@ -25,7 +26,7 @@ class TestGetRelnamesToProcess: """Test suite for get_relnames_to_process() function.""" - def test_single_file_relpath(self, tmp_path): + def test_single_file_relpath(self, tmp_path, monkeypatch): """Test giving it a single file by its relative path""" # Setup inputdata_root = tmp_path / "inputdata" @@ -37,11 +38,15 @@ def test_single_file_relpath(self, tmp_path): test_file = inputdata_root / filename test_file.write_text("abc123") + # cwd outside the inputdata tree: relative name stays unanchored + monkeypatch.chdir(tmp_path) + # Run files_to_process, result = rimport.get_files_to_process( file=filename, filelist=None, items_to_process=None, + inputdata_root=inputdata_root, ) # Verify @@ -65,6 +70,7 @@ def test_single_file_abspath(self, tmp_path): file=test_file, filelist=None, items_to_process=None, + inputdata_root=inputdata_root, ) # Verify @@ -94,6 +100,7 @@ def test_filelist_relpath_with_relpaths(self, tmp_path): file=None, filelist=filelist_relpath, items_to_process=None, + inputdata_root=inputdata_root, ) # Verify @@ -122,6 +129,7 @@ def test_filelist_abspath_with_relpaths(self, tmp_path): file=None, filelist=filelist, items_to_process=None, + inputdata_root=inputdata_root, ) # Verify @@ -151,6 +159,7 @@ def test_filelist_relpath_with_abspaths(self, tmp_path): file=None, filelist=filelist_relpath, items_to_process=None, + inputdata_root=inputdata_root, ) # Verify @@ -179,32 +188,41 @@ def test_filelist_abspath_with_abspaths(self, tmp_path): file=None, filelist=filelist, items_to_process=None, + inputdata_root=inputdata_root, ) # Verify assert result == 0 assert files_to_process == filenames - def test_filelist_not_found(self): + def test_filelist_not_found(self, tmp_path): """Test giving it a file list that doesn't exist""" + inputdata_root = tmp_path / "inputdata" + inputdata_root.mkdir() + filelist = "bsfearirn" assert not os.path.exists(filelist) files_to_process, result = rimport.get_files_to_process( file=None, filelist=filelist, items_to_process=None, + inputdata_root=inputdata_root, ) assert result == 2 assert files_to_process is None def test_filelist_empty(self, tmp_path): """Test giving it an empty file list""" + inputdata_root = tmp_path / "inputdata" + inputdata_root.mkdir() + filelist = tmp_path / "bsfearirn" filelist.write_text("") files_to_process, result = rimport.get_files_to_process( file=None, filelist=filelist, items_to_process=[], + inputdata_root=inputdata_root, ) assert result == 2 assert files_to_process is None @@ -228,13 +246,14 @@ def test_items_to_process_abspaths(self, tmp_path): file=None, filelist=None, items_to_process=filenames, + inputdata_root=inputdata_root, ) # Verify assert result == 0 assert files_to_process == filenames - def test_items_to_process_relpaths(self, tmp_path): + def test_items_to_process_relpaths(self, tmp_path, monkeypatch): """Test giving it a list of relative paths in items_to_process""" # Setup inputdata_root = tmp_path / "inputdata" @@ -246,18 +265,22 @@ def test_items_to_process_relpaths(self, tmp_path): filenames.append(os.path.basename(filename)) filename.write_text("def567") + # cwd outside the inputdata tree: relative names stay unanchored + monkeypatch.chdir(tmp_path) + # Run files_to_process, result = rimport.get_files_to_process( file=None, filelist=None, items_to_process=filenames, + inputdata_root=inputdata_root, ) # Verify assert result == 0 assert files_to_process == filenames - def test_items_to_process_mixpaths(self, tmp_path): + def test_items_to_process_mixpaths(self, tmp_path, monkeypatch): """Test giving it a list of absolute and relative paths in items_to_process""" # Setup inputdata_root = tmp_path / "inputdata" @@ -274,18 +297,22 @@ def test_items_to_process_mixpaths(self, tmp_path): filename.write_text("def567") assert len(filenames) == 4 + # cwd outside the inputdata tree: relative names stay unanchored + monkeypatch.chdir(tmp_path) + # Run files_to_process, result = rimport.get_files_to_process( file=None, filelist=None, items_to_process=filenames, + inputdata_root=inputdata_root, ) # Verify assert result == 0 assert files_to_process == filenames - def test_single_file_and_list(self, tmp_path): + def test_single_file_and_list(self, tmp_path, monkeypatch): """Test giving it a single file by its relative path""" # Setup inputdata_root = tmp_path / "inputdata" @@ -306,26 +333,166 @@ def test_single_file_and_list(self, tmp_path): filelist = tmp_path / "file_list.txt" filelist.write_text("\n".join(filenames), encoding="utf8") + # cwd outside the inputdata tree: relative `file` name stays unanchored + monkeypatch.chdir(tmp_path) + # Run files_to_process, result = rimport.get_files_to_process( file=filename, filelist=filelist, items_to_process=None, + inputdata_root=inputdata_root, ) # Verify assert result == 0 assert files_to_process == [filename] + filenames - def test_single_or_filelist_or_list_required(self): + def test_single_or_filelist_or_list_required(self, tmp_path): """Test that at least one of file, filelist, items_to_process is required""" + inputdata_root = tmp_path / "inputdata" + inputdata_root.mkdir() + # Run files_to_process, result = rimport.get_files_to_process( file=None, filelist=None, items_to_process=None, + inputdata_root=inputdata_root, ) # Verify assert result == 2 assert files_to_process is None + + def test_cli_file_relative_cwd_inside_tree(self, tmp_path, monkeypatch): + """Test that a relative --file name anchors to cwd when cwd is inside the tree""" + inputdata_root = tmp_path / "inputdata" + subdir = inputdata_root / "sub" + subdir.mkdir(parents=True) + monkeypatch.chdir(subdir) + cwd = Path.cwd().resolve() + + filename = "test.nc" + + # Run + files_to_process, result = rimport.get_files_to_process( + file=filename, + filelist=None, + items_to_process=None, + inputdata_root=inputdata_root, + ) + + # Verify + assert result == 0 + assert files_to_process == [str(cwd / filename)] + + def test_cli_items_relative_cwd_inside_tree(self, tmp_path, monkeypatch): + """Test that relative items_to_process names anchor to cwd when cwd is inside the tree""" + inputdata_root = tmp_path / "inputdata" + subdir = inputdata_root / "sub" + subdir.mkdir(parents=True) + monkeypatch.chdir(subdir) + cwd = Path.cwd().resolve() + + filenames = ["test0.txt", "test1.txt"] + + # Run + files_to_process, result = rimport.get_files_to_process( + file=None, + filelist=None, + items_to_process=filenames, + inputdata_root=inputdata_root, + ) + + # Verify + assert result == 0 + assert files_to_process == [str(cwd / f) for f in filenames] + + def test_cli_relative_cwd_outside_tree_unchanged(self, tmp_path, monkeypatch): + """Test that a relative --file name is left unchanged when cwd is outside the tree""" + inputdata_root = tmp_path / "inputdata" + inputdata_root.mkdir() + outside = tmp_path / "outside" + outside.mkdir() + monkeypatch.chdir(outside) + + filename = "test.nc" + + # Run + files_to_process, result = rimport.get_files_to_process( + file=filename, + filelist=None, + items_to_process=None, + inputdata_root=inputdata_root, + ) + + # Verify + assert result == 0 + assert files_to_process == [filename] + + def test_cli_cwd_equals_root_anchors_to_root(self, tmp_path, monkeypatch): + """Test that cwd == inputdata root counts as inside the tree""" + inputdata_root = tmp_path / "inputdata" + inputdata_root.mkdir() + monkeypatch.chdir(inputdata_root) + cwd = Path.cwd().resolve() + + filename = "test.nc" + + # Run + files_to_process, result = rimport.get_files_to_process( + file=filename, + filelist=None, + items_to_process=None, + inputdata_root=inputdata_root, + ) + + # Verify + assert result == 0 + assert files_to_process == [str(cwd / filename)] + + def test_cli_absolute_unchanged_cwd_inside_tree(self, tmp_path, monkeypatch): + """Test that an absolute --file name is left unchanged even when cwd is inside the tree""" + inputdata_root = tmp_path / "inputdata" + subdir = inputdata_root / "sub" + subdir.mkdir(parents=True) + monkeypatch.chdir(subdir) + + abs_file = str(inputdata_root / "other" / "test.nc") + + # Run + files_to_process, result = rimport.get_files_to_process( + file=abs_file, + filelist=None, + items_to_process=None, + inputdata_root=inputdata_root, + ) + + # Verify + assert result == 0 + assert files_to_process == [abs_file] + + def test_cli_cwd_inside_tree_via_symlink(self, tmp_path, monkeypatch): + """Test that a relative name anchors to the real (resolved) cwd when cwd was reached + through a symlink into the tree.""" + inputdata_root = tmp_path / "inputdata" + real_sub = inputdata_root / "sub" + real_sub.mkdir(parents=True) + link = tmp_path / "link" + link.symlink_to(real_sub) + monkeypatch.chdir(link) + + filename = "test.nc" + + # Run + files_to_process, result = rimport.get_files_to_process( + file=filename, + filelist=None, + items_to_process=None, + inputdata_root=inputdata_root, + ) + + # Verify + assert result == 0 + assert files_to_process == [str(real_sub.resolve() / filename)] From f9b5faef4bc548a1d7d8b63a1381d926c30db11a Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Tue, 25 Aug 2026 13:35:52 -0600 Subject: [PATCH 02/21] rimport: Anchor --list relative entries to the list file's own directory Per-entry anchoring for --list (--filelist) entries: a relative entry resolves against the list file's own resolved directory (root itself counts as inside the tree), not the cwd or unconditionally the inputdata root. A relative entry in a list file whose directory is outside the inputdata tree is now a fatal error (rc 2), naming both the offending entry and the list file. This is an intentional breaking change: filelists outside the tree with relative entries previously resolved against the root and now error. Updated the tests that encoded the old contract accordingly. Co-Authored-By: Claude Opus 5 (1M context) --- rimport | 27 ++++- tests/rimport/test_cmdline.py | 97 ++++++++++++++++- tests/rimport/test_get_files_to_process.py | 119 +++++++++++++++++++-- tests/rimport/test_main.py | 8 +- 4 files changed, 233 insertions(+), 18 deletions(-) diff --git a/rimport b/rimport index 266591f..bb46acd 100755 --- a/rimport +++ b/rimport @@ -341,9 +341,13 @@ def get_files_to_process( Non-absolute `file` and `items_to_process` entries are CLI args: if cwd is inside the inputdata tree (the root itself counts), they are anchored (eagerly, absolute) to cwd, with no fallback to the root. If cwd is outside the tree, they are left unchanged for - normalize_paths to later resolve against `inputdata_root`, as before. `--list` entries are - untouched here; they keep resolving against the root (list-relative anchoring is handled - elsewhere). + normalize_paths to later resolve against `inputdata_root`, as before. + + Non-absolute `--list` entries are anchored against the list file's own directory (the root + itself counts as inside), not the cwd: if that directory is inside the inputdata tree, a + relative entry is resolved (eagerly, absolute) against it; if the list file's directory is + outside the tree, a relative entry is a fatal error (relative paths are not allowed in list + files outside the inputdata tree). Args: file (str): Single file to process. @@ -381,7 +385,22 @@ def get_files_to_process( if not files_in_list: logger.error("rimport: no filenames found in list: %s", list_path) return None, 2 - files_to_process.extend(files_in_list) + + list_base = list_path.parent + list_inside = list_base.is_relative_to(root_resolved) + for entry in files_in_list: + if Path(entry).is_absolute(): + files_to_process.append(entry) + elif list_inside: + files_to_process.append(str(list_base / entry)) + else: + logger.error( + "rimport: relative path '%s' not allowed in list file outside " + "the inputdata tree: %s", + entry, + list_path, + ) + return None, 2 if items_to_process: files_to_process.extend(_anchor_cli(item) for item in items_to_process) diff --git a/tests/rimport/test_cmdline.py b/tests/rimport/test_cmdline.py index ef9a236..940ea3e 100644 --- a/tests/rimport/test_cmdline.py +++ b/tests/rimport/test_cmdline.py @@ -100,9 +100,9 @@ def test_list_option_stages_multiple_files( file1.write_text("data1") file2.write_text("data2") - # Create filelist + # Create filelist (outside the tree: entries must be absolute) filelist = tmp_path / "filelist.txt" - filelist.write_text("file1.nc\nfile2.nc\n") + filelist.write_text(f"{file1}\n{file2}\n") # Run rimport with -list option command = [ @@ -137,6 +137,95 @@ def test_list_option_stages_multiple_files( assert file2.is_symlink() assert file2.resolve() == (staging_root / "file2.nc") + def test_list_inside_tree_relative_entries( + self, rimport_script, test_env, rimport_env + ): + """Test that a list file inside the inputdata tree anchors relative entries to the + list file's own directory, not the inputdata root.""" + inputdata_root = test_env["inputdata_root"] + staging_root = test_env["staging_root"] + + # Create nested file and a list file alongside it inside the tree + nested_file = inputdata_root / "lnd" / "clm2" / "file1.nc" + nested_file.parent.mkdir(parents=True) + nested_file.write_text("nested data") + + filelist = inputdata_root / "lnd" / "filelist.txt" + filelist.write_text("clm2/file1.nc\n") + + # Run rimport with -list option + command = [ + sys.executable, + rimport_script, + "-list", + str(filelist), + "-inputdata", + str(inputdata_root), + ] + + result = subprocess.run( + command, + capture_output=True, + text=True, + check=False, + env=rimport_env, + ) + + # Verify success + assert result.returncode == 0, f"Command failed: {result.stderr}" + + # Verify the file was staged, anchored to the list dir's own subtree + staged_file = staging_root / "lnd" / "clm2" / "file1.nc" + assert staged_file.exists() + assert staged_file.read_text() == "nested data" + + # Verify file was relinked + assert nested_file.is_symlink() + assert nested_file.resolve() == staged_file + + def test_list_outside_tree_relative_entry_error( + self, rimport_script, test_env, rimport_env + ): + """Test that a relative entry in a list file outside the tree is a fatal error.""" + inputdata_root = test_env["inputdata_root"] + staging_root = test_env["staging_root"] + tmp_path = test_env["tmp_path"] + + # Create the file that would be staged if this succeeded + test_file = inputdata_root / "file1.nc" + test_file.write_text("data1") + + # Create filelist OUTSIDE the tree with a relative entry + filelist = tmp_path / "filelist.txt" + filelist.write_text("file1.nc\n") + + # Run rimport with -list option + command = [ + sys.executable, + rimport_script, + "-list", + str(filelist), + "-inputdata", + str(inputdata_root), + ] + + result = subprocess.run( + command, + capture_output=True, + text=True, + check=False, + env=rimport_env, + ) + + # Verify fatal error, naming both the offending entry and the list file + assert result.returncode == 2 + assert "file1.nc" in result.stderr + assert str(filelist.resolve()) in result.stderr + + # Verify nothing was staged or symlinked + assert not (staging_root / "file1.nc").exists() + assert not test_file.is_symlink() + def test_preserves_directory_structure(self, rimport_script, test_env, rimport_env): """Test that directory structure is preserved in staging.""" inputdata_root = test_env["inputdata_root"] @@ -293,9 +382,9 @@ def test_list_with_comments_and_blanks(self, rimport_script, test_env, rimport_e file1.write_text("data1") file2.write_text("data2") - # Create filelist with comments and blanks + # Create filelist with comments and blanks (outside the tree: entries must be absolute) filelist = tmp_path / "filelist.txt" - filelist.write_text("# Comment\nfile1.nc\n\n# Another comment\nfile2.nc\n") + filelist.write_text(f"# Comment\n{file1}\n\n# Another comment\n{file2}\n") # Run rimport command = [ diff --git a/tests/rimport/test_get_files_to_process.py b/tests/rimport/test_get_files_to_process.py index a101c03..f0e55bd 100644 --- a/tests/rimport/test_get_files_to_process.py +++ b/tests/rimport/test_get_files_to_process.py @@ -78,7 +78,8 @@ def test_single_file_abspath(self, tmp_path): assert files_to_process == [test_file] def test_filelist_relpath_with_relpaths(self, tmp_path): - """Test giving it a file list by its relative path, containing relative paths""" + """Test giving it a file list (outside tree) by its relative path, containing relative + paths: fatal error, since the list file is outside the inputdata tree""" # Setup inputdata_root = tmp_path / "inputdata" inputdata_root.mkdir() @@ -104,11 +105,12 @@ def test_filelist_relpath_with_relpaths(self, tmp_path): ) # Verify - assert result == 0 - assert files_to_process == filenames + assert result == 2 + assert files_to_process is None def test_filelist_abspath_with_relpaths(self, tmp_path): - """Test giving it a file list by its absolute path, containing relative paths""" + """Test giving it a file list (outside tree) by its absolute path, containing relative + paths: fatal error, since the list file is outside the inputdata tree""" # Setup inputdata_root = tmp_path / "inputdata" inputdata_root.mkdir() @@ -133,8 +135,8 @@ def test_filelist_abspath_with_relpaths(self, tmp_path): ) # Verify - assert result == 0 - assert files_to_process == filenames + assert result == 2 + assert files_to_process is None def test_filelist_relpath_with_abspaths(self, tmp_path): """Test giving it a file list by its relative path, containing absolute paths""" @@ -195,6 +197,105 @@ def test_filelist_abspath_with_abspaths(self, tmp_path): assert result == 0 assert files_to_process == filenames + def test_list_inside_tree_relative_entries_anchored_to_list_dir(self, tmp_path): + """Test that relative entries in a list file inside the tree anchor to the list file's + own directory, not the inputdata root""" + # Setup + inputdata_root = tmp_path / "inputdata" + list_dir = inputdata_root / "lnd" + list_dir.mkdir(parents=True) + + filenames = ["clm2/file1.nc", "file2.nc"] + filelist = list_dir / "filelist.txt" + filelist.write_text("\n".join(filenames), encoding="utf8") + + # Run + files_to_process, result = rimport.get_files_to_process( + file=None, + filelist=filelist, + items_to_process=None, + inputdata_root=inputdata_root, + ) + + # Verify + assert result == 0 + list_dir_resolved = list_dir.resolve() + assert files_to_process == [str(list_dir_resolved / f) for f in filenames] + + def test_list_at_root_relative_entries_anchored_to_root(self, tmp_path): + """Test that a list file located at the inputdata root itself (root counts as inside the + tree) anchors relative entries to the root""" + # Setup + inputdata_root = tmp_path / "inputdata" + inputdata_root.mkdir() + + filenames = ["test0.txt", "test1.txt"] + filelist = inputdata_root / "filelist.txt" + filelist.write_text("\n".join(filenames), encoding="utf8") + + # Run + files_to_process, result = rimport.get_files_to_process( + file=None, + filelist=filelist, + items_to_process=None, + inputdata_root=inputdata_root, + ) + + # Verify + assert result == 0 + root_resolved = inputdata_root.resolve() + assert files_to_process == [str(root_resolved / f) for f in filenames] + + def test_list_outside_tree_relative_entry_errors(self, tmp_path, caplog): + """Test that a relative entry in a list file outside the tree is a fatal error""" + # Setup + inputdata_root = tmp_path / "inputdata" + inputdata_root.mkdir() + + filelist = tmp_path / "filelist.txt" + filelist.write_text("relative_file.nc\n", encoding="utf8") + + # Run + files_to_process, result = rimport.get_files_to_process( + file=None, + filelist=filelist, + items_to_process=None, + inputdata_root=inputdata_root, + ) + + # Verify + assert result == 2 + assert files_to_process is None + assert "relative_file.nc" in caplog.text + assert str(filelist.resolve()) in caplog.text + + def test_list_outside_tree_absolute_entries_ok(self, tmp_path): + """Test that a list file outside the tree still works when all entries are absolute""" + # Setup + inputdata_root = tmp_path / "inputdata" + inputdata_root.mkdir() + + filenames = [] + for i in range(2): + filename = inputdata_root / f"test{i}.txt" + filenames.append(str(filename)) + filename.write_text("def567") + + filelist = tmp_path / "file_list.txt" + filelist.write_text("\n".join(filenames), encoding="utf8") + + # Run + files_to_process, result = rimport.get_files_to_process( + file=None, + filelist=filelist, + items_to_process=None, + inputdata_root=inputdata_root, + ) + + # Verify + assert result == 0 + assert files_to_process == filenames + def test_filelist_not_found(self, tmp_path): """Test giving it a file list that doesn't exist""" inputdata_root = tmp_path / "inputdata" @@ -330,7 +431,7 @@ def test_single_file_and_list(self, tmp_path, monkeypatch): filenames.append(f) (inputdata_root / f).write_text("def567") - filelist = tmp_path / "file_list.txt" + filelist = inputdata_root / "file_list.txt" filelist.write_text("\n".join(filenames), encoding="utf8") # cwd outside the inputdata tree: relative `file` name stays unanchored @@ -346,7 +447,9 @@ def test_single_file_and_list(self, tmp_path, monkeypatch): # Verify assert result == 0 - assert files_to_process == [filename] + filenames + assert files_to_process == [filename] + [ + str(inputdata_root.resolve() / f) for f in filenames + ] def test_single_or_filelist_or_list_required(self, tmp_path): """Test that at least one of file, filelist, items_to_process is required""" diff --git a/tests/rimport/test_main.py b/tests/rimport/test_main.py index 3451199..604d71e 100644 --- a/tests/rimport/test_main.py +++ b/tests/rimport/test_main.py @@ -86,7 +86,7 @@ def test_file_list_success( inputdata_root.mkdir() staging_root = tmp_path / "staging" staging_root.mkdir() - filelist = tmp_path / "files.txt" + filelist = inputdata_root / "files.txt" filelist.write_text("file1.nc\nfile2.nc\n") mock_get_staging_root.return_value = staging_root @@ -104,7 +104,11 @@ def test_file_list_success( assert result == 0 mock_read_filelist.assert_called_once_with(filelist) mock_normalize_paths.assert_called_once_with( - inputdata_root, ["file1.nc", "file2.nc"] + inputdata_root, + [ + str(inputdata_root.resolve() / "file1.nc"), + str(inputdata_root.resolve() / "file2.nc"), + ], ) assert mock_stage_data.call_count == 2 check = False From 27885c4a01e8d9593949e6891ef214ca4491e982 Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Tue, 25 Aug 2026 13:47:53 -0600 Subject: [PATCH 03/21] relink: Pin cwd-relative resolution of positionals from an inputdata subdir relink.py already resolves relative positionals (a bare filename and ".") against the caller's cwd rather than the inputdata root, via shared.validate_paths -> os.path.abspath. Nothing pinned that behavior, so add characterization tests that run relink.py from inside a nested inputdata subdirectory and confirm the resulting symlink points at the matching target file. --- tests/relink/test_cmdline.py | 117 +++++++++++++++++++++++++++++++++++ 1 file changed, 117 insertions(+) diff --git a/tests/relink/test_cmdline.py b/tests/relink/test_cmdline.py index 46a03c8..5f5b865 100644 --- a/tests/relink/test_cmdline.py +++ b/tests/relink/test_cmdline.py @@ -29,6 +29,26 @@ def fixture_mock_dirs(tmp_path): return source_dir, target_dir, source_file, target_file +@pytest.fixture(name="nested_mock_dirs") +def fixture_nested_mock_dirs(tmp_path): + """Create a nested source/target layout for testing relative-path resolution + from inside an inputdata subdirectory.""" + source_dir = tmp_path / "source" + target_dir = tmp_path / "target" + source_sub_dir = source_dir / "sub" + target_sub_dir = target_dir / "sub" + source_sub_dir.mkdir(parents=True) + target_sub_dir.mkdir(parents=True) + + # Create a test file + source_file = source_sub_dir / "test_file.txt" + target_file = target_sub_dir / "test_file.txt" + source_file.write_text("source content") + target_file.write_text("target content") + + return source_dir, target_dir, source_sub_dir, source_file, target_file + + def test_command_line_execution_dry_run(mock_dirs): """Test executing relink.py from command line with --dry-run flag.""" source_dir, target_dir, source_file, _ = mock_dirs @@ -136,6 +156,103 @@ def test_command_line_execution_given_file(mock_dirs): assert f"{INDENT}Created symbolic link:" in result.stdout +def test_command_line_relative_file_from_inputdata_subdir(nested_mock_dirs): + """Test that a bare relative filename is resolved against the cwd (an + inputdata subdirectory), not against the inputdata root.""" + source_dir, target_dir, source_sub_dir, source_file, target_file = ( + nested_mock_dirs + ) + + # Get the path to relink.py + relink_script = os.path.join( + os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__)))), + "relink.py", + ) + + # Build the command + command = [ + sys.executable, + relink_script, + "test_file.txt", + "--target-root", + str(target_dir), + "--inputdata-root", + str(source_dir), + ] + + # Execute the command with cwd set to the inputdata subdirectory + result = subprocess.run( + command, cwd=str(source_sub_dir), capture_output=True, text=True, check=False + ) + + # Verify the command executed successfully + assert result.returncode == 0, f"Command failed with stderr: {result.stderr}" + + # Verify the file was converted to a symlink pointing at the target copy + assert source_file.is_symlink() + assert os.readlink(str(source_file)) == str(target_file) + + +def test_command_line_relative_dir_dot_from_inputdata_subdir(nested_mock_dirs): + """Test that '.' is resolved against the cwd (an inputdata subdirectory), + not against the inputdata root. + + A decoy file sits directly under the inputdata root, outside "sub", with + a matching target copy so it *would* be relinkable if reached. Because + relink's search is recursive, resolving '.' against cwd (source_dir/sub) + never reaches the decoy, while a root-relative regression -- resolving + '.' against inputdata_root (source_dir) instead -- would recurse into + the decoy too. Only the decoy assertion below actually discriminates + between those two resolutions; the symlink-target subdirectory + (test_file.txt) is reached by recursion either way. + """ + source_dir, target_dir, source_sub_dir, source_file, target_file = ( + nested_mock_dirs + ) + + # Decoy file directly under the inputdata root (outside "sub"), with a + # matching target copy. Correct cwd-relative resolution of "." never + # reaches this file; a root-relative regression would. + decoy_file = source_dir / "decoy.txt" + decoy_target = target_dir / "decoy.txt" + decoy_file.write_text("decoy content") + decoy_target.write_text("decoy target content") + + # Get the path to relink.py + relink_script = os.path.join( + os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__)))), + "relink.py", + ) + + # Build the command + command = [ + sys.executable, + relink_script, + ".", + "--target-root", + str(target_dir), + "--inputdata-root", + str(source_dir), + ] + + # Execute the command with cwd set to the inputdata subdirectory + result = subprocess.run( + command, cwd=str(source_sub_dir), capture_output=True, text=True, check=False + ) + + # Verify the command executed successfully + assert result.returncode == 0, f"Command failed with stderr: {result.stderr}" + + # Verify the file was converted to a symlink pointing at the target copy + assert source_file.is_symlink() + assert os.readlink(str(source_file)) == str(target_file) + + # Verify the decoy outside "sub" was NOT reached/relinked + assert decoy_file.is_file() + assert not decoy_file.is_symlink() + assert decoy_file.read_text() == "decoy content" + + def test_command_line_multiple_source_dirs(temp_dirs): """Test executing relink.py with multiple source directories.""" inputdata_dir, target_dir = temp_dirs From 34b02a746b3ad30f13ca8781649778b65bbf9359 Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Tue, 25 Aug 2026 13:52:17 -0600 Subject: [PATCH 04/21] docs: Describe cwd/list-relative path resolution in rimport help and README rimport's --file/--list/positional help strings, the normalize_paths and main docstrings, and the README Notes section still described the old "everything is relative to the inputdata root" behavior. Update them to match what get_files_to_process actually does now: CLI-arg names anchor to cwd inside the inputdata tree with no fallback to the root, --list entries anchor to the list file's own directory, and a relative --list entry outside the tree is a fatal (rc 2) error. Flag the list-file case as a breaking change in the README. No logic changes. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 2 ++ rimport | 23 +++++++++++++++++++---- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 6f4e1ab..ff4b9c4 100644 --- a/README.md +++ b/README.md @@ -10,6 +10,8 @@ Tools used for publishing CESM input data. Notes: - Use `rimport --check` if you'd like to see the current status of a file, including whether it's available for download. - The `relink.py` script was previously used for step 3 above, but that functionality is now built into `rimport`. It's still there if you want to use it by itself. +- A relative filename passed to `rimport` directly (via `--file` or as a positional argument) is resolved against your current directory, not the inputdata root, whenever you run `rimport` from inside the inputdata tree (running from the root itself counts). There's no fallback to the root on a miss, so run from outside the tree, or pass an absolute path, if you want the old root-relative resolution. +- A relative entry in a `--list` file is resolved against that list file's own directory rather than against the inputdata root. **Breaking change:** if the list file lives outside the inputdata tree, relative entries are no longer resolved against the root at all — `rimport` now exits with an error, so list files kept outside the tree must use absolute paths. ## Filenames and metadata: diff --git a/rimport b/rimport index bb46acd..10aa528 100755 --- a/rimport +++ b/rimport @@ -52,7 +52,11 @@ def build_parser() -> argparse.ArgumentParser: "-file", dest="file", metavar="filename", - help="Provide a file to import. Must be in the CESM inputdata directory.", + help=( + "Provide a file to import. Must be in the CESM inputdata directory. Relative names are" + " resolved against the current directory when run from inside the inputdata tree;" + " otherwise against the inputdata root." + ), ) parser.add_argument( @@ -62,14 +66,20 @@ def build_parser() -> argparse.ArgumentParser: metavar="filelist", help=( "Provide a file that contains a list of filenames to import. All filenames in the list" - " must be in the CESM inputdata directory." + " must be in the CESM inputdata directory. Relative entries are resolved against the" + " list file's directory; if the list file is outside the inputdata tree, entries must" + " be absolute." ), ) parser.add_argument( "items_to_process", nargs="*", - help="One or more files to process. (Optional; can use --file instead to process just one.)" + help=( + "One or more files to process. (Optional; can use --file instead to process just one.)" + " Relative names are resolved against the current directory when run from inside the" + " inputdata tree; otherwise against the inputdata root." + ), ) # Add inputdata_root option flags @@ -122,6 +132,10 @@ def normalize_paths(root: Path, relnames: Iterable[str]) -> List[Path]: - If the name is relative, it is assumed to be relative to `root` and made absolute All paths are then normalized to their absolute form, replacing . and .. as needed. + Callers (get_files_to_process) already anchor inside-tree names to cwd or to the list file's + directory before calling this function, so a name that is still relative when it reaches here + is one whose anchor was outside the inputdata tree; the root-join above is what resolves those. + Note that symlinks are NOT resolved. Args: @@ -432,7 +446,8 @@ def main(argv: List[str] | None = None) -> int: Exit Codes: 0: All files staged successfully. 1: One or more files failed to stage or relink (errors printed to stderr). - 2: Fatal error (missing inputdata directory, missing file list, etc.). + 2: Fatal error (missing inputdata directory, missing file list, a relative entry in a + --list file whose directory is outside the inputdata tree, etc.). """ parser = build_parser() args = parser.parse_args(argv) From 75c79cd5764ac10bec2c94e5100ec9bda507ebee Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Tue, 25 Aug 2026 14:35:31 -0600 Subject: [PATCH 05/21] rimport: Guard stage_data against directory sources stage_data checked that the source exists but never that it was a regular file. When run from inside an inputdata subdir on a directory whose staging mirror already exists, control fell into the already-published relink branch and called replace_one_file_with_symlink, which renamed the directory to '.tmp', symlinked over it, then failed to roll back (ENOTDIR), leaving the tree mangled. An empty-string argument (e.g. an unset shell variable) hit the same path against the inputdata root itself. Add a guard right after the existing existence check: raise RuntimeError if the source is a directory (or a symlink to one). Since the guard sits before the check/no-check branches, it also closes the --check path, which previously reported a directory as "already published ... available for download". --- rimport | 6 ++ tests/rimport/test_cmdline.py | 154 ++++++++++++++++++++++++++++++++++ 2 files changed, 160 insertions(+) diff --git a/rimport b/rimport index 10aa528..7229bd9 100755 --- a/rimport +++ b/rimport @@ -186,12 +186,15 @@ def stage_data( RuntimeError: If `src` is a live symlink pointing outside staging, or if `src` is outside the inputdata root, or if `src` is already under staging directory. RuntimeError: If `src` is a broken symlink. + RuntimeError: If `src` is a directory (or a symlink to one). RuntimeError: If it failed to replace `src` with a symlink to the staged file. FileNotFoundError: If `src` does not exist. Guardrails: * Raise if `src` is a *live* symlink to a file outside staging root ("outside staging"). * Raise if `src` is a broken symlink or is outside the inputdata root. + * Raise if `src` is a directory, so a directory source can never reach the + replace-with-symlink path (which assumes a regular file and mangles a directory). """ if src.is_symlink(): if not os.path.exists(src.resolve()): @@ -210,6 +213,9 @@ def stage_data( if not src.exists(): raise FileNotFoundError(f"source not found: {src}") + if src.is_dir(): + raise RuntimeError(f"source is a directory, not a file: {src}") + try: rel = src.resolve().relative_to(inputdata_root.resolve()) except ValueError as exc: diff --git a/tests/rimport/test_cmdline.py b/tests/rimport/test_cmdline.py index 940ea3e..6bb0ae0 100644 --- a/tests/rimport/test_cmdline.py +++ b/tests/rimport/test_cmdline.py @@ -825,3 +825,157 @@ def test_check_doesnt_relink_published(self, rimport_script, test_env, rimport_e assert "Deleted original file".lower() not in result.stdout.lower() assert "Created symbolic link".lower() not in result.stdout.lower() assert "Error creating symlink".lower() not in result.stdout.lower() + + def test_directory_argument_from_subdir_errors_and_leaves_tree_intact( + self, rimport_script, test_env, rimport_env + ): + """Test that pointing rimport at a directory (e.g. via the cwd-anchored positional from + inside an inputdata subdir) errors cleanly instead of falling into the destructive + replace-with-symlink path, which would rename the directory to '.tmp', symlink it + away, and then fail to roll back.""" + inputdata_root = test_env["inputdata_root"] + staging_root = test_env["staging_root"] + + subdir = inputdata_root / "lnd" / "clm2" + subdir.mkdir(parents=True) + inner_file = subdir / "data.nc" + inner_file.write_text("clm2 data") + + # The matching staging mirror must ALSO exist as a directory, or dst.exists() is False + # and stage_data takes the harmless "not already published" branch instead of the + # destructive one — this is the fixture detail that makes the bug actually bite. + staging_mirror = staging_root / "lnd" / "clm2" + staging_mirror.mkdir(parents=True) + + # Run rimport with a relative positional directory name, from inside the parent dir + command = [ + sys.executable, + rimport_script, + "clm2", + "-inputdata", + str(inputdata_root), + ] + + result = subprocess.run( + command, + capture_output=True, + text=True, + check=False, + env=rimport_env, + cwd=subdir.parent, + ) + + # Verify failure + assert result.returncode != 0, f"Command unexpectedly passed: {result.stdout}" + assert "directory" in result.stderr + assert "not a file" in result.stderr + + # Verify the tree is intact: clm2 is still a real directory, not a symlink; no + # '.tmp' path was left anywhere under inputdata_root; and its contents are + # untouched. + assert subdir.is_dir() and not subdir.is_symlink(), ( + f"clm2 should still be a plain, non-symlink directory after the error; " + f"is_dir={subdir.is_dir()} is_symlink={subdir.is_symlink()}" + ) + tmp_paths = list(inputdata_root.rglob("*.tmp")) + assert not tmp_paths, f"Found unexpected '.tmp' path(s) left behind: {tmp_paths}" + assert inner_file.read_text() == "clm2 data" + + def test_empty_string_argument_errors_and_leaves_tree_intact( + self, rimport_script, test_env, rimport_env + ): + """Test that an empty-string positional (as from an unset shell variable, e.g. + `rimport "$maybe_unset"`) errors cleanly instead of anchoring to the inputdata root + itself and running that root through the destructive replace-with-symlink path.""" + inputdata_root = test_env["inputdata_root"] + # staging_root itself need not be assigned here — the fixture already created it, and + # its mere existence is what makes dst.exists() true for rel="." (see the brief: this is + # what turns an unset shell variable into a whole-tree rename). + + marker_file = inputdata_root / "marker.nc" + marker_file.write_text("root marker") + + # Run rimport with an empty-string positional, from inside the inputdata root itself, + # so it anchors (via _anchor_cli) to the root — the same as an unset shell variable + # expanding to "". + command = [ + sys.executable, + rimport_script, + "", + "-inputdata", + str(inputdata_root), + ] + + result = subprocess.run( + command, + capture_output=True, + text=True, + check=False, + env=rimport_env, + cwd=inputdata_root, + ) + + # Verify failure + assert result.returncode != 0, f"Command unexpectedly passed: {result.stdout}" + assert "directory" in result.stderr + assert "not a file" in result.stderr + + # Verify the inputdata root itself is untouched: still a real directory, not renamed, + # not replaced with a symlink, no '.tmp' sibling. + assert inputdata_root.is_dir() and not inputdata_root.is_symlink(), ( + f"inputdata root should still be a plain, non-symlink directory after the error; " + f"is_dir={inputdata_root.is_dir()} is_symlink={inputdata_root.is_symlink()}" + ) + tmp_siblings = list(inputdata_root.parent.glob(f"{inputdata_root.name}.tmp")) + assert not tmp_siblings, f"Found unexpected '.tmp' sibling(s): {tmp_siblings}" + assert marker_file.read_text() == "root marker" + + def test_check_directory_argument_reports_error_not_publishable( + self, rimport_script, test_env, rimport_env + ): + """Test that --check on a directory argument reports it as an error, rather than + claiming (as it did before the guard) that the directory is already published but not + linked and available for download.""" + inputdata_root = test_env["inputdata_root"] + staging_root = test_env["staging_root"] + + subdir = inputdata_root / "lnd" / "clm2" + subdir.mkdir(parents=True) + inner_file = subdir / "data.nc" + inner_file.write_text("clm2 data") + + # Matching staging mirror, as in the destructive-path test above. + staging_mirror = staging_root / "lnd" / "clm2" + staging_mirror.mkdir(parents=True) + + command = [ + sys.executable, + rimport_script, + "clm2", + "-inputdata", + str(inputdata_root), + "--check", + ] + + result = subprocess.run( + command, + capture_output=True, + text=True, + check=False, + env=rimport_env, + cwd=subdir.parent, + ) + + # Verify failure + assert result.returncode != 0, f"Command unexpectedly passed: {result.stdout}" + assert "directory" in result.stderr + assert "not a file" in result.stderr + + # Verify --check does NOT claim the directory is already published / downloadable + assert "already published" not in result.stdout.lower() + assert "available for download" not in result.stdout.lower() + + # Verify the tree is intact + assert subdir.is_dir() and not subdir.is_symlink() + assert not list(inputdata_root.rglob("*.tmp")) + assert inner_file.read_text() == "clm2 data" From 964a7190e14b4a8c33dd28a77d05e0f6d0256126 Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Tue, 25 Aug 2026 14:45:23 -0600 Subject: [PATCH 06/21] rimport: Fix stage_data docstring overclaim on symlink-to-directory Review found that the guard's docstring claimed RuntimeError covers "a directory (or a symlink to one)". False: the is_symlink() branch above the guard returns early (rc 0, "already published and linked") for a live symlink whose target is a directory, without ever reaching the new guard. That state is reachable in practice: it is exactly the tree left behind by the pre-fix bug this task closed, so a user cleaning up after that incident would hit it. Correct the docstring to say only what the code does; no logic change. --- rimport | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/rimport b/rimport index 7229bd9..d0b6f73 100755 --- a/rimport +++ b/rimport @@ -186,15 +186,20 @@ def stage_data( RuntimeError: If `src` is a live symlink pointing outside staging, or if `src` is outside the inputdata root, or if `src` is already under staging directory. RuntimeError: If `src` is a broken symlink. - RuntimeError: If `src` is a directory (or a symlink to one). + RuntimeError: If `src` is a directory. This check runs only when `src` is not itself + a symlink — see the symlink guardrails above, which return early (without + raising) for a symlink whose target is a directory. RuntimeError: If it failed to replace `src` with a symlink to the staged file. FileNotFoundError: If `src` does not exist. Guardrails: * Raise if `src` is a *live* symlink to a file outside staging root ("outside staging"). * Raise if `src` is a broken symlink or is outside the inputdata root. - * Raise if `src` is a directory, so a directory source can never reach the - replace-with-symlink path (which assumes a regular file and mangles a directory). + * Raise if `src` is a directory and not itself a symlink, so a non-symlink directory + source can never reach the replace-with-symlink path (which assumes a regular file + and mangles a directory). A *symlink* whose target is a directory is NOT covered by + this guardrail: it is handled by the live-symlink guardrails above instead, which log + "already published and linked" and return without raising. """ if src.is_symlink(): if not os.path.exists(src.resolve()): From 8768208352db8b00eac74f9801dba3e79feba2a3 Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Tue, 25 Aug 2026 14:54:30 -0600 Subject: [PATCH 07/21] tests: Pin --list entries anchoring to list-file dir, not cwd Add the two missing semantic tests for --list resolution flagged by the final branch review: every existing list test runs with cwd outside the inputdata tree, so cwd-anchoring and list-dir-anchoring produce the same answer and can't be told apart. Add a unit test (test_get_files_to_process.py) and an e2e counterpart (test_cmdline.py) that put the cwd inside the tree at a location different from the list file, with a decoy file at the cwd-anchored path, to make the discrimination concrete. Also add the list-side twin of test_dotdot_escape_from_subdir_errors: an in-tree list file with a '..'-escaping entry, pinning the already-correct behavior (rc 1, "not under inputdata root") that was previously unpinned. No production-code change; test-only. Co-Authored-By: Claude Opus 5 (1M context) --- tests/rimport/test_cmdline.py | 113 +++++++++++++++++++++ tests/rimport/test_get_files_to_process.py | 41 ++++++++ 2 files changed, 154 insertions(+) diff --git a/tests/rimport/test_cmdline.py b/tests/rimport/test_cmdline.py index 6bb0ae0..0c04583 100644 --- a/tests/rimport/test_cmdline.py +++ b/tests/rimport/test_cmdline.py @@ -183,6 +183,68 @@ def test_list_inside_tree_relative_entries( assert nested_file.is_symlink() assert nested_file.resolve() == staged_file + def test_list_inside_tree_relative_entries_anchor_to_list_dir_not_cwd( + self, rimport_script, test_env, rimport_env + ): + """Test that a list file's relative entries anchor to the list file's own directory, + not the cwd, even when rimport is run with its cwd inside the tree at a DIFFERENT + location. The existing e2e list test passes no cwd= to subprocess.run, so pytest's own + (outside-the-tree) cwd applies and cwd-anchoring and list-dir-anchoring agree; this test + sets cwd= explicitly so the two schemes can be told apart.""" + inputdata_root = test_env["inputdata_root"] + staging_root = test_env["staging_root"] + + # Real file, alongside the list file inside "lnd" + nested_file = inputdata_root / "lnd" / "clm2" / "file1.nc" + nested_file.parent.mkdir(parents=True) + nested_file.write_text("real data") + + filelist = inputdata_root / "lnd" / "filelist.txt" + filelist.write_text("clm2/file1.nc\n") + + # Decoy at the cwd-anchored location: a cwd-anchoring regression would resolve here + # instead, giving a wrong-file failure rather than a merely-missing-file one. + decoy_file = inputdata_root / "atm" / "clm2" / "file1.nc" + decoy_file.parent.mkdir(parents=True) + decoy_file.write_text("decoy data") + + # Run rimport with -list option, cwd inside the tree but at a DIFFERENT location + # than the list file + command = [ + sys.executable, + rimport_script, + "-list", + str(filelist), + "-inputdata", + str(inputdata_root), + ] + + result = subprocess.run( + command, + capture_output=True, + text=True, + check=False, + env=rimport_env, + cwd=inputdata_root / "atm", + ) + + # Verify success + assert result.returncode == 0, f"Command failed: {result.stderr}" + + # Verify the real file (not the decoy) was staged, anchored to the list dir's subtree + staged_file = staging_root / "lnd" / "clm2" / "file1.nc" + assert staged_file.exists() + assert staged_file.read_text() == "real data" + + # Verify file was relinked + assert nested_file.is_symlink() + assert nested_file.resolve() == staged_file + + # Verify the decoy was left untouched, and nothing staged at the cwd-anchored path + assert not decoy_file.is_symlink() + assert decoy_file.read_text() == "decoy data" + assert not (staging_root / "atm" / "clm2" / "file1.nc").exists() + def test_list_outside_tree_relative_entry_error( self, rimport_script, test_env, rimport_env ): @@ -777,6 +839,57 @@ def test_dotdot_escape_from_subdir_errors( assert not outside_file.is_symlink() assert outside_file.read_text() == "outside data" + def test_dotdot_escape_from_list_inside_tree_errors( + self, rimport_script, test_env, rimport_env + ): + """Test that a '..'-escaping relative entry in a list file INSIDE the tree is rejected + by stage_data's existing outside-the-root guardrail. This is the list-side twin of + test_dotdot_escape_from_subdir_errors above.""" + inputdata_root = test_env["inputdata_root"] + staging_root = test_env["staging_root"] + tmp_path = test_env["tmp_path"] + + list_dir = inputdata_root / "lnd" + list_dir.mkdir(parents=True) + + # File must exist, or the "source not found" check fires before the guardrail and the + # "not under inputdata root" message never appears. + outside_file = tmp_path / "outside.nc" + outside_file.write_text("outside data") + + # List file INSIDE the tree, with an entry that escapes the tree + filelist = list_dir / "filelist.txt" + filelist.write_text("../../outside.nc\n") + + # Run rimport with -list option + command = [ + sys.executable, + rimport_script, + "-list", + str(filelist), + "-inputdata", + str(inputdata_root), + ] + + result = subprocess.run( + command, + capture_output=True, + text=True, + check=False, + env=rimport_env, + ) + + # Verify failure + assert result.returncode == 1, f"Command unexpectedly passed: {result.stdout}" + assert "not under inputdata root" in result.stderr + + # Verify nothing was staged + assert not any(staging_root.rglob("*")) + + # Verify the outside file was left untouched + assert not outside_file.is_symlink() + assert outside_file.read_text() == "outside data" + def test_check_doesnt_relink_published(self, rimport_script, test_env, rimport_env): """Test that published file is not relinked if check is True.""" inputdata_root = test_env["inputdata_root"] diff --git a/tests/rimport/test_get_files_to_process.py b/tests/rimport/test_get_files_to_process.py index f0e55bd..385e433 100644 --- a/tests/rimport/test_get_files_to_process.py +++ b/tests/rimport/test_get_files_to_process.py @@ -222,6 +222,47 @@ def test_list_inside_tree_relative_entries_anchored_to_list_dir(self, tmp_path): list_dir_resolved = list_dir.resolve() assert files_to_process == [str(list_dir_resolved / f) for f in filenames] + def test_list_relative_entries_anchor_to_list_dir_not_cwd(self, tmp_path, monkeypatch): + """Test that relative list entries anchor to the list file's own directory even when + the cwd is inside the tree at a DIFFERENT location. This is the discriminating setup: + every other list test runs with cwd outside the tree, where cwd-anchoring and + list-dir-anchoring agree and so can't tell the two schemes apart.""" + # Setup + inputdata_root = tmp_path / "inputdata" + atm_dir = inputdata_root / "atm" + list_dir = inputdata_root / "lnd" + atm_dir.mkdir(parents=True) + list_dir.mkdir(parents=True) + + # Real file, alongside the list file's own subtree + real_file = list_dir / "clm2" / "file1.nc" + real_file.parent.mkdir(parents=True) + real_file.write_text("real data") + + # Decoy at the cwd-anchored location: a cwd-anchoring regression would resolve here + # instead, giving a wrong-file failure rather than a merely-missing-file one. + decoy_file = atm_dir / "clm2" / "file1.nc" + decoy_file.parent.mkdir(parents=True) + decoy_file.write_text("decoy data") + + filelist = list_dir / "filelist.txt" + filelist.write_text("clm2/file1.nc\n", encoding="utf8") + + # cwd inside the tree, but at a different location than the list file + monkeypatch.chdir(atm_dir) + + # Run + files_to_process, result = rimport.get_files_to_process( + file=None, + filelist=filelist, + items_to_process=None, + inputdata_root=inputdata_root, + ) + + # Verify + assert result == 0 + assert files_to_process == [str(real_file.resolve())] + def test_list_at_root_relative_entries_anchored_to_root(self, tmp_path): """Test that a list file located at the inputdata root itself (root counts as inside the tree) anchors relative entries to the root""" From 17cf275d42786e4927a905dd5dfe2d0602a8472b Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Tue, 25 Aug 2026 15:02:31 -0600 Subject: [PATCH 08/21] rimport: Don't raise when cwd has been deleted get_files_to_process() called Path.cwd() unconditionally to decide whether relative CLI-arg names should anchor to cwd. If the process's cwd has been deleted out from under it, Path.cwd() raises FileNotFoundError (a subclass of OSError), so rimport died with an unhandled traceback even when every argument was absolute and cwd was irrelevant to the operation. Before the branch that introduced this call, that case returned rc 0. Catch OSError around the cwd lookup and treat "cwd can't be determined" the same as "cwd is outside the tree": fall back to the pre-existing root-relative behavior instead of propagating the exception. cwd is only ever dereferenced when cwd_inside is True, and cwd_inside is now always False in the except branch, so cwd=None is never used unguarded. Also warn (once) when this fallback actually changes behavior: if any of the CLI-supplied file/positional names are relative, log that cwd couldn't be determined and those names will resolve against the inputdata root instead. The common absolute-path case stays silent, since cwd genuinely doesn't matter there. Co-Authored-By: Claude Opus 5 (1M context) --- rimport | 19 +++++++- tests/rimport/test_get_files_to_process.py | 57 ++++++++++++++++++++++ 2 files changed, 74 insertions(+), 2 deletions(-) diff --git a/rimport b/rimport index d0b6f73..b6d3aca 100755 --- a/rimport +++ b/rimport @@ -386,8 +386,23 @@ def get_files_to_process( int: Result code """ root_resolved = Path(inputdata_root).expanduser().resolve() - cwd = Path.cwd().resolve() - cwd_inside = cwd.is_relative_to(root_resolved) + try: + cwd = Path.cwd().resolve() + cwd_inside = cwd.is_relative_to(root_resolved) + except OSError: + # cwd may have been deleted out from under the process (FileNotFoundError, itself an + # OSError). We can't anchor to a cwd we can't determine, so treat this the same as + # "cwd is outside the tree": fall back to the pre-existing root-relative behavior + # instead of letting the exception propagate. + cwd = None + cwd_inside = False + cli_args = ([file] if file is not None else []) + list(items_to_process or []) + if any(not Path(name).is_absolute() for name in cli_args): + logger.warning( + "rimport: could not determine the current working directory (it may have " + "been deleted); relative --file/positional names will be resolved against " + "the inputdata root instead of cwd" + ) def _anchor_cli(name): if Path(name).is_absolute(): diff --git a/tests/rimport/test_get_files_to_process.py b/tests/rimport/test_get_files_to_process.py index 385e433..1eddf00 100644 --- a/tests/rimport/test_get_files_to_process.py +++ b/tests/rimport/test_get_files_to_process.py @@ -640,3 +640,60 @@ def test_cli_cwd_inside_tree_via_symlink(self, tmp_path, monkeypatch): # Verify assert result == 0 assert files_to_process == [str(real_sub.resolve() / filename)] + + def test_deleted_cwd_with_absolute_names_still_works(self, tmp_path, monkeypatch, caplog): + """Test that a deleted cwd does not raise: absolute names are returned unchanged since + cwd is irrelevant to resolving them. Simulates a deleted cwd for real (not mocked): chdir + into a directory, then remove it out from under the process (confirmed to actually raise + FileNotFoundError from Path.cwd() on this platform before writing this test).""" + inputdata_root = tmp_path / "inputdata" + inputdata_root.mkdir() + abs_file = str(inputdata_root / "test.nc") + + deleted_dir = tmp_path / "deleted" + deleted_dir.mkdir() + monkeypatch.chdir(deleted_dir) + deleted_dir.rmdir() + + # Run + files_to_process, result = rimport.get_files_to_process( + file=abs_file, + filelist=None, + items_to_process=None, + inputdata_root=inputdata_root, + ) + + # Verify + assert result == 0 + assert files_to_process == [abs_file] + # No warning: the cwd being undeterminable doesn't matter for absolute names + assert "working directory" not in caplog.text.lower() + + def test_deleted_cwd_with_relative_names_falls_back_to_root( + self, tmp_path, monkeypatch, caplog + ): + """Test that a deleted cwd does not raise for relative names either: since cwd can't be + determined, relative names are left unanchored for normalize_paths to later resolve + against inputdata_root (the pre-existing legacy behavior), rather than raising.""" + inputdata_root = tmp_path / "inputdata" + inputdata_root.mkdir() + filename = "test.nc" + + deleted_dir = tmp_path / "deleted" + deleted_dir.mkdir() + monkeypatch.chdir(deleted_dir) + deleted_dir.rmdir() + + # Run + files_to_process, result = rimport.get_files_to_process( + file=filename, + filelist=None, + items_to_process=None, + inputdata_root=inputdata_root, + ) + + # Verify + assert result == 0 + assert files_to_process == [filename] + # Relative names ARE affected (resolved against the root, not cwd, once staged) -- warn + assert "working directory" in caplog.text.lower() From 69b3e4749b8850df2fcc69d4cb44454b5e2a1e95 Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Tue, 25 Aug 2026 15:15:07 -0600 Subject: [PATCH 09/21] rimport: Make --help and the list-file error self-sufficient --help for --file and the positional items now states explicitly that there is no fallback to the inputdata root when a relative name isn't found under cwd, so the strict resolution rule doesn't have to be inferred from the README. The out-of-tree list-file error now appends the remedy (absolute paths, or move the list file into the tree) so a user hitting it doesn't need to go read the README. Also corrects two docstring passages in stage_data() that overstated when a directory-target symlink returns without raising: that only holds when the target is under staging_root, not when it's outside (which raises via the existing "outside staging" guardrail). Fixed an adjacent one-word inaccuracy ("file" -> "target") in the same Guardrails block, since the underlying check never inspects target type. And documents get_files_to_process()'s deleted-cwd fallback (added in 17cf275) and the condition under which its warning fires. Text-only change; no logic touched. Co-Authored-By: Claude Opus 5 (1M context) --- rimport | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/rimport b/rimport index b6d3aca..a47d3ef 100755 --- a/rimport +++ b/rimport @@ -54,8 +54,9 @@ def build_parser() -> argparse.ArgumentParser: metavar="filename", help=( "Provide a file to import. Must be in the CESM inputdata directory. Relative names are" - " resolved against the current directory when run from inside the inputdata tree;" - " otherwise against the inputdata root." + " resolved against the current directory when run from inside the inputdata tree (no" + " fallback to the root if the file isn't found there); otherwise against the" + " inputdata root." ), ) @@ -78,7 +79,8 @@ def build_parser() -> argparse.ArgumentParser: help=( "One or more files to process. (Optional; can use --file instead to process just one.)" " Relative names are resolved against the current directory when run from inside the" - " inputdata tree; otherwise against the inputdata root." + " inputdata tree (no fallback to the root if the file isn't found there); otherwise" + " against the inputdata root." ), ) @@ -188,18 +190,21 @@ def stage_data( RuntimeError: If `src` is a broken symlink. RuntimeError: If `src` is a directory. This check runs only when `src` is not itself a symlink — see the symlink guardrails above, which return early (without - raising) for a symlink whose target is a directory. + raising) for a symlink whose target is a directory under `staging_root`. + A symlink whose directory target is outside `staging_root` instead raises + via the first entry above. RuntimeError: If it failed to replace `src` with a symlink to the staged file. FileNotFoundError: If `src` does not exist. Guardrails: - * Raise if `src` is a *live* symlink to a file outside staging root ("outside staging"). + * Raise if `src` is a *live* symlink to a target outside staging root ("outside staging"). * Raise if `src` is a broken symlink or is outside the inputdata root. * Raise if `src` is a directory and not itself a symlink, so a non-symlink directory source can never reach the replace-with-symlink path (which assumes a regular file and mangles a directory). A *symlink* whose target is a directory is NOT covered by - this guardrail: it is handled by the live-symlink guardrails above instead, which log - "already published and linked" and return without raising. + this guardrail: it is handled by the live-symlink guardrails above instead, which for + a target under `staging_root` log "already published and linked" and return without + raising, and for a target outside `staging_root` raise (see the first guardrail above). """ if src.is_symlink(): if not os.path.exists(src.resolve()): @@ -368,6 +373,15 @@ def get_files_to_process( with no fallback to the root. If cwd is outside the tree, they are left unchanged for normalize_paths to later resolve against `inputdata_root`, as before. + If `Path.cwd()` itself raises (e.g. the working directory was deleted out from under the + process), that is treated the same as "cwd is outside the tree": non-absolute + `file`/`items_to_process` entries fall back to the root-relative resolution described above, + instead of the exception propagating. A warning is logged in this case, but only when at + least one `file`/`items_to_process` entry is non-absolute — that's the only case where the + fallback changes which file gets resolved. This fallback and its warning are specific to + `file`/`items_to_process`; `--list` entries are unaffected, since they are anchored to the + list file's own directory rather than to cwd. + Non-absolute `--list` entries are anchored against the list file's own directory (the root itself counts as inside), not the cwd: if that directory is inside the inputdata tree, a relative entry is resolved (eagerly, absolute) against it; if the list file's directory is @@ -436,7 +450,8 @@ def get_files_to_process( else: logger.error( "rimport: relative path '%s' not allowed in list file outside " - "the inputdata tree: %s", + "the inputdata tree: %s; use absolute paths or move the list " + "file into the inputdata tree", entry, list_path, ) From 1b770e435047fe5e415ed68a7caea17b9b780f4f Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Tue, 25 Aug 2026 15:27:43 -0600 Subject: [PATCH 10/21] tests: Add decoy to relink single-file pinning test The single-file sibling of test_command_line_relative_dir_dot_from_inputdata_subdir never got the decoy treatment its neighbor received after review. Add a same-named decoy file at the inputdata root (distinct content, matching decoy target) so the test discriminates cwd-relative resolution from a root-relative regression by file content, not merely by whether a symlink exists. Co-Authored-By: Claude Opus 5 (1M context) --- tests/relink/test_cmdline.py | 34 ++++++++++++++++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/tests/relink/test_cmdline.py b/tests/relink/test_cmdline.py index 5f5b865..e2f83da 100644 --- a/tests/relink/test_cmdline.py +++ b/tests/relink/test_cmdline.py @@ -158,11 +158,35 @@ def test_command_line_execution_given_file(mock_dirs): def test_command_line_relative_file_from_inputdata_subdir(nested_mock_dirs): """Test that a bare relative filename is resolved against the cwd (an - inputdata subdirectory), not against the inputdata root.""" + inputdata subdirectory), not against the inputdata root. + + A same-named decoy file sits directly under the inputdata root, outside + "sub", with different content than the intended file, plus a matching + target copy (also with different content) so it *would* be relinked if a + root-relative regression resolved "test_file.txt" against inputdata_root + instead of cwd. Because this is a single-file argument rather than a + directory to recurse into, such a regression would process the decoy + INSTEAD OF the subdir file, not in addition to it. The return code and + decoy_file.is_file() pass either way (the latter because is_file() + follows a symlink to a real file); what actually discriminates is that + the decoy stays a plain file with its original content (not relinked), + and that the intended subdir file is the one converted to a symlink -- + pointing at the subdir's target copy, not the root decoy's. + """ source_dir, target_dir, source_sub_dir, source_file, target_file = ( nested_mock_dirs ) + # Decoy file directly under the inputdata root (outside "sub"), same + # name as the intended file but different content, with a matching + # target copy (also different content). Correct cwd-relative resolution + # of "test_file.txt" never reaches this file; a root-relative regression + # would relink it instead of the subdir file. + decoy_file = source_dir / "test_file.txt" + decoy_target = target_dir / "test_file.txt" + decoy_file.write_text("decoy content") + decoy_target.write_text("decoy target content") + # Get the path to relink.py relink_script = os.path.join( os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__)))), @@ -188,10 +212,16 @@ def test_command_line_relative_file_from_inputdata_subdir(nested_mock_dirs): # Verify the command executed successfully assert result.returncode == 0, f"Command failed with stderr: {result.stderr}" - # Verify the file was converted to a symlink pointing at the target copy + # Verify the intended subdir file was converted to a symlink pointing at + # the subdir's target copy (not the root decoy's) assert source_file.is_symlink() assert os.readlink(str(source_file)) == str(target_file) + # Verify the decoy at the inputdata root was NOT reached/relinked + assert decoy_file.is_file() + assert not decoy_file.is_symlink() + assert decoy_file.read_text() == "decoy content" + def test_command_line_relative_dir_dot_from_inputdata_subdir(nested_mock_dirs): """Test that '.' is resolved against the cwd (an inputdata subdirectory), From 1a2991d725caef652d4db956fe85b69cd09abcbd Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Tue, 25 Aug 2026 15:34:10 -0600 Subject: [PATCH 11/21] tests(rimport): cover list-error remedy text, pin log level, add Setup headers Three small, independent test-hardening edits deferred as Minor findings from the Task I and J reviews, batched into one commit: - Assert on a distinctive fragment of the list-error remedy clause ("use absolute paths or move the list file") in test_list_outside_tree_relative_entry_errors, so deleting the remedy text from rimport's error message would be caught (previously only the two dynamic values were asserted). - Wrap the deleted-cwd tests' calls in caplog.at_level(logging.WARNING), matching the idiom used in tests/relink/test_verbosity.py and test_timing.py, so they no longer rely implicitly on shared.get_log_level never exceeding WARNING. - Add the missing "# Setup" comment header to those same two tests to match the Setup/Run/Verify triad used elsewhere in this file. No production code changed. --- tests/rimport/test_get_files_to_process.py | 30 +++++++++++++--------- 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/tests/rimport/test_get_files_to_process.py b/tests/rimport/test_get_files_to_process.py index 1eddf00..7b7d6b5 100644 --- a/tests/rimport/test_get_files_to_process.py +++ b/tests/rimport/test_get_files_to_process.py @@ -2,6 +2,7 @@ Tests for get_files_to_process function in rimport script. """ +import logging import os import importlib.util from importlib.machinery import SourceFileLoader @@ -309,6 +310,7 @@ def test_list_outside_tree_relative_entry_errors(self, tmp_path, caplog): assert files_to_process is None assert "relative_file.nc" in caplog.text assert str(filelist.resolve()) in caplog.text + assert "use absolute paths or move the list file" in caplog.text def test_list_outside_tree_absolute_entries_ok(self, tmp_path): """Test that a list file outside the tree still works when all entries are absolute""" @@ -646,6 +648,7 @@ def test_deleted_cwd_with_absolute_names_still_works(self, tmp_path, monkeypatch cwd is irrelevant to resolving them. Simulates a deleted cwd for real (not mocked): chdir into a directory, then remove it out from under the process (confirmed to actually raise FileNotFoundError from Path.cwd() on this platform before writing this test).""" + # Setup inputdata_root = tmp_path / "inputdata" inputdata_root.mkdir() abs_file = str(inputdata_root / "test.nc") @@ -656,12 +659,13 @@ def test_deleted_cwd_with_absolute_names_still_works(self, tmp_path, monkeypatch deleted_dir.rmdir() # Run - files_to_process, result = rimport.get_files_to_process( - file=abs_file, - filelist=None, - items_to_process=None, - inputdata_root=inputdata_root, - ) + with caplog.at_level(logging.WARNING): + files_to_process, result = rimport.get_files_to_process( + file=abs_file, + filelist=None, + items_to_process=None, + inputdata_root=inputdata_root, + ) # Verify assert result == 0 @@ -675,6 +679,7 @@ def test_deleted_cwd_with_relative_names_falls_back_to_root( """Test that a deleted cwd does not raise for relative names either: since cwd can't be determined, relative names are left unanchored for normalize_paths to later resolve against inputdata_root (the pre-existing legacy behavior), rather than raising.""" + # Setup inputdata_root = tmp_path / "inputdata" inputdata_root.mkdir() filename = "test.nc" @@ -685,12 +690,13 @@ def test_deleted_cwd_with_relative_names_falls_back_to_root( deleted_dir.rmdir() # Run - files_to_process, result = rimport.get_files_to_process( - file=filename, - filelist=None, - items_to_process=None, - inputdata_root=inputdata_root, - ) + with caplog.at_level(logging.WARNING): + files_to_process, result = rimport.get_files_to_process( + file=filename, + filelist=None, + items_to_process=None, + inputdata_root=inputdata_root, + ) # Verify assert result == 0 From 85fb2d72113c4e0016e71722696bf21656f7c42d Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Tue, 25 Aug 2026 15:40:09 -0600 Subject: [PATCH 12/21] tests(rimport): revert item-3 Setup headers per review finding Task L review found the "# Setup" headers added to the two deleted-cwd tests matched the wrong neighbouring convention: the 8 tests directly above them (test_single_or_filelist_or_list_required through test_cli_cwd_inside_tree_via_symlink, lines 497-622) all have real setup code followed straight by "# Run"/"# Verify", with no "# Setup" header. Adding the header made the two tests match a non-adjacent earlier block instead of their true nearest neighbours, creating a new inconsistency. Revert the two "# Setup" additions; items 1 and 2 (the remedy-text assertion and the caplog.at_level(logging.WARNING) wraps) are untouched. --- tests/rimport/test_get_files_to_process.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/rimport/test_get_files_to_process.py b/tests/rimport/test_get_files_to_process.py index 7b7d6b5..d3ad127 100644 --- a/tests/rimport/test_get_files_to_process.py +++ b/tests/rimport/test_get_files_to_process.py @@ -648,7 +648,6 @@ def test_deleted_cwd_with_absolute_names_still_works(self, tmp_path, monkeypatch cwd is irrelevant to resolving them. Simulates a deleted cwd for real (not mocked): chdir into a directory, then remove it out from under the process (confirmed to actually raise FileNotFoundError from Path.cwd() on this platform before writing this test).""" - # Setup inputdata_root = tmp_path / "inputdata" inputdata_root.mkdir() abs_file = str(inputdata_root / "test.nc") @@ -679,7 +678,6 @@ def test_deleted_cwd_with_relative_names_falls_back_to_root( """Test that a deleted cwd does not raise for relative names either: since cwd can't be determined, relative names are left unanchored for normalize_paths to later resolve against inputdata_root (the pre-existing legacy behavior), rather than raising.""" - # Setup inputdata_root = tmp_path / "inputdata" inputdata_root.mkdir() filename = "test.nc" From 6422292e47e3781e33453d7e2d67199da3c6323b Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Tue, 25 Aug 2026 17:09:24 -0600 Subject: [PATCH 13/21] tests(rimport): use absolute paths in eight e2e tests, drop hidden fallback dep Eight e2e tests in test_cmdline.py passed a bare relative filename to rimport without cwd= on subprocess.run, so they only passed via rimport's legacy "resolve a relative name against the inputdata root" fallback rather than any real cwd relationship. Swap each bare name for an absolute path -- the str() of a Path already in scope (test_file, nested_file, src), or inputdata_root / "nonexistent.nc" for the negative test -- so these staging-mechanics tests no longer ride on a fallback that's about to be removed. Co-Authored-By: Claude Sonnet 5 Co-Authored-By: Claude Opus 5 (1M context) --- tests/rimport/test_cmdline.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/rimport/test_cmdline.py b/tests/rimport/test_cmdline.py index 0c04583..5e3cdce 100644 --- a/tests/rimport/test_cmdline.py +++ b/tests/rimport/test_cmdline.py @@ -61,7 +61,7 @@ def test_file_option_stages_single_file( sys.executable, rimport_script, "-file", - "test.nc", + str(test_file), "-inputdata", str(inputdata_root), ] @@ -303,7 +303,7 @@ def test_preserves_directory_structure(self, rimport_script, test_env, rimport_e sys.executable, rimport_script, "-file", - "dir1/dir2/file.nc", + str(nested_file), "-inputdata", str(inputdata_root), ] @@ -337,7 +337,7 @@ def test_error_for_nonexistent_file(self, rimport_script, test_env, rimport_env) sys.executable, rimport_script, "-file", - "nonexistent.nc", + str(inputdata_root / "nonexistent.nc"), "-inputdata", str(inputdata_root), ] @@ -501,7 +501,7 @@ def test_prints_and_exits_for_already_published_linked_file( sys.executable, rimport_script, "-file", - "link.nc", + str(src), "-inputdata", str(inputdata_root), ] @@ -544,7 +544,7 @@ def test_error_broken_symlink(self, rimport_script, test_env, rimport_env): sys.executable, rimport_script, "-file", - "link.nc", + str(src), "-inputdata", str(inputdata_root), ] @@ -586,7 +586,7 @@ def test_error_symlink_pointing_outside_staging( sys.executable, rimport_script, "-file", - "link.nc", + str(src), "-inputdata", str(inputdata_root), ] @@ -624,7 +624,7 @@ def test_check_doesnt_copy_unpublished(self, rimport_script, test_env, rimport_e sys.executable, rimport_script, "-file", - file_basename, + str(test_file), "-inputdata", str(inputdata_root), "--check", @@ -910,7 +910,7 @@ def test_check_doesnt_relink_published(self, rimport_script, test_env, rimport_e sys.executable, rimport_script, "-file", - file_basename, + str(test_file), "-inputdata", str(inputdata_root), "--check", From 4f7a9a20c78306a049673c54c7a16312927ad975 Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Tue, 25 Aug 2026 17:38:22 -0600 Subject: [PATCH 14/21] rimport: Delete the root-fallback for relative CLI/list resolution Replace the two-mode design in get_files_to_process with a single rule: a relative --file/positional name always anchors to cwd, a relative --list entry always anchors to the list file's own directory, and absolute paths are never touched. There is no root-join anywhere any more. The fallback was the bug it was meant to accommodate. When a subdirectory of the inputdata tree is a symlink, os.getcwd() returns the physical path, the "is cwd inside the tree" test fails, and the fallback silently published a same-named file from the root instead -- the wrong file, with exit code 0. Root-relative CLI paths were never an intended workflow, so nothing supported is lost; files under a symlinked-out subdirectory were already unpublishable, because stage_data resolves the source and rejects anything outside the root. Also drop the now-dead inputdata_root parameter (main is the only production caller), and make an undeterminable cwd fatal for relative names -- with no fallback there is nothing to anchor against -- reporting every offending name in one message rather than warning and guessing. Tests: delete three unit tests and one e2e test that existed only to pin the removed mode; flip five unit tests that asserted relative names come back unanchored; replace the deleted outside-the-tree e2e test with one that pins the new behavior, keeping a same-named decoy at the root so it fails if the fallback ever returns. Co-Authored-By: Claude Sonnet 5 Co-Authored-By: Claude Opus 5 (1M context) --- rimport | 90 +++------ tests/rimport/test_cmdline.py | 83 +++----- tests/rimport/test_get_files_to_process.py | 216 ++++++++------------- tests/rimport/test_main.py | 8 +- 4 files changed, 142 insertions(+), 255 deletions(-) diff --git a/rimport b/rimport index a47d3ef..4189005 100755 --- a/rimport +++ b/rimport @@ -361,74 +361,56 @@ def print_can_file_be_downloaded(file_can_be_downloaded: bool): logger.info("%sFile is not (yet) available for download.", INDENT) -def get_files_to_process( - file: str, filelist: str, items_to_process: list, inputdata_root: Path -): +def get_files_to_process(file: str, filelist: str, items_to_process: list): """Get list of files to process. Uses --file and/or --filelist arguments, as well as positional items_to_process if given. - Non-absolute `file` and `items_to_process` entries are CLI args: if cwd is inside the - inputdata tree (the root itself counts), they are anchored (eagerly, absolute) to cwd, - with no fallback to the root. If cwd is outside the tree, they are left unchanged for - normalize_paths to later resolve against `inputdata_root`, as before. - - If `Path.cwd()` itself raises (e.g. the working directory was deleted out from under the - process), that is treated the same as "cwd is outside the tree": non-absolute - `file`/`items_to_process` entries fall back to the root-relative resolution described above, - instead of the exception propagating. A warning is logged in this case, but only when at - least one `file`/`items_to_process` entry is non-absolute — that's the only case where the - fallback changes which file gets resolved. This fallback and its warning are specific to - `file`/`items_to_process`; `--list` entries are unaffected, since they are anchored to the - list file's own directory rather than to cwd. - - Non-absolute `--list` entries are anchored against the list file's own directory (the root - itself counts as inside), not the cwd: if that directory is inside the inputdata tree, a - relative entry is resolved (eagerly, absolute) against it; if the list file's directory is - outside the tree, a relative entry is a fatal error (relative paths are not allowed in list - files outside the inputdata tree). + One rule, no modes, no root-relative fallback: non-absolute `file` and + `items_to_process` entries are CLI args and always anchor (eagerly, absolute) to cwd. + Non-absolute `--list` entries always anchor (eagerly, absolute) against the list + file's own directory. Absolute paths are returned unchanged everywhere. + + If `Path.cwd()` itself raises (e.g. the working directory was deleted out from under + the process) and at least one `file`/`items_to_process` entry is relative, there is + nothing to anchor it against: this is fatal, reporting every offending relative name + in one error message and returning `(None, 2)`. Absolute `file`/`items_to_process` + entries, and all `--list` entries (which never anchor to cwd), are unaffected by an + undeterminable cwd. Args: file (str): Single file to process. filelist (str): File containing list of files to process. items_to_process (list): List of files to process. - inputdata_root (Path): Root of the inputdata tree, used to decide whether cwd is inside - it and, if so, to anchor relative CLI-arg names against cwd. Returns: list: List of files to process int: Result code """ - root_resolved = Path(inputdata_root).expanduser().resolve() - try: - cwd = Path.cwd().resolve() - cwd_inside = cwd.is_relative_to(root_resolved) - except OSError: - # cwd may have been deleted out from under the process (FileNotFoundError, itself an - # OSError). We can't anchor to a cwd we can't determine, so treat this the same as - # "cwd is outside the tree": fall back to the pre-existing root-relative behavior - # instead of letting the exception propagate. - cwd = None - cwd_inside = False - cli_args = ([file] if file is not None else []) + list(items_to_process or []) - if any(not Path(name).is_absolute() for name in cli_args): - logger.warning( + cli_args = ([file] if file is not None else []) + list(items_to_process or []) + relative_cli_args = [name for name in cli_args if not Path(name).is_absolute()] + + cwd = None + if relative_cli_args: + try: + cwd = Path.cwd().resolve() + except OSError: + # cwd may have been deleted out from under the process (FileNotFoundError, itself + # an OSError). There is no root fallback to anchor a relative name against instead, + # so this is fatal: report every offending name in one error message. + logger.error( "rimport: could not determine the current working directory (it may have " - "been deleted); relative --file/positional names will be resolved against " - "the inputdata root instead of cwd" + "been deleted); cannot resolve relative name(s): %s", + ", ".join(relative_cli_args), ) + return None, 2 def _anchor_cli(name): if Path(name).is_absolute(): return name - if cwd_inside: - return str(cwd / name) # strict: NO fallback to root - return name # legacy: normalize_paths joins onto root later + return str(cwd / name) - if file is not None: - files_to_process = [_anchor_cli(file)] - else: - files_to_process = [] + files_to_process = [_anchor_cli(file)] if file is not None else [] if filelist is not None: list_path = Path(filelist).expanduser().resolve() @@ -441,21 +423,11 @@ def get_files_to_process( return None, 2 list_base = list_path.parent - list_inside = list_base.is_relative_to(root_resolved) for entry in files_in_list: if Path(entry).is_absolute(): files_to_process.append(entry) - elif list_inside: - files_to_process.append(str(list_base / entry)) else: - logger.error( - "rimport: relative path '%s' not allowed in list file outside " - "the inputdata tree: %s; use absolute paths or move the list " - "file into the inputdata tree", - entry, - list_path, - ) - return None, 2 + files_to_process.append(str(list_base / entry)) if items_to_process: files_to_process.extend(_anchor_cli(item) for item in items_to_process) @@ -510,7 +482,7 @@ def main(argv: List[str] | None = None) -> int: # Determine the list of relative filenames to handle files_to_process, status = get_files_to_process( - args.file, args.filelist, args.items_to_process, root + args.file, args.filelist, args.items_to_process ) if status: return status diff --git a/tests/rimport/test_cmdline.py b/tests/rimport/test_cmdline.py index 5e3cdce..29d381d 100644 --- a/tests/rimport/test_cmdline.py +++ b/tests/rimport/test_cmdline.py @@ -245,49 +245,6 @@ def test_list_inside_tree_relative_entries_anchor_to_list_dir_not_cwd( assert decoy_file.read_text() == "decoy data" assert not (staging_root / "atm" / "clm2" / "file1.nc").exists() - def test_list_outside_tree_relative_entry_error( - self, rimport_script, test_env, rimport_env - ): - """Test that a relative entry in a list file outside the tree is a fatal error.""" - inputdata_root = test_env["inputdata_root"] - staging_root = test_env["staging_root"] - tmp_path = test_env["tmp_path"] - - # Create the file that would be staged if this succeeded - test_file = inputdata_root / "file1.nc" - test_file.write_text("data1") - - # Create filelist OUTSIDE the tree with a relative entry - filelist = tmp_path / "filelist.txt" - filelist.write_text("file1.nc\n") - - # Run rimport with -list option - command = [ - sys.executable, - rimport_script, - "-list", - str(filelist), - "-inputdata", - str(inputdata_root), - ] - - result = subprocess.run( - command, - capture_output=True, - text=True, - check=False, - env=rimport_env, - ) - - # Verify fatal error, naming both the offending entry and the list file - assert result.returncode == 2 - assert "file1.nc" in result.stderr - assert str(filelist.resolve()) in result.stderr - - # Verify nothing was staged or symlinked - assert not (staging_root / "file1.nc").exists() - assert not test_file.is_symlink() - def test_preserves_directory_structure(self, rimport_script, test_env, rimport_env): """Test that directory structure is preserved in staging.""" inputdata_root = test_env["inputdata_root"] @@ -755,17 +712,28 @@ def test_relative_file_from_subdir_missing_errors_no_root_fallback( # Verify nothing was staged assert not any(staging_root.iterdir()) - def test_relative_file_from_outside_tree_resolves_against_root( + def test_relative_file_from_outside_tree_errors_no_root_fallback( self, rimport_script, test_env, rimport_env ): - """Test that a relative positional filename still resolves against the inputdata root, - as before, when rimport is run from outside the inputdata tree.""" + """Test that a relative positional filename run from OUTSIDE the inputdata tree anchors + to cwd and errors, rather than falling back to a same-named file at the inputdata root. + + This is the configuration the deleted root-fallback actually operated in: with cwd + inside the tree the old dual-mode code already anchored to cwd, so the sibling tests + above would have passed against it unmodified. Only an outside-the-tree cwd + discriminates the old behavior (silently stage the root file, rc 0) from the new one + (error, stage nothing). The decoy is what makes it discriminating: without a file at + the root-anchored path there would be nothing for a regression to wrongly publish. + """ inputdata_root = test_env["inputdata_root"] staging_root = test_env["staging_root"] tmp_path = test_env["tmp_path"] - test_file = inputdata_root / "test.nc" - test_file.write_text("root data") + outside = tmp_path / "outside" + outside.mkdir() + + decoy_file = inputdata_root / "test.nc" + decoy_file.write_text("decoy data") # Run rimport with a relative positional filename, from outside the tree command = [ @@ -782,20 +750,19 @@ def test_relative_file_from_outside_tree_resolves_against_root( text=True, check=False, env=rimport_env, - cwd=tmp_path, + cwd=outside, ) - # Verify success - assert result.returncode == 0, f"Command failed: {result.stderr}" + # Verify failure. Deliberately not pinning the exact code: pre-flight validation + # (a later task) shifts this class of user error from 1 to 2. + assert result.returncode != 0, f"Command unexpectedly passed: {result.stdout}" - # Verify the file was staged, resolved against the inputdata root - staged_file = staging_root / "test.nc" - assert staged_file.exists() - assert staged_file.read_text() == "root data" + # Verify the decoy at the inputdata root was NOT published + assert not decoy_file.is_symlink() + assert decoy_file.read_text() == "decoy data" - # Verify file was relinked - assert test_file.is_symlink() - assert test_file.resolve() == staged_file + # Verify nothing was staged + assert not any(staging_root.iterdir()) def test_dotdot_escape_from_subdir_errors( self, rimport_script, test_env, rimport_env diff --git a/tests/rimport/test_get_files_to_process.py b/tests/rimport/test_get_files_to_process.py index d3ad127..b6a90fa 100644 --- a/tests/rimport/test_get_files_to_process.py +++ b/tests/rimport/test_get_files_to_process.py @@ -39,20 +39,19 @@ def test_single_file_relpath(self, tmp_path, monkeypatch): test_file = inputdata_root / filename test_file.write_text("abc123") - # cwd outside the inputdata tree: relative name stays unanchored - monkeypatch.chdir(tmp_path) + # A relative name always anchors to cwd + monkeypatch.chdir(inputdata_root) # Run files_to_process, result = rimport.get_files_to_process( file=filename, filelist=None, items_to_process=None, - inputdata_root=inputdata_root, ) # Verify assert result == 0 - assert files_to_process == [filename] + assert files_to_process == [str(inputdata_root.resolve() / filename)] def test_single_file_abspath(self, tmp_path): """Test giving it a single file by its absolute path""" @@ -71,74 +70,12 @@ def test_single_file_abspath(self, tmp_path): file=test_file, filelist=None, items_to_process=None, - inputdata_root=inputdata_root, ) # Verify assert result == 0 assert files_to_process == [test_file] - def test_filelist_relpath_with_relpaths(self, tmp_path): - """Test giving it a file list (outside tree) by its relative path, containing relative - paths: fatal error, since the list file is outside the inputdata tree""" - # Setup - inputdata_root = tmp_path / "inputdata" - inputdata_root.mkdir() - staging_root = tmp_path / "staging" - staging_root.mkdir() - - filenames = [] - for i in range(2): - filename = f"test{i}.txt" - filenames.append(filename) - (inputdata_root / filename).write_text("def567") - - filelist = tmp_path / "file_list.txt" - filelist.write_text("\n".join(filenames), encoding="utf8") - filelist_relpath = os.path.relpath(filelist) - - # Run - files_to_process, result = rimport.get_files_to_process( - file=None, - filelist=filelist_relpath, - items_to_process=None, - inputdata_root=inputdata_root, - ) - - # Verify - assert result == 2 - assert files_to_process is None - - def test_filelist_abspath_with_relpaths(self, tmp_path): - """Test giving it a file list (outside tree) by its absolute path, containing relative - paths: fatal error, since the list file is outside the inputdata tree""" - # Setup - inputdata_root = tmp_path / "inputdata" - inputdata_root.mkdir() - staging_root = tmp_path / "staging" - staging_root.mkdir() - - filenames = [] - for i in range(2): - filename = f"test{i}.txt" - filenames.append(filename) - (inputdata_root / filename).write_text("def567") - - filelist = tmp_path / "file_list.txt" - filelist.write_text("\n".join(filenames), encoding="utf8") - - # Run - files_to_process, result = rimport.get_files_to_process( - file=None, - filelist=filelist, - items_to_process=None, - inputdata_root=inputdata_root, - ) - - # Verify - assert result == 2 - assert files_to_process is None - def test_filelist_relpath_with_abspaths(self, tmp_path): """Test giving it a file list by its relative path, containing absolute paths""" # Setup @@ -162,7 +99,6 @@ def test_filelist_relpath_with_abspaths(self, tmp_path): file=None, filelist=filelist_relpath, items_to_process=None, - inputdata_root=inputdata_root, ) # Verify @@ -191,7 +127,6 @@ def test_filelist_abspath_with_abspaths(self, tmp_path): file=None, filelist=filelist, items_to_process=None, - inputdata_root=inputdata_root, ) # Verify @@ -215,7 +150,6 @@ def test_list_inside_tree_relative_entries_anchored_to_list_dir(self, tmp_path): file=None, filelist=filelist, items_to_process=None, - inputdata_root=inputdata_root, ) # Verify @@ -257,7 +191,6 @@ def test_list_relative_entries_anchor_to_list_dir_not_cwd(self, tmp_path, monkey file=None, filelist=filelist, items_to_process=None, - inputdata_root=inputdata_root, ) # Verify @@ -280,7 +213,6 @@ def test_list_at_root_relative_entries_anchored_to_root(self, tmp_path): file=None, filelist=filelist, items_to_process=None, - inputdata_root=inputdata_root, ) # Verify @@ -288,29 +220,32 @@ def test_list_at_root_relative_entries_anchored_to_root(self, tmp_path): root_resolved = inputdata_root.resolve() assert files_to_process == [str(root_resolved / f) for f in filenames] - def test_list_outside_tree_relative_entry_errors(self, tmp_path, caplog): - """Test that a relative entry in a list file outside the tree is a fatal error""" + def test_list_outside_tree_relative_entry_anchors_to_list_dir(self, tmp_path, monkeypatch): + """Test that a relative entry in a list file OUTSIDE the tree now anchors to the list + file's own directory instead of erroring (the deleted root-fallback used to make this a + fatal error). Discriminating setup: cwd is neither the list dir nor the inputdata root, + so the assertion can distinguish list-dir-anchoring from cwd-anchoring and from the + (now-impossible) root-anchoring.""" # Setup - inputdata_root = tmp_path / "inputdata" - inputdata_root.mkdir() - - filelist = tmp_path / "filelist.txt" + list_dir = tmp_path / "outside" / "listdir" + list_dir.mkdir(parents=True) + filelist = list_dir / "filelist.txt" filelist.write_text("relative_file.nc\n", encoding="utf8") + elsewhere = tmp_path / "outside" / "elsewhere" + elsewhere.mkdir(parents=True) + monkeypatch.chdir(elsewhere) + # Run files_to_process, result = rimport.get_files_to_process( file=None, filelist=filelist, items_to_process=None, - inputdata_root=inputdata_root, ) # Verify - assert result == 2 - assert files_to_process is None - assert "relative_file.nc" in caplog.text - assert str(filelist.resolve()) in caplog.text - assert "use absolute paths or move the list file" in caplog.text + assert result == 0 + assert files_to_process == [str((list_dir / "relative_file.nc").resolve())] def test_list_outside_tree_absolute_entries_ok(self, tmp_path): """Test that a list file outside the tree still works when all entries are absolute""" @@ -332,7 +267,6 @@ def test_list_outside_tree_absolute_entries_ok(self, tmp_path): file=None, filelist=filelist, items_to_process=None, - inputdata_root=inputdata_root, ) # Verify @@ -341,32 +275,24 @@ def test_list_outside_tree_absolute_entries_ok(self, tmp_path): def test_filelist_not_found(self, tmp_path): """Test giving it a file list that doesn't exist""" - inputdata_root = tmp_path / "inputdata" - inputdata_root.mkdir() - filelist = "bsfearirn" assert not os.path.exists(filelist) files_to_process, result = rimport.get_files_to_process( file=None, filelist=filelist, items_to_process=None, - inputdata_root=inputdata_root, ) assert result == 2 assert files_to_process is None def test_filelist_empty(self, tmp_path): """Test giving it an empty file list""" - inputdata_root = tmp_path / "inputdata" - inputdata_root.mkdir() - filelist = tmp_path / "bsfearirn" filelist.write_text("") files_to_process, result = rimport.get_files_to_process( file=None, filelist=filelist, items_to_process=[], - inputdata_root=inputdata_root, ) assert result == 2 assert files_to_process is None @@ -390,7 +316,6 @@ def test_items_to_process_abspaths(self, tmp_path): file=None, filelist=None, items_to_process=filenames, - inputdata_root=inputdata_root, ) # Verify @@ -409,20 +334,19 @@ def test_items_to_process_relpaths(self, tmp_path, monkeypatch): filenames.append(os.path.basename(filename)) filename.write_text("def567") - # cwd outside the inputdata tree: relative names stay unanchored - monkeypatch.chdir(tmp_path) + # A relative name always anchors to cwd + monkeypatch.chdir(inputdata_root) # Run files_to_process, result = rimport.get_files_to_process( file=None, filelist=None, items_to_process=filenames, - inputdata_root=inputdata_root, ) # Verify assert result == 0 - assert files_to_process == filenames + assert files_to_process == [str(inputdata_root.resolve() / f) for f in filenames] def test_items_to_process_mixpaths(self, tmp_path, monkeypatch): """Test giving it a list of absolute and relative paths in items_to_process""" @@ -441,31 +365,38 @@ def test_items_to_process_mixpaths(self, tmp_path, monkeypatch): filename.write_text("def567") assert len(filenames) == 4 - # cwd outside the inputdata tree: relative names stay unanchored - monkeypatch.chdir(tmp_path) + # A relative name always anchors to cwd; absolute names are unaffected + monkeypatch.chdir(inputdata_root) # Run files_to_process, result = rimport.get_files_to_process( file=None, filelist=None, items_to_process=filenames, - inputdata_root=inputdata_root, ) # Verify assert result == 0 - assert files_to_process == filenames + assert files_to_process == [str(inputdata_root.resolve() / f) for f in filenames[:2]] + ( + filenames[2:] + ) def test_single_file_and_list(self, tmp_path, monkeypatch): - """Test giving it a single file by its relative path""" + """Test giving it a single file by its relative path together with a list file. + Discriminating setup: cwd is a SUBDIRECTORY of inputdata_root, distinct from the list + file's own directory (inputdata_root itself), so the test pins cwd-anchoring for the + relative `file` and list-dir-anchoring for the list entries at once, and can tell the + two apart.""" # Setup inputdata_root = tmp_path / "inputdata" inputdata_root.mkdir() staging_root = tmp_path / "staging" staging_root.mkdir() + subdir = inputdata_root / "sub" + subdir.mkdir() filename = "test.nc" - test_file = inputdata_root / filename + test_file = subdir / filename test_file.write_text("abc123") filenames = [] @@ -477,34 +408,29 @@ def test_single_file_and_list(self, tmp_path, monkeypatch): filelist = inputdata_root / "file_list.txt" filelist.write_text("\n".join(filenames), encoding="utf8") - # cwd outside the inputdata tree: relative `file` name stays unanchored - monkeypatch.chdir(tmp_path) + # cwd is inside the tree, but at the subdir, not the list file's own directory + monkeypatch.chdir(subdir) # Run files_to_process, result = rimport.get_files_to_process( file=filename, filelist=filelist, items_to_process=None, - inputdata_root=inputdata_root, ) # Verify assert result == 0 - assert files_to_process == [filename] + [ + assert files_to_process == [str(subdir.resolve() / filename)] + [ str(inputdata_root.resolve() / f) for f in filenames ] def test_single_or_filelist_or_list_required(self, tmp_path): """Test that at least one of file, filelist, items_to_process is required""" - inputdata_root = tmp_path / "inputdata" - inputdata_root.mkdir() - # Run files_to_process, result = rimport.get_files_to_process( file=None, filelist=None, items_to_process=None, - inputdata_root=inputdata_root, ) # Verify @@ -526,7 +452,6 @@ def test_cli_file_relative_cwd_inside_tree(self, tmp_path, monkeypatch): file=filename, filelist=None, items_to_process=None, - inputdata_root=inputdata_root, ) # Verify @@ -548,17 +473,16 @@ def test_cli_items_relative_cwd_inside_tree(self, tmp_path, monkeypatch): file=None, filelist=None, items_to_process=filenames, - inputdata_root=inputdata_root, ) # Verify assert result == 0 assert files_to_process == [str(cwd / f) for f in filenames] - def test_cli_relative_cwd_outside_tree_unchanged(self, tmp_path, monkeypatch): - """Test that a relative --file name is left unchanged when cwd is outside the tree""" - inputdata_root = tmp_path / "inputdata" - inputdata_root.mkdir() + def test_cli_relative_cwd_outside_tree_still_anchors_to_cwd(self, tmp_path, monkeypatch): + """Test that a relative --file name anchors to cwd even when cwd is outside the + inputdata tree: there is no inside/outside distinction any more, one rule applies + everywhere, with no root-relative fallback.""" outside = tmp_path / "outside" outside.mkdir() monkeypatch.chdir(outside) @@ -570,12 +494,11 @@ def test_cli_relative_cwd_outside_tree_unchanged(self, tmp_path, monkeypatch): file=filename, filelist=None, items_to_process=None, - inputdata_root=inputdata_root, ) # Verify assert result == 0 - assert files_to_process == [filename] + assert files_to_process == [str(outside.resolve() / filename)] def test_cli_cwd_equals_root_anchors_to_root(self, tmp_path, monkeypatch): """Test that cwd == inputdata root counts as inside the tree""" @@ -591,7 +514,6 @@ def test_cli_cwd_equals_root_anchors_to_root(self, tmp_path, monkeypatch): file=filename, filelist=None, items_to_process=None, - inputdata_root=inputdata_root, ) # Verify @@ -612,7 +534,6 @@ def test_cli_absolute_unchanged_cwd_inside_tree(self, tmp_path, monkeypatch): file=abs_file, filelist=None, items_to_process=None, - inputdata_root=inputdata_root, ) # Verify @@ -636,7 +557,6 @@ def test_cli_cwd_inside_tree_via_symlink(self, tmp_path, monkeypatch): file=filename, filelist=None, items_to_process=None, - inputdata_root=inputdata_root, ) # Verify @@ -663,7 +583,6 @@ def test_deleted_cwd_with_absolute_names_still_works(self, tmp_path, monkeypatch file=abs_file, filelist=None, items_to_process=None, - inputdata_root=inputdata_root, ) # Verify @@ -672,14 +591,11 @@ def test_deleted_cwd_with_absolute_names_still_works(self, tmp_path, monkeypatch # No warning: the cwd being undeterminable doesn't matter for absolute names assert "working directory" not in caplog.text.lower() - def test_deleted_cwd_with_relative_names_falls_back_to_root( - self, tmp_path, monkeypatch, caplog - ): - """Test that a deleted cwd does not raise for relative names either: since cwd can't be - determined, relative names are left unanchored for normalize_paths to later resolve - against inputdata_root (the pre-existing legacy behavior), rather than raising.""" - inputdata_root = tmp_path / "inputdata" - inputdata_root.mkdir() + def test_deleted_cwd_with_relative_name_errors(self, tmp_path, monkeypatch, caplog): + """Test that a deleted cwd is now a FATAL error for relative names, rather than falling + back to root-relative resolution: with no fallback, there is nothing left to anchor a + relative name against. Confirms rc 2, files_to_process is None, and that the error + message names the offending relative name.""" filename = "test.nc" deleted_dir = tmp_path / "deleted" @@ -688,16 +604,44 @@ def test_deleted_cwd_with_relative_names_falls_back_to_root( deleted_dir.rmdir() # Run - with caplog.at_level(logging.WARNING): + with caplog.at_level(logging.ERROR): files_to_process, result = rimport.get_files_to_process( file=filename, filelist=None, items_to_process=None, - inputdata_root=inputdata_root, ) # Verify - assert result == 0 - assert files_to_process == [filename] - # Relative names ARE affected (resolved against the root, not cwd, once staged) -- warn + assert result == 2 + assert files_to_process is None + assert filename in caplog.text assert "working directory" in caplog.text.lower() + + def test_deleted_cwd_with_multiple_relative_names_reports_all( + self, tmp_path, monkeypatch, caplog + ): + """Test that when a deleted cwd leaves several relative names unresolvable, the error + message names ALL of them, not just the first -- Sam's stated preference is to fail + before doing anything and name every offending input, not just one.""" + filename = "test.nc" + item_names = ["a.txt", "b.txt"] + + deleted_dir = tmp_path / "deleted" + deleted_dir.mkdir() + monkeypatch.chdir(deleted_dir) + deleted_dir.rmdir() + + # Run + with caplog.at_level(logging.ERROR): + files_to_process, result = rimport.get_files_to_process( + file=filename, + filelist=None, + items_to_process=item_names, + ) + + # Verify + assert result == 2 + assert files_to_process is None + assert filename in caplog.text + for item_name in item_names: + assert item_name in caplog.text diff --git a/tests/rimport/test_main.py b/tests/rimport/test_main.py index 604d71e..9837a67 100644 --- a/tests/rimport/test_main.py +++ b/tests/rimport/test_main.py @@ -55,11 +55,15 @@ def test_single_file_success( mock_normalize_paths.return_value = [test_file] # Run - result = rimport.main(["-file", "test.nc", "-inputdata", str(inputdata_root)]) + # Absolute -file removes cwd coupling entirely: get_files_to_process runs for real here + # (unmocked), and a relative name would anchor to the pytest invocation dir, not tmp_path. + result = rimport.main( + ["-file", str(test_file), "-inputdata", str(inputdata_root)] + ) # Verify assert result == 0 - mock_normalize_paths.assert_called_once_with(inputdata_root, ["test.nc"]) + mock_normalize_paths.assert_called_once_with(inputdata_root, [str(test_file)]) check = False mock_stage_data.assert_called_once_with( test_file, inputdata_root, staging_root, check From 92739b802a5a8f946227f073c51be8ed60a99fb9 Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Tue, 25 Aug 2026 18:18:41 -0600 Subject: [PATCH 15/21] rimport: Add pre-flight validation so a bad path aborts before publishing main() previously looped over the resolved paths calling stage_data, counting failures and continuing, so a batch containing one typo'd filename published some files and then exited 1. Validate every path first instead: if any fail, report all of them and exit 2 without touching the tree. Extract stage_data's read-only guardrails into validate_source_path(), which returns the exception unraised rather than raising it. stage_data raises whatever comes back, so its exception types and message text are unchanged (test_stage_data.py is untouched); main's gate calls the same function, so the two can't drift apart about what is stageable. A live symlink whose target resolves under staging_root validates as OK, not as a failure: that is the normal state of an already-published file, and treating it as bad would break re-running rimport over a published tree. This settles the exit-code split: 2 means we rejected the input before doing anything, 1 means a genuine runtime failure after work began. Ten e2e tests shift from 1 to 2 as a result; only one pinned the exact code. The gate deliberately covers --check too, so one bad entry aborts the batch rather than being reported per-file. Co-Authored-By: Claude Sonnet 5 Co-Authored-By: Claude Opus 5 (1M context) --- rimport | 116 ++++++++++++---- tests/rimport/test_cmdline.py | 138 ++++++++++++++++++- tests/rimport/test_main.py | 93 +++++++++++++ tests/rimport/test_validate_source_path.py | 146 +++++++++++++++++++++ 4 files changed, 463 insertions(+), 30 deletions(-) create mode 100644 tests/rimport/test_validate_source_path.py diff --git a/rimport b/rimport index 4189005..a4eeb03 100755 --- a/rimport +++ b/rimport @@ -169,6 +169,67 @@ def check_relink_worked(src: Path, dst: Path) -> None: raise RuntimeError("Error relinking during rimport") +def validate_source_path( + src: Path, inputdata_root: Path, staging_root: Path +) -> Exception | None: + """Run stage_data's read-only guardrails against `src` without raising or logging. + + This is the pre-flight half of `stage_data`'s checks: broken symlink, live symlink whose + target is outside staging, missing file, directory, outside the inputdata root, and already + under the staging directory. It performs no I/O beyond stat-ing `src` and its resolved + target — in particular it never makes a network call — so it is cheap to run over an entire + batch before anything is staged. + + Critically, a *live* symlink whose target resolves under `staging_root` is NOT a failure: + that is the normal state of a file that has already been published and linked by a previous + run, and is treated here exactly like a plain, stageable file (returns `None`). Re-running + rimport over an already-published tree must not report every published file as bad. + + `stage_data` calls this first and raises whatever it returns; `main`'s pre-flight gate calls + it over every resolved path before staging anything, so a bad path anywhere in the batch is + reported without partially publishing the rest. + + Args: + src: Source file path to validate. + inputdata_root: Root directory of the inputdata tree. + staging_root: Root directory where files will be staged. + + Returns: + The exception (unraised) describing why `src` cannot be staged, or `None` if `src` is + fine to proceed with — including the already-published-and-linked symlink case. + """ + if src.is_symlink(): + if not os.path.exists(src.resolve()): + return RuntimeError(f"Source is a broken symlink: {src}") + if not src.resolve().is_relative_to(staging_root.resolve()): + return RuntimeError( + f"Source is a symlink, but target ({src.resolve()}) is outside staging directory " + f"({staging_root})" + ) + # Live symlink already resolving under staging_root: already published and linked. + # This is a legitimate no-op, not a validation failure. + return None + + if not src.exists(): + return FileNotFoundError(f"source not found: {src}") + + if src.is_dir(): + return RuntimeError(f"source is a directory, not a file: {src}") + + try: + src.resolve().relative_to(inputdata_root.resolve()) + except ValueError: + if src.resolve().is_relative_to(staging_root.resolve()): + return RuntimeError( + f"Source file '{src.name}' is already under staging directory '{staging_root}'." + ) + return RuntimeError( + f"source not under inputdata root: {src} not in {inputdata_root}" + ) + + return None + + def stage_data( src: Path, inputdata_root: Path, staging_root: Path, check: bool = False ) -> None: @@ -205,38 +266,26 @@ def stage_data( this guardrail: it is handled by the live-symlink guardrails above instead, which for a target under `staging_root` log "already published and linked" and return without raising, and for a target outside `staging_root` raise (see the first guardrail above). + + These read-only guardrails are delegated to `validate_source_path`, which returns the + exception to raise (or None); this function raises whatever comes back. That keeps this + function and `main`'s pre-flight gate in permanent agreement about what is stageable, + since both call the same check. """ + error = validate_source_path(src, inputdata_root, staging_root) + if error is not None: + raise error + if src.is_symlink(): - if not os.path.exists(src.resolve()): - raise RuntimeError(f"Source is a broken symlink: {src}") - if not src.resolve().is_relative_to(staging_root.resolve()): - raise RuntimeError( - f"Source is a symlink, but target ({src.resolve()}) is outside staging directory " - f"({staging_root})" - ) + # validate_source_path only lets a symlink through here when it is live and its target + # resolves under staging_root: the "already published and linked" case. logger.info("%sFile is already published and linked.", INDENT) print_can_file_be_downloaded( can_file_be_downloaded(src.resolve(), staging_root) ) return - if not src.exists(): - raise FileNotFoundError(f"source not found: {src}") - - if src.is_dir(): - raise RuntimeError(f"source is a directory, not a file: {src}") - - try: - rel = src.resolve().relative_to(inputdata_root.resolve()) - except ValueError as exc: - if src.resolve().is_relative_to(staging_root.resolve()): - raise RuntimeError( - f"Source file '{src.name}' is already under staging directory '{staging_root}'." - ) from exc - raise RuntimeError( - f"source not under inputdata root: {src} not in {inputdata_root}" - ) from exc - + rel = src.resolve().relative_to(inputdata_root.resolve()) dst = staging_root / rel if dst.exists(): @@ -490,6 +539,25 @@ def main(argv: List[str] | None = None) -> int: # Resolve to full paths (keep accepting absolute names too) paths = normalize_paths(root, files_to_process) staging_root = get_staging_root() + + # Pre-flight: validate every path before staging anything, so a batch containing a bad + # path is reported (all bad paths at once) instead of half-completing. Runs for --check + # too: a bad entry aborts the whole batch rather than being reported per-file. + failures = [] + for p in paths: + error = validate_source_path(p, root, staging_root) + if error is not None: + failures.append((p, error)) + if failures: + logger.error( + "rimport: %d of %d file(s) failed pre-flight validation; nothing was published:", + len(failures), + len(paths), + ) + for p, error in failures: + logger.error("%srimport: '%s': %s", INDENT, p, error) + return 2 + # Execute the new action per file errors = 0 for p in paths: diff --git a/tests/rimport/test_cmdline.py b/tests/rimport/test_cmdline.py index 29d381d..c24cb97 100644 --- a/tests/rimport/test_cmdline.py +++ b/tests/rimport/test_cmdline.py @@ -307,9 +307,12 @@ def test_error_for_nonexistent_file(self, rimport_script, test_env, rimport_env) env=rimport_env, ) - # Verify error + # Verify error. Assert the actual reason, not the substring "error": pre-flight + # reports "N of M file(s) failed pre-flight validation", which contains no such + # word, so a bare "error" check is satisfied only by tmp_path echoing this test's + # own name back in the offending path. assert result.returncode != 0 - assert "error" in result.stderr.lower() + assert "source not found" in result.stderr def test_error_for_nonexistent_list_file( self, rimport_script, test_env, rimport_env @@ -810,8 +813,8 @@ def test_dotdot_escape_from_list_inside_tree_errors( self, rimport_script, test_env, rimport_env ): """Test that a '..'-escaping relative entry in a list file INSIDE the tree is rejected - by stage_data's existing outside-the-root guardrail. This is the list-side twin of - test_dotdot_escape_from_subdir_errors above.""" + by validate_source_path's outside-the-root guardrail, via main()'s pre-flight gate. + This is the list-side twin of test_dotdot_escape_from_subdir_errors above.""" inputdata_root = test_env["inputdata_root"] staging_root = test_env["staging_root"] tmp_path = test_env["tmp_path"] @@ -846,8 +849,9 @@ def test_dotdot_escape_from_list_inside_tree_errors( env=rimport_env, ) - # Verify failure - assert result.returncode == 1, f"Command unexpectedly passed: {result.stdout}" + # Verify failure. rc 2 (not 1): pre-flight validation catches this before anything is + # staged, rather than the per-file loop catching it after some files may have run. + assert result.returncode == 2, f"Command unexpectedly passed: {result.stdout}" assert "not under inputdata root" in result.stderr # Verify nothing was staged @@ -1059,3 +1063,125 @@ def test_check_directory_argument_reports_error_not_publishable( assert subdir.is_dir() and not subdir.is_symlink() assert not list(inputdata_root.rglob("*.tmp")) assert inner_file.read_text() == "clm2 data" + + def test_mixed_validity_list_aborts_and_stages_nothing( + self, rimport_script, test_env, rimport_env + ): + """Test the pre-flight gate end to end: a --list with one valid entry and two entries + that are invalid in DIFFERENT ways (missing, and a directory) aborts the whole batch + with rc 2, reports every failure reason, gets the "N of M" count right, and — the + assertion that matters most — never stages or relinks the valid entry. + + The list file lives OUTSIDE the inputdata tree with absolute entries (this configuration + previously had no end-to-end coverage at all).""" + inputdata_root = test_env["inputdata_root"] + staging_root = test_env["staging_root"] + tmp_path = test_env["tmp_path"] + + valid_file = inputdata_root / "good.nc" + valid_file.write_text("good data") + + missing_file = inputdata_root / "missing.nc" + + bad_dir = inputdata_root / "adir" + bad_dir.mkdir() + + # List file OUTSIDE the tree, with absolute entries. + filelist = tmp_path / "filelist.txt" + filelist.write_text(f"{valid_file}\n{missing_file}\n{bad_dir}\n") + + command = [ + sys.executable, + rimport_script, + "-list", + str(filelist), + "-inputdata", + str(inputdata_root), + ] + + result = subprocess.run( + command, + capture_output=True, + text=True, + check=False, + env=rimport_env, + ) + + # Verify failure: rc 2, all reasons present, correct "N of M" count + assert result.returncode == 2, f"Command unexpectedly passed: {result.stdout}" + assert "2 of 3 file(s) failed pre-flight validation" in result.stderr + assert f"source not found: {missing_file}" in result.stderr + assert f"source is a directory, not a file: {bad_dir}" in result.stderr + + # Verify the valid file was NOT staged and NOT turned into a symlink + assert not (staging_root / "good.nc").exists() + assert not valid_file.is_symlink() + assert valid_file.read_text() == "good data" + + # Verify nothing at all was staged + assert not any(staging_root.rglob("*")) + + def test_check_mode_is_gated_too_and_reports_nothing_for_valid_entry( + self, rimport_script, test_env, rimport_env + ): + """Test that --check is subject to the same pre-flight gate as a real run: a mix of + valid and invalid entries aborts with rc 2 and the valid entry's status is NOT + reported. + + This pins a deliberate design decision (uniform abort, chosen over per-file --check + reporting, even though it means a --check run tells you nothing about the files that + would have been fine) — a future reader should not "fix" this into per-file --check + reporting without first re-litigating that choice with the repo owner.""" + inputdata_root = test_env["inputdata_root"] + staging_root = test_env["staging_root"] + tmp_path = test_env["tmp_path"] + + valid_file = inputdata_root / "good.nc" + valid_file.write_text("good data") + + missing_file = inputdata_root / "missing.nc" + + bad_dir = inputdata_root / "adir" + bad_dir.mkdir() + + # List file OUTSIDE the tree, with absolute entries. + filelist = tmp_path / "filelist.txt" + filelist.write_text(f"{valid_file}\n{missing_file}\n{bad_dir}\n") + + # Make sure --check skips ensure_running_as() + del rimport_env["RIMPORT_SKIP_USER_CHECK"] + + command = [ + sys.executable, + rimport_script, + "-list", + str(filelist), + "-inputdata", + str(inputdata_root), + "--check", + ] + + result = subprocess.run( + command, + capture_output=True, + text=True, + check=False, + env=rimport_env, + ) + + # Verify failure: rc 2, all reasons present + assert result.returncode == 2, f"Command unexpectedly passed: {result.stdout}" + assert "2 of 3 file(s) failed pre-flight validation" in result.stderr + assert f"source not found: {missing_file}" in result.stderr + assert f"source is a directory, not a file: {bad_dir}" in result.stderr + + # Verify the valid entry's check status is NOT reported: --check never gets to run + # per-file, so neither the "already published" nor "not already published" messages + # appear anywhere, for any file. + assert "already published" not in result.stdout.lower() + assert "not already published" not in result.stdout.lower() + assert result.stdout == "" + + # Verify nothing was staged + assert not any(staging_root.rglob("*")) + assert not valid_file.is_symlink() diff --git a/tests/rimport/test_main.py b/tests/rimport/test_main.py index 9837a67..04ba444 100644 --- a/tests/rimport/test_main.py +++ b/tests/rimport/test_main.py @@ -30,6 +30,7 @@ class TestMain: """Test suite for main() function.""" + @patch.object(rimport, "validate_source_path") @patch.object(rimport, "stage_data") @patch.object(rimport, "get_staging_root") @patch.object(rimport, "normalize_paths") @@ -40,6 +41,7 @@ def test_single_file_success( mock_normalize_paths, mock_get_staging_root, mock_stage_data, + mock_validate_source_path, tmp_path, caplog, ): @@ -53,6 +55,9 @@ def test_single_file_success( mock_get_staging_root.return_value = staging_root test_file = inputdata_root / "test.nc" mock_normalize_paths.return_value = [test_file] + # test_file doesn't exist on disk; bypass pre-flight so this test still exercises only + # main()'s control flow, not validate_source_path's real checks. + mock_validate_source_path.return_value = None # Run # Absolute -file removes cwd coupling entirely: get_files_to_process runs for real here @@ -70,6 +75,7 @@ def test_single_file_success( ) assert "No need to run relink.py" in caplog.text + @patch.object(rimport, "validate_source_path") @patch.object(rimport, "stage_data") @patch.object(rimport, "get_staging_root") @patch.object(rimport, "normalize_paths") @@ -82,6 +88,7 @@ def test_file_list_success( mock_normalize_paths, mock_get_staging_root, mock_stage_data, + mock_validate_source_path, tmp_path, ): """Test main() logic flow when a file list stages successfully.""" @@ -98,6 +105,9 @@ def test_file_list_success( file1 = inputdata_root / "file1.nc" file2 = inputdata_root / "file2.nc" mock_normalize_paths.return_value = [file1, file2] + # Neither file exists on disk; bypass pre-flight so this test still exercises only + # main()'s control flow, not validate_source_path's real checks. + mock_validate_source_path.return_value = None # Run result = rimport.main( @@ -123,6 +133,7 @@ def test_file_list_success( ] ) + @patch.object(rimport, "validate_source_path") @patch.object(rimport, "stage_data") @patch.object(rimport, "get_staging_root") @patch.object(rimport, "normalize_paths") @@ -133,6 +144,7 @@ def test_stage_data_exception_handling( mock_normalize_paths, _mock_get_staging_root, mock_stage_data, + mock_validate_source_path, tmp_path, capsys, ): @@ -145,6 +157,9 @@ def test_stage_data_exception_handling( file2 = inputdata_root / "file2.nc" file3 = inputdata_root / "file3.nc" mock_normalize_paths.return_value = [file1, file2, file3] + # None of the files exist on disk; bypass pre-flight so this test still exercises only + # main()'s per-file error handling, not validate_source_path's real checks. + mock_validate_source_path.return_value = None # Make stage_data fail for file2 but succeed for others def stage_data_side_effect(src, *_args, **_kwargs): @@ -231,6 +246,7 @@ def test_requires_file_or_filelist(self, _mock_ensure_running_as, tmp_path, caps captured = capsys.readouterr() assert "At least one of --file or --filelist is required" in captured.err + @patch.object(rimport, "validate_source_path") @patch.object(rimport, "stage_data") @patch.object(rimport, "get_staging_root") @patch.object(rimport, "normalize_paths") @@ -241,6 +257,7 @@ def test_check_mode_calls( mock_normalize_paths, mock_get_staging_root, mock_stage_data, + mock_validate_source_path, tmp_path, caplog, ): @@ -253,6 +270,9 @@ def test_check_mode_calls( mock_get_staging_root.return_value = staging_root test_file = inputdata_root / "test.nc" mock_normalize_paths.return_value = [test_file] + # test_file doesn't exist on disk; bypass pre-flight (which runs in --check mode too) + # so this test still exercises only main()'s control flow. + mock_validate_source_path.return_value = None result = rimport.main( ["-file", "test.nc", "-inputdata", str(inputdata_root), "--check"] @@ -269,6 +289,7 @@ def test_check_mode_calls( # Message about relink.py should not have been printed assert "No need to run relink.py" not in caplog.text + @patch.object(rimport, "validate_source_path") @patch.object(rimport, "stage_data") @patch.object(rimport, "get_staging_root") @patch.object(rimport, "normalize_paths") @@ -279,6 +300,7 @@ def test_skip_user_check_env_var( mock_normalize_paths, _mock_get_staging_root, _mock_stage, + mock_validate_source_path, tmp_path, monkeypatch, ): @@ -290,6 +312,9 @@ def test_skip_user_check_env_var( test_file = inputdata_root / "test.nc" mock_normalize_paths.return_value = [test_file] + # test_file doesn't exist on disk; bypass pre-flight so this test still exercises only + # the user-check skip logic. + mock_validate_source_path.return_value = None result = rimport.main(["-file", "test.nc", "-inputdata", str(inputdata_root)]) @@ -297,6 +322,7 @@ def test_skip_user_check_env_var( # ensure_running_as should NOT be called when env var is set mock_ensure_running_as.assert_not_called() + @patch.object(rimport, "validate_source_path") @patch.object(rimport, "stage_data") @patch.object(rimport, "get_staging_root") @patch.object(rimport, "normalize_paths") @@ -307,6 +333,7 @@ def test_prints_file_path_before_processing( mock_normalize_paths, _mock_get_staging_root, _mock_stage, + mock_validate_source_path, tmp_path, capsys, ): @@ -316,6 +343,9 @@ def test_prints_file_path_before_processing( file1 = inputdata_root / "file1.nc" file2 = inputdata_root / "file2.nc" mock_normalize_paths.return_value = [file1, file2] + # Neither file exists on disk; bypass pre-flight so this test still exercises only the + # per-file print, not validate_source_path's real checks. + mock_validate_source_path.return_value = None result = rimport.main(["-file", "test.nc", "-inputdata", str(inputdata_root)]) @@ -325,6 +355,7 @@ def test_prints_file_path_before_processing( assert f"'{file1}':" in captured.out assert f"'{file2}':" in captured.out + @patch.object(rimport, "validate_source_path") @patch.object(rimport, "stage_data") @patch.object(rimport, "get_staging_root") @patch.object(rimport, "normalize_paths") @@ -335,6 +366,7 @@ def test_multiple_errors_returns_1( mock_normalize_paths, _mock_get_staging_root, mock_stage_data, + mock_validate_source_path, tmp_path, ): """Test that main() returns 1 when multiple files fail.""" @@ -345,6 +377,9 @@ def test_multiple_errors_returns_1( file2 = inputdata_root / "file2.nc" file3 = inputdata_root / "file3.nc" mock_normalize_paths.return_value = [file1, file2, file3] + # None of the files exist on disk; bypass pre-flight so this test still exercises the + # per-file loop's runtime-error handling, not validate_source_path's real checks. + mock_validate_source_path.return_value = None # Make all files fail mock_stage_data.side_effect = RuntimeError("Test error") @@ -354,6 +389,7 @@ def test_multiple_errors_returns_1( assert result == 1 assert mock_stage_data.call_count == 3 + @patch.object(rimport, "validate_source_path") @patch.object(rimport, "stage_data") @patch.object(rimport, "get_staging_root") @patch.object(rimport, "normalize_paths") @@ -364,6 +400,7 @@ def test_error_counter_increments_correctly( mock_normalize_paths, _mock_get_staging_root, mock_stage_data, + mock_validate_source_path, tmp_path, capsys, ): @@ -373,6 +410,9 @@ def test_error_counter_increments_correctly( files = [inputdata_root / f"file{i}.nc" for i in range(5)] mock_normalize_paths.return_value = files + # None of the files exist on disk; bypass pre-flight so this test still exercises only + # the per-file error counter, not validate_source_path's real checks. + mock_validate_source_path.return_value = None # Make files 1 and 3 fail def stage_data_side_effect(src, *_args, **_kwargs): @@ -465,3 +505,56 @@ def test_error_if_file_newly_published_but_relink_fails( captured = capsys.readouterr() assert "rimport: error processing" in captured.err assert "Error relinking during rimport" in captured.err + + @patch.object(rimport, "stage_data") + @patch.object(rimport, "get_staging_root") + @patch.object(rimport, "ensure_running_as") + def test_preflight_gate_rejects_whole_batch_and_never_calls_stage_data( + self, + _mock_ensure_running_as, + mock_get_staging_root, + mock_stage_data, + tmp_path, + capsys, + ): + """Test main()'s pre-flight gate: a batch with a mix of valid and invalid paths returns + 2, logs every failure, and never calls stage_data — not even for the valid path. + + Unlike the other main() tests in this file, this one does NOT mock + validate_source_path (or normalize_paths): it lets the real pre-flight gate run + against real files, so it is actually exercising the gate rather than assuming it + works. + """ + inputdata_root = tmp_path / "inputdata" + inputdata_root.mkdir() + staging_root = tmp_path / "staging" + staging_root.mkdir() + mock_get_staging_root.return_value = staging_root + + valid = inputdata_root / "good.nc" + valid.write_text("data") + missing = inputdata_root / "missing.nc" + bad_dir = inputdata_root / "adir" + bad_dir.mkdir() + + result = rimport.main( + [ + "-inputdata", + str(inputdata_root), + str(valid), + str(missing), + str(bad_dir), + ] + ) + + assert result == 2 + mock_stage_data.assert_not_called() + + captured = capsys.readouterr() + assert "2 of 3 file(s) failed pre-flight validation" in captured.err + assert f"source not found: {missing}" in captured.err + assert f"source is a directory, not a file: {bad_dir}" in captured.err + + # The valid file was never staged or turned into a symlink. + assert not (staging_root / "good.nc").exists() + assert not valid.is_symlink() diff --git a/tests/rimport/test_validate_source_path.py b/tests/rimport/test_validate_source_path.py new file mode 100644 index 0000000..7f136b3 --- /dev/null +++ b/tests/rimport/test_validate_source_path.py @@ -0,0 +1,146 @@ +""" +Tests for validate_source_path() function in rimport script. +""" + +import os +import importlib.util +from importlib.machinery import SourceFileLoader + + +# Import rimport module from file without .py extension +rimport_path = os.path.join( + os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__)))), + "rimport", +) +loader = SourceFileLoader("rimport", rimport_path) +spec = importlib.util.spec_from_loader("rimport", loader) +if spec is None: + raise ImportError(f"Could not create spec for rimport from {rimport_path}") +rimport = importlib.util.module_from_spec(spec) +# Don't add to sys.modules to avoid conflict with other test files +loader.exec_module(rimport) + + +def test_ok_regular_file_under_root(tmp_path): + """A valid regular file under the inputdata root returns None.""" + inputdata_root = tmp_path / "inputdata" + inputdata_root.mkdir() + staging_root = tmp_path / "staging" + staging_root.mkdir() + + src = inputdata_root / "file.nc" + src.write_text("data") + + assert rimport.validate_source_path(src, inputdata_root, staging_root) is None + + +def test_ok_already_published_symlink_is_not_a_failure(tmp_path): + """DANGEROUS CASE: a live symlink whose target resolves under staging_root is the normal + state of a file that has already been published and linked by a previous run. This must + return None, not an exception — otherwise re-running rimport over an already-published + tree would report every published file as a pre-flight failure.""" + inputdata_root = tmp_path / "inputdata" + inputdata_root.mkdir() + staging_root = tmp_path / "staging" + staging_root.mkdir() + + real_file = staging_root / "real_file.nc" + real_file.write_text("data") + src = inputdata_root / "link.nc" + src.symlink_to(real_file) + + assert rimport.validate_source_path(src, inputdata_root, staging_root) is None + + +def test_error_broken_symlink(tmp_path): + """A symlink whose target does not exist returns a RuntimeError, unraised.""" + inputdata_root = tmp_path / "inputdata" + inputdata_root.mkdir() + staging_root = tmp_path / "staging" + staging_root.mkdir() + + src = inputdata_root / "broken_link.nc" + src.symlink_to(tmp_path / "nonexistent.nc") + + result = rimport.validate_source_path(src, inputdata_root, staging_root) + assert isinstance(result, RuntimeError) + assert "Source is a broken symlink" in str(result) + + +def test_error_live_symlink_target_outside_staging(tmp_path): + """A live symlink whose target resolves outside staging_root returns a RuntimeError, + unraised.""" + inputdata_root = tmp_path / "inputdata" + inputdata_root.mkdir() + staging_root = tmp_path / "staging" + staging_root.mkdir() + + real_file = tmp_path / "real_file.nc" + real_file.write_text("data") + src = inputdata_root / "link.nc" + src.symlink_to(real_file) + + result = rimport.validate_source_path(src, inputdata_root, staging_root) + assert isinstance(result, RuntimeError) + assert "outside staging directory" in str(result) + + +def test_error_missing_file(tmp_path): + """A path that does not exist returns a FileNotFoundError, unraised.""" + inputdata_root = tmp_path / "inputdata" + inputdata_root.mkdir() + staging_root = tmp_path / "staging" + staging_root.mkdir() + + src = inputdata_root / "nonexistent.nc" + + result = rimport.validate_source_path(src, inputdata_root, staging_root) + assert isinstance(result, FileNotFoundError) + assert "source not found" in str(result) + + +def test_error_directory(tmp_path): + """A directory (not a symlink) returns a RuntimeError, unraised.""" + inputdata_root = tmp_path / "inputdata" + inputdata_root.mkdir() + staging_root = tmp_path / "staging" + staging_root.mkdir() + + src = inputdata_root / "adir" + src.mkdir() + + result = rimport.validate_source_path(src, inputdata_root, staging_root) + assert isinstance(result, RuntimeError) + assert "source is a directory, not a file" in str(result) + + +def test_error_file_outside_inputdata_root(tmp_path): + """A regular file outside the inputdata root returns a RuntimeError, unraised.""" + inputdata_root = tmp_path / "inputdata" + inputdata_root.mkdir() + staging_root = tmp_path / "staging" + staging_root.mkdir() + + src = tmp_path / "outside" / "file.nc" + src.parent.mkdir() + src.write_text("data") + + result = rimport.validate_source_path(src, inputdata_root, staging_root) + assert isinstance(result, RuntimeError) + assert "not under inputdata root" in str(result) + + +def test_error_file_already_under_staging_directory(tmp_path): + """A regular (non-symlink) file already living under staging_root returns a RuntimeError, + unraised.""" + inputdata_root = tmp_path / "inputdata" + inputdata_root.mkdir() + staging_root = tmp_path / "staging" + staging_root.mkdir() + + src = staging_root / "file.nc" + src.write_text("data") + + result = rimport.validate_source_path(src, inputdata_root, staging_root) + assert isinstance(result, RuntimeError) + assert "already under staging directory" in str(result) From bdb433599024f1441c1bb04d4feb886e81dcc831 Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Tue, 25 Aug 2026 18:41:41 -0600 Subject: [PATCH 16/21] docs(rimport): describe the one-rule resolution and pre-flight validation The help strings, README notes, and docstrings still described the two-mode resolution design that this branch deleted, and said nothing about pre-flight validation. Bring them in line with what the code now does. The README's advice was wrong rather than merely stale: it told users to "run from outside the tree, or pass an absolute path, if you want the old root-relative resolution." Running from outside the tree now anchors to that outside directory, so following that advice does not do what it promises -- it fails cleanly with rc 2 ("source not found", or "source not under inputdata root" if a same-named file happens to sit at the cwd-anchored path). The README also carried a "Breaking change" warning about relative entries in an out-of-tree list file erroring; they no longer error. State the pre-flight guarantee with its limit intact: if pre-flight passes, no file fails for a reason pre-flight could have detected, and if it fails nothing was touched -- but it is not a promise that a clean batch finishes, since a runtime failure can still partially complete one. normalize_paths' docstring claimed callers pre-anchor only "inside-tree" names. Every clause of that is now false: get_files_to_process anchors every relative name, so nothing relative reaches normalize_paths on main's call path and its root-join is unreachable there. Say so plainly, and say that the branch is kept deliberately, so nobody reading the function alone concludes root-relative resolution survives somewhere. No logic changes; the suite is unmoved at 287. Co-Authored-By: Claude Sonnet 5 Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 7 ++-- rimport | 62 ++++++++++++++++++++++++----------- tests/rimport/test_cmdline.py | 4 +-- 3 files changed, 50 insertions(+), 23 deletions(-) diff --git a/README.md b/README.md index ff4b9c4..742922f 100644 --- a/README.md +++ b/README.md @@ -10,8 +10,11 @@ Tools used for publishing CESM input data. Notes: - Use `rimport --check` if you'd like to see the current status of a file, including whether it's available for download. - The `relink.py` script was previously used for step 3 above, but that functionality is now built into `rimport`. It's still there if you want to use it by itself. -- A relative filename passed to `rimport` directly (via `--file` or as a positional argument) is resolved against your current directory, not the inputdata root, whenever you run `rimport` from inside the inputdata tree (running from the root itself counts). There's no fallback to the root on a miss, so run from outside the tree, or pass an absolute path, if you want the old root-relative resolution. -- A relative entry in a `--list` file is resolved against that list file's own directory rather than against the inputdata root. **Breaking change:** if the list file lives outside the inputdata tree, relative entries are no longer resolved against the root at all — `rimport` now exits with an error, so list files kept outside the tree must use absolute paths. +- A relative filename passed to `rimport` directly (via `--file` or as a positional argument) is always resolved against your current directory — never against the inputdata root, and it doesn't matter whether you're running from inside or outside the inputdata tree. Pass an absolute path if you want to name a file without regard to your current directory. +- A relative entry in a `--list` file is always resolved against that list file's own directory — again never against the inputdata root, wherever the list file itself lives. Pass absolute entries in the list if you want them independent of the list file's location. +- Before staging anything, `rimport` validates every file it's about to process (all `--file`/`--list`/positional entries together). If any of them fail — missing, a directory, a broken symlink, outside the inputdata root, etc. — none of them are touched, and every failing path is reported at once so you can fix them all in one pass. This is a promise about rejected input, not about success: pre-flight passing doesn't guarantee the whole batch will finish, since a file can still fail later for a reason pre-flight can't see (e.g. a runtime/relink failure partway through). +- `--check` is gated by the same pre-flight validation as a real run: if any file in the batch fails validation, `rimport` reports the failures and exits without checking (or reporting on) any of the other files. This is deliberate, not a bug — fix the bad entries and re-run to see the rest. +- Exit codes: `0` means everything succeeded (or, under `--check`, everything checked cleanly); `2` means the run was rejected before touching anything (bad arguments, a missing/empty list file, or a pre-flight validation failure); `1` means pre-flight passed but something failed for real while actually being staged or relinked. ## Filenames and metadata: diff --git a/rimport b/rimport index a4eeb03..408287e 100755 --- a/rimport +++ b/rimport @@ -53,9 +53,8 @@ def build_parser() -> argparse.ArgumentParser: dest="file", metavar="filename", help=( - "Provide a file to import. Must be in the CESM inputdata directory. Relative names are" - " resolved against the current directory when run from inside the inputdata tree (no" - " fallback to the root if the file isn't found there); otherwise against the" + "Provide a file to import. Must be in the CESM inputdata directory. A relative name" + " is resolved against the current directory; there is no fallback to the" " inputdata root." ), ) @@ -67,9 +66,8 @@ def build_parser() -> argparse.ArgumentParser: metavar="filelist", help=( "Provide a file that contains a list of filenames to import. All filenames in the list" - " must be in the CESM inputdata directory. Relative entries are resolved against the" - " list file's directory; if the list file is outside the inputdata tree, entries must" - " be absolute." + " must be in the CESM inputdata directory. A relative entry is resolved against the" + " list file's own directory, wherever that directory is." ), ) @@ -78,9 +76,8 @@ def build_parser() -> argparse.ArgumentParser: nargs="*", help=( "One or more files to process. (Optional; can use --file instead to process just one.)" - " Relative names are resolved against the current directory when run from inside the" - " inputdata tree (no fallback to the root if the file isn't found there); otherwise" - " against the inputdata root." + " A relative name is resolved against the current directory; there is no fallback to" + " the inputdata root." ), ) @@ -92,7 +89,11 @@ def build_parser() -> argparse.ArgumentParser: "-check", "-c", action="store_true", - help="Check whether file(s) is/are already published.", + help=( + "Check whether file(s) is/are already published, without staging anything. A bad" + " path anywhere in the batch aborts before any file is checked, reporting all bad" + " paths at once." + ), ) # Add verbosity options @@ -131,17 +132,24 @@ def normalize_paths(root: Path, relnames: Iterable[str]) -> List[Path]: """Convert relative or absolute path names to normalized absolute Paths. For each name in relnames: - - If the name is relative, it is assumed to be relative to `root` and made absolute + - If the name is relative, it is joined onto `root` and made absolute. All paths are then normalized to their absolute form, replacing . and .. as needed. - Callers (get_files_to_process) already anchor inside-tree names to cwd or to the list file's - directory before calling this function, so a name that is still relative when it reaches here - is one whose anchor was outside the inputdata tree; the root-join above is what resolves those. + The `root / name` branch above is unreachable from `main`: `get_files_to_process` now + anchors every relative name before returning it -- CLI-style names (`--file`, positional) + against cwd, `--list` entries against the list file's own directory -- so nothing relative + ever reaches this function on `main`'s call path, and `root` is effectively unused there. + This is dead code, kept deliberately rather than removed; do not read this function in + isolation and conclude that root-relative resolution is still reachable anywhere in + `rimport`. The branch is still real code, though: it is exercised directly by this + function's own unit tests in test_normalize_paths.py, which call normalize_paths() with + relative names and a `root` of their choosing. Note that symlinks are NOT resolved. Args: - root: Base directory under which relative paths are assumed to be. + root: Base directory under which relative paths are assumed to be. Only matters for + the unreachable-from-`main` branch described above. relnames: Iterable of path names (relative or absolute) to normalize. Returns: @@ -506,10 +514,26 @@ def main(argv: List[str] | None = None) -> int: RIMPORT_STAGING: Override the default staging root directory. Exit Codes: - 0: All files staged successfully. - 1: One or more files failed to stage or relink (errors printed to stderr). - 2: Fatal error (missing inputdata directory, missing file list, a relative entry in a - --list file whose directory is outside the inputdata tree, etc.). + 0: All files staged successfully (or, under --check, checked without error). + 1: Pre-flight validation passed, but one or more files failed while actually being + staged or relinked -- a genuine runtime failure, not a rejected input (errors + printed to stderr for each). + 2: The run was rejected before any file was staged or checked. Causes: + * argparse rejected the command line itself -- e.g. an unrecognized option, + or an --inputdata-root that does not exist (argparse validates it via + type=, so this fires before any of the code below runs). + * The inputdata root directory does not exist. + * No --file, --list, or positional argument was given. + * The --list file was not found, or contained no filenames. + * A relative --file or positional name could not be anchored, because the + current working directory could not be determined. + * User-switching to STAGE_OWNER failed: target user not found, or no TTY + available for the sudo/2FA prompt. + * Most commonly: pre-flight validation rejected one or more of the resolved + paths (missing file, directory, broken symlink, source outside the + inputdata root, etc.). Pre-flight checks every resolved path before + staging anything and reports every failure at once, so a bad path never + leaves the batch half-published. This gate applies to --check runs too. """ parser = build_parser() args = parser.parse_args(argv) diff --git a/tests/rimport/test_cmdline.py b/tests/rimport/test_cmdline.py index c24cb97..ae31018 100644 --- a/tests/rimport/test_cmdline.py +++ b/tests/rimport/test_cmdline.py @@ -100,7 +100,7 @@ def test_list_option_stages_multiple_files( file1.write_text("data1") file2.write_text("data2") - # Create filelist (outside the tree: entries must be absolute) + # Create filelist (outside the tree; absolute entries) filelist = tmp_path / "filelist.txt" filelist.write_text(f"{file1}\n{file2}\n") @@ -404,7 +404,7 @@ def test_list_with_comments_and_blanks(self, rimport_script, test_env, rimport_e file1.write_text("data1") file2.write_text("data2") - # Create filelist with comments and blanks (outside the tree: entries must be absolute) + # Create filelist with comments and blanks (outside the tree; absolute entries) filelist = tmp_path / "filelist.txt" filelist.write_text(f"# Comment\n{file1}\n\n# Another comment\n{file2}\n") From 03866de3c0867f6f25aba911906b3308f453cc56 Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Mon, 31 Aug 2026 10:54:48 -0600 Subject: [PATCH 17/21] tests: bring nested_mock_dirs fixture in shape with fixture_temp_dirs Finishes the review-requested move of fixture_nested_mock_dirs from tests/relink/test_cmdline.py into tests/conftest.py (moved verbatim by the repo owner) by matching its sibling fixture_temp_dirs in shape: explicit scope="function", yield instead of return, and a docstring matching the sibling's voice. Deliberately does NOT add DEFAULT_INPUTDATA_ROOT/DEFAULT_STAGING_ROOT patching (every user runs relink.py as a subprocess, where in-process patching is inert and could mask a dropped --inputdata-root/--target-root flag falling back to the live production tree), and deliberately omits an explicit shutil.rmtree cleanup since everything is built under tmp_path, which pytest already cleans up automatically. Co-Authored-By: Claude Sonnet 5 --- tests/conftest.py | 25 +++++++++++++++++++++++++ tests/relink/test_cmdline.py | 20 -------------------- 2 files changed, 25 insertions(+), 20 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 0a7e232..eef610e 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -31,3 +31,28 @@ def fixture_temp_dirs(): # Cleanup shutil.rmtree(source_dir, ignore_errors=True) shutil.rmtree(target_dir, ignore_errors=True) + + +@pytest.fixture(scope="function", name="nested_mock_dirs") +def fixture_nested_mock_dirs(tmp_path): + """Create a nested source/target directory layout for testing relative-path + resolution.""" + source_dir = tmp_path / "source" + target_dir = tmp_path / "target" + source_sub_dir = source_dir / "sub" + target_sub_dir = target_dir / "sub" + source_sub_dir.mkdir(parents=True) + target_sub_dir.mkdir(parents=True) + + # Create a test file + source_file = source_sub_dir / "test_file.txt" + target_file = target_sub_dir / "test_file.txt" + source_file.write_text("source content") + target_file.write_text("target content") + + yield source_dir, target_dir, source_sub_dir, source_file, target_file + + # No explicit cleanup: everything above was created under tmp_path, which + # pytest already removes automatically. Unlike fixture_temp_dirs (which + # uses tempfile.mkdtemp, a directory pytest does not manage), an explicit + # shutil.rmtree here would be redundant. diff --git a/tests/relink/test_cmdline.py b/tests/relink/test_cmdline.py index e2f83da..64b1feb 100644 --- a/tests/relink/test_cmdline.py +++ b/tests/relink/test_cmdline.py @@ -29,26 +29,6 @@ def fixture_mock_dirs(tmp_path): return source_dir, target_dir, source_file, target_file -@pytest.fixture(name="nested_mock_dirs") -def fixture_nested_mock_dirs(tmp_path): - """Create a nested source/target layout for testing relative-path resolution - from inside an inputdata subdirectory.""" - source_dir = tmp_path / "source" - target_dir = tmp_path / "target" - source_sub_dir = source_dir / "sub" - target_sub_dir = target_dir / "sub" - source_sub_dir.mkdir(parents=True) - target_sub_dir.mkdir(parents=True) - - # Create a test file - source_file = source_sub_dir / "test_file.txt" - target_file = target_sub_dir / "test_file.txt" - source_file.write_text("source content") - target_file.write_text("target content") - - return source_dir, target_dir, source_sub_dir, source_file, target_file - - def test_command_line_execution_dry_run(mock_dirs): """Test executing relink.py from command line with --dry-run flag.""" source_dir, target_dir, source_file, _ = mock_dirs From 142111b9a633629da6f1e238046f417a8d9813e2 Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Mon, 31 Aug 2026 11:15:14 -0600 Subject: [PATCH 18/21] tests: derive decoy basenames from real files, assert decoy content differs Every decoy in the branch-added tests (relink's single-file and "." e2e tests, plus the four rimport --list/positional-argument no-root-fallback tests and their test_get_files_to_process.py in-process twin) previously wrote its basename as a second, independent literal -- sometimes matching the real file's basename by coincidence, sometimes (the relink "." test) deliberately different, per review-comment feedback asking why. The repo owner's ruling was to make them consistent: every decoy is same-named with the file it shadows, and that name must be derived from the real file's basename rather than copy-pasted, so the two can never quietly drift apart and stop discriminating. Where a basename was also duplicated into a CLI command argument, bound it to the same variable used for the path. Also added an explicit assertion at each decoy's creation time that its content differs from the real file's -- documented at the point of setup rather than left for the reader to notice two different string literals. Skipped where there is no real file to differ from (the two no-root-fallback error tests). Verified empirically, for every touched test, that the decoy assertion can still be made to fail: reproduced each scenario against the real production scripts (unmodified) plus scratch-only patched copies that simulate the specific regression each test guards against (root-relative resolution for relink; cwd-anchoring of --list entries; reintroduced root-fallback for CLI/positional names), confirming the decoy gets wrongly touched under the regression and the assertion flips to failing. This included re-confirming the relink "." test's newly same-named decoy still discriminates by location, as the owner's ruling argued it would. No production code changed. 287 tests still pass. Co-Authored-By: Claude Sonnet 5 --- tests/relink/test_cmdline.py | 40 +++++++++++++--------- tests/rimport/test_cmdline.py | 31 ++++++++++------- tests/rimport/test_get_files_to_process.py | 5 +-- 3 files changed, 45 insertions(+), 31 deletions(-) diff --git a/tests/relink/test_cmdline.py b/tests/relink/test_cmdline.py index 64b1feb..b9fc980 100644 --- a/tests/relink/test_cmdline.py +++ b/tests/relink/test_cmdline.py @@ -162,10 +162,12 @@ def test_command_line_relative_file_from_inputdata_subdir(nested_mock_dirs): # target copy (also different content). Correct cwd-relative resolution # of "test_file.txt" never reaches this file; a root-relative regression # would relink it instead of the subdir file. - decoy_file = source_dir / "test_file.txt" - decoy_target = target_dir / "test_file.txt" + decoy_file = source_dir / source_file.name + decoy_target = target_dir / target_file.name decoy_file.write_text("decoy content") decoy_target.write_text("decoy target content") + assert decoy_file.read_text() != source_file.read_text() + assert decoy_target.read_text() != target_file.read_text() # Get the path to relink.py relink_script = os.path.join( @@ -177,7 +179,7 @@ def test_command_line_relative_file_from_inputdata_subdir(nested_mock_dirs): command = [ sys.executable, relink_script, - "test_file.txt", + source_file.name, "--target-root", str(target_dir), "--inputdata-root", @@ -207,26 +209,32 @@ def test_command_line_relative_dir_dot_from_inputdata_subdir(nested_mock_dirs): """Test that '.' is resolved against the cwd (an inputdata subdirectory), not against the inputdata root. - A decoy file sits directly under the inputdata root, outside "sub", with - a matching target copy so it *would* be relinkable if reached. Because - relink's search is recursive, resolving '.' against cwd (source_dir/sub) - never reaches the decoy, while a root-relative regression -- resolving - '.' against inputdata_root (source_dir) instead -- would recurse into - the decoy too. Only the decoy assertion below actually discriminates - between those two resolutions; the symlink-target subdirectory - (test_file.txt) is reached by recursion either way. + A same-named decoy file sits directly under the inputdata root, outside + "sub", with different content than the intended file, plus a matching + target copy (also with different content) so it *would* be relinkable if + reached. This test's discriminator is LOCATION, not the name collision: + because relink's search is recursive, resolving '.' against cwd + (source_dir/sub) never reaches the decoy, while a root-relative + regression -- resolving '.' against inputdata_root (source_dir) instead + -- would recurse into the decoy too. Only the decoy assertion below + actually discriminates between those two resolutions; the symlink-target + subdirectory (source_file) is reached by recursion either way. """ source_dir, target_dir, source_sub_dir, source_file, target_file = ( nested_mock_dirs ) - # Decoy file directly under the inputdata root (outside "sub"), with a - # matching target copy. Correct cwd-relative resolution of "." never - # reaches this file; a root-relative regression would. - decoy_file = source_dir / "decoy.txt" - decoy_target = target_dir / "decoy.txt" + # Decoy file directly under the inputdata root (outside "sub"), same + # name as the intended file but different content, with a matching + # target copy (also different content). Correct cwd-relative resolution + # of "." never reaches this file; a root-relative regression would + # recurse into it too. + decoy_file = source_dir / source_file.name + decoy_target = target_dir / target_file.name decoy_file.write_text("decoy content") decoy_target.write_text("decoy target content") + assert decoy_file.read_text() != source_file.read_text() + assert decoy_target.read_text() != target_file.read_text() # Get the path to relink.py relink_script = os.path.join( diff --git a/tests/rimport/test_cmdline.py b/tests/rimport/test_cmdline.py index ae31018..cf9e14f 100644 --- a/tests/rimport/test_cmdline.py +++ b/tests/rimport/test_cmdline.py @@ -200,13 +200,14 @@ def test_list_inside_tree_relative_entries_anchor_to_list_dir_not_cwd( nested_file.write_text("real data") filelist = inputdata_root / "lnd" / "filelist.txt" - filelist.write_text("clm2/file1.nc\n") + filelist.write_text(f"clm2/{nested_file.name}\n") # Decoy at the cwd-anchored location: a cwd-anchoring regression would resolve here # instead, giving a wrong-file failure rather than a merely-missing-file one. - decoy_file = inputdata_root / "atm" / "clm2" / "file1.nc" + decoy_file = inputdata_root / "atm" / "clm2" / nested_file.name decoy_file.parent.mkdir(parents=True) decoy_file.write_text("decoy data") + assert decoy_file.read_text() != nested_file.read_text() # Run rimport with -list option, cwd inside the tree but at a DIFFERENT location # than the list file @@ -232,7 +233,7 @@ def test_list_inside_tree_relative_entries_anchor_to_list_dir_not_cwd( assert result.returncode == 0, f"Command failed: {result.stderr}" # Verify the real file (not the decoy) was staged, anchored to the list dir's subtree - staged_file = staging_root / "lnd" / "clm2" / "file1.nc" + staged_file = staging_root / "lnd" / "clm2" / nested_file.name assert staged_file.exists() assert staged_file.read_text() == "real data" @@ -243,7 +244,7 @@ def test_list_inside_tree_relative_entries_anchor_to_list_dir_not_cwd( # Verify the decoy was left untouched, and nothing staged at the cwd-anchored path assert not decoy_file.is_symlink() assert decoy_file.read_text() == "decoy data" - assert not (staging_root / "atm" / "clm2" / "file1.nc").exists() + assert not (staging_root / "atm" / "clm2" / nested_file.name).exists() def test_preserves_directory_structure(self, rimport_script, test_env, rimport_env): """Test that directory structure is preserved in staging.""" @@ -629,17 +630,19 @@ def test_relative_file_from_inputdata_subdir_stages_that_file( subdir = inputdata_root / "lnd" / "clm2" subdir.mkdir(parents=True) - subdir_file = subdir / "test.nc" + file_basename = "test.nc" + subdir_file = subdir / file_basename subdir_file.write_text("subdir data") - decoy_file = inputdata_root / "test.nc" + decoy_file = inputdata_root / file_basename decoy_file.write_text("decoy data") + assert decoy_file.read_text() != subdir_file.read_text() # Run rimport with a relative positional filename, from inside the subdir command = [ sys.executable, rimport_script, - "test.nc", + file_basename, "-inputdata", str(inputdata_root), ] @@ -657,7 +660,7 @@ def test_relative_file_from_inputdata_subdir_stages_that_file( assert result.returncode == 0, f"Command failed: {result.stderr}" # Verify the subdir file (not the decoy) was staged - staged_file = staging_root / "lnd" / "clm2" / "test.nc" + staged_file = staging_root / "lnd" / "clm2" / file_basename assert staged_file.exists() assert staged_file.read_text() == "subdir data" @@ -670,7 +673,7 @@ def test_relative_file_from_inputdata_subdir_stages_that_file( assert decoy_file.read_text() == "decoy data" # Verify nothing was staged at the root-anchored path - assert not (staging_root / "test.nc").exists() + assert not (staging_root / file_basename).exists() def test_relative_file_from_subdir_missing_errors_no_root_fallback( self, rimport_script, test_env, rimport_env @@ -683,14 +686,15 @@ def test_relative_file_from_subdir_missing_errors_no_root_fallback( subdir = inputdata_root / "lnd" / "clm2" subdir.mkdir(parents=True) - root_file = inputdata_root / "test.nc" + file_basename = "test.nc" + root_file = inputdata_root / file_basename root_file.write_text("root data") # Run rimport with a relative positional filename, from inside the subdir command = [ sys.executable, rimport_script, - "test.nc", + file_basename, "-inputdata", str(inputdata_root), ] @@ -735,14 +739,15 @@ def test_relative_file_from_outside_tree_errors_no_root_fallback( outside = tmp_path / "outside" outside.mkdir() - decoy_file = inputdata_root / "test.nc" + file_basename = "test.nc" + decoy_file = inputdata_root / file_basename decoy_file.write_text("decoy data") # Run rimport with a relative positional filename, from outside the tree command = [ sys.executable, rimport_script, - "test.nc", + file_basename, "-inputdata", str(inputdata_root), ] diff --git a/tests/rimport/test_get_files_to_process.py b/tests/rimport/test_get_files_to_process.py index b6a90fa..c10665a 100644 --- a/tests/rimport/test_get_files_to_process.py +++ b/tests/rimport/test_get_files_to_process.py @@ -176,12 +176,13 @@ def test_list_relative_entries_anchor_to_list_dir_not_cwd(self, tmp_path, monkey # Decoy at the cwd-anchored location: a cwd-anchoring regression would resolve here # instead, giving a wrong-file failure rather than a merely-missing-file one. - decoy_file = atm_dir / "clm2" / "file1.nc" + decoy_file = atm_dir / "clm2" / real_file.name decoy_file.parent.mkdir(parents=True) decoy_file.write_text("decoy data") + assert decoy_file.read_text() != real_file.read_text() filelist = list_dir / "filelist.txt" - filelist.write_text("clm2/file1.nc\n", encoding="utf8") + filelist.write_text(f"clm2/{real_file.name}\n", encoding="utf8") # cwd inside the tree, but at a different location than the list file monkeypatch.chdir(atm_dir) From 66ca8b321c61124606d6356f10fdcdf409dc953d Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Mon, 31 Aug 2026 11:32:13 -0600 Subject: [PATCH 19/21] tests(rimport): give test_list_inside_tree_relative_entries a decoy It previously discriminated only by absence: a --list entry anchored to the inputdata root instead of the list file's own directory hits a nonexistent path, so the run just fails. That's weaker than it looks -- if a file happened to already exist at the root-anchored path, a root-relative regression would silently succeed by publishing the wrong file, and the test would pass. Add a same-named decoy (basename derived from the real file's, per the convention Task R just established) at the root-anchored path inputdata_root/clm2/file1.nc, with content asserted to differ from the real file's at creation time. Assert it stays untouched (not a symlink, original content intact) and that nothing lands in staging_root at the root-anchored path, alongside the existing checks that the real file (under lnd/clm2/) is what actually got staged and relinked. No cwd decoy is added: this test passes no cwd= to subprocess.run, so the subprocess inherits pytest's own outside-the-tree cwd, and a cwd-anchoring regression would resolve outside the inputdata root and fail loudly -- nowhere useful to plant one. The sibling test (test_list_inside_tree_relative_entries_anchor_to_list_dir_not_cwd) already covers that case. Verified empirically that the decoy assertion is not dead coverage: against a scratch-only patched copy of rimport (list entries anchored to the inputdata root instead of the list file's directory -- the tracked rimport was never touched), the run succeeds, the decoy gets wrongly staged and relinked, and all three added decoy assertions flip to failing; a pytest run with the decoy checks temporarily reordered ahead of the pre-existing ones (scratch copy only) fails directly on the new `assert not decoy_file.is_symlink()` line. Scratch dir removed afterward. No production code changed. 287 tests still pass. Co-Authored-By: Claude Sonnet 5 --- tests/rimport/test_cmdline.py | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/tests/rimport/test_cmdline.py b/tests/rimport/test_cmdline.py index cf9e14f..d02d34c 100644 --- a/tests/rimport/test_cmdline.py +++ b/tests/rimport/test_cmdline.py @@ -151,7 +151,14 @@ def test_list_inside_tree_relative_entries( nested_file.write_text("nested data") filelist = inputdata_root / "lnd" / "filelist.txt" - filelist.write_text("clm2/file1.nc\n") + filelist.write_text(f"clm2/{nested_file.name}\n") + + # Decoy at the root-anchored location: a root-relative regression would resolve + # here instead, giving a wrong-file failure rather than a merely-missing-file one. + decoy_file = inputdata_root / "clm2" / nested_file.name + decoy_file.parent.mkdir(parents=True) + decoy_file.write_text("decoy data") + assert decoy_file.read_text() != nested_file.read_text() # Run rimport with -list option command = [ @@ -183,6 +190,11 @@ def test_list_inside_tree_relative_entries( assert nested_file.is_symlink() assert nested_file.resolve() == staged_file + # Verify the decoy was left untouched, and nothing staged at the root-anchored path + assert not decoy_file.is_symlink() + assert decoy_file.read_text() == "decoy data" + assert not (staging_root / "clm2" / nested_file.name).exists() + def test_list_inside_tree_relative_entries_anchor_to_list_dir_not_cwd( self, rimport_script, test_env, rimport_env ): From 8f756a2327e284099f7dfa23fbeb520d2cbbc3ff Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Mon, 31 Aug 2026 11:52:03 -0600 Subject: [PATCH 20/21] tests: de-duplicate two near-identical test pairs added in this PR Extract a shared helper for each pair (relink's filename-vs-"." relative resolution test, rimport's mixed-validity list vs --check test) while keeping each test's own docstring and payoff assertions intact, per the PR review comments asking to reduce duplication without losing what each test discriminates. Co-Authored-By: Claude Sonnet 5 --- tests/relink/test_cmdline.py | 103 ++++++++++++++++------------------ tests/rimport/test_cmdline.py | 94 ++++++++++++++----------------- 2 files changed, 89 insertions(+), 108 deletions(-) diff --git a/tests/relink/test_cmdline.py b/tests/relink/test_cmdline.py index b9fc980..a6bca83 100644 --- a/tests/relink/test_cmdline.py +++ b/tests/relink/test_cmdline.py @@ -136,22 +136,23 @@ def test_command_line_execution_given_file(mock_dirs): assert f"{INDENT}Created symbolic link:" in result.stdout -def test_command_line_relative_file_from_inputdata_subdir(nested_mock_dirs): - """Test that a bare relative filename is resolved against the cwd (an - inputdata subdirectory), not against the inputdata root. - - A same-named decoy file sits directly under the inputdata root, outside - "sub", with different content than the intended file, plus a matching - target copy (also with different content) so it *would* be relinked if a - root-relative regression resolved "test_file.txt" against inputdata_root - instead of cwd. Because this is a single-file argument rather than a - directory to recurse into, such a regression would process the decoy - INSTEAD OF the subdir file, not in addition to it. The return code and - decoy_file.is_file() pass either way (the latter because is_file() - follows a symlink to a real file); what actually discriminates is that - the decoy stays a plain file with its original content (not relinked), - and that the intended subdir file is the one converted to a symlink -- - pointing at the subdir's target copy, not the root decoy's. +def _run_relink_relative_positional(nested_mock_dirs, positional_arg): + """Create a same-named decoy at the inputdata root (outside "sub"), then + run relink.py with `positional_arg` as the sole path argument and cwd + set to the inputdata subdirectory. + + Shared setup and invocation for + test_command_line_relative_file_from_inputdata_subdir and + test_command_line_relative_dir_dot_from_inputdata_subdir. Those two + tests differ ONLY in `positional_arg` (a bare filename vs "."), and that + single difference changes what regression each one discriminates and + which of its own assertions does the discriminating -- see each test's + docstring. This helper deliberately makes no assertion about the + outcome of the run beyond confirming the decoy setup itself: the payoff + assertions that make each test meaningful differ between the two + callers and stay in the tests, not here. + + Returns (result, decoy_file) for the caller to assert against. """ source_dir, target_dir, source_sub_dir, source_file, target_file = ( nested_mock_dirs @@ -159,9 +160,7 @@ def test_command_line_relative_file_from_inputdata_subdir(nested_mock_dirs): # Decoy file directly under the inputdata root (outside "sub"), same # name as the intended file but different content, with a matching - # target copy (also different content). Correct cwd-relative resolution - # of "test_file.txt" never reaches this file; a root-relative regression - # would relink it instead of the subdir file. + # target copy (also different content). decoy_file = source_dir / source_file.name decoy_target = target_dir / target_file.name decoy_file.write_text("decoy content") @@ -179,7 +178,7 @@ def test_command_line_relative_file_from_inputdata_subdir(nested_mock_dirs): command = [ sys.executable, relink_script, - source_file.name, + positional_arg, "--target-root", str(target_dir), "--inputdata-root", @@ -191,6 +190,32 @@ def test_command_line_relative_file_from_inputdata_subdir(nested_mock_dirs): command, cwd=str(source_sub_dir), capture_output=True, text=True, check=False ) + return result, decoy_file + + +def test_command_line_relative_file_from_inputdata_subdir(nested_mock_dirs): + """Test that a bare relative filename is resolved against the cwd (an + inputdata subdirectory), not against the inputdata root. + + A same-named decoy file sits directly under the inputdata root, outside + "sub", with different content than the intended file, plus a matching + target copy (also with different content) so it *would* be relinked if a + root-relative regression resolved "test_file.txt" against inputdata_root + instead of cwd. Because this is a single-file argument rather than a + directory to recurse into, such a regression would process the decoy + INSTEAD OF the subdir file, not in addition to it. The return code and + decoy_file.is_file() pass either way (the latter because is_file() + follows a symlink to a real file); what actually discriminates is that + the decoy stays a plain file with its original content (not relinked), + and that the intended subdir file is the one converted to a symlink -- + pointing at the subdir's target copy, not the root decoy's. + """ + *_, source_file, target_file = nested_mock_dirs + + result, decoy_file = _run_relink_relative_positional( + nested_mock_dirs, source_file.name + ) + # Verify the command executed successfully assert result.returncode == 0, f"Command failed with stderr: {result.stderr}" @@ -220,43 +245,9 @@ def test_command_line_relative_dir_dot_from_inputdata_subdir(nested_mock_dirs): actually discriminates between those two resolutions; the symlink-target subdirectory (source_file) is reached by recursion either way. """ - source_dir, target_dir, source_sub_dir, source_file, target_file = ( - nested_mock_dirs - ) - - # Decoy file directly under the inputdata root (outside "sub"), same - # name as the intended file but different content, with a matching - # target copy (also different content). Correct cwd-relative resolution - # of "." never reaches this file; a root-relative regression would - # recurse into it too. - decoy_file = source_dir / source_file.name - decoy_target = target_dir / target_file.name - decoy_file.write_text("decoy content") - decoy_target.write_text("decoy target content") - assert decoy_file.read_text() != source_file.read_text() - assert decoy_target.read_text() != target_file.read_text() + *_, source_file, target_file = nested_mock_dirs - # Get the path to relink.py - relink_script = os.path.join( - os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__)))), - "relink.py", - ) - - # Build the command - command = [ - sys.executable, - relink_script, - ".", - "--target-root", - str(target_dir), - "--inputdata-root", - str(source_dir), - ] - - # Execute the command with cwd set to the inputdata subdirectory - result = subprocess.run( - command, cwd=str(source_sub_dir), capture_output=True, text=True, check=False - ) + result, decoy_file = _run_relink_relative_positional(nested_mock_dirs, ".") # Verify the command executed successfully assert result.returncode == 0, f"Command failed with stderr: {result.stderr}" diff --git a/tests/rimport/test_cmdline.py b/tests/rimport/test_cmdline.py index d02d34c..e7382b6 100644 --- a/tests/rimport/test_cmdline.py +++ b/tests/rimport/test_cmdline.py @@ -1081,16 +1081,24 @@ def test_check_directory_argument_reports_error_not_publishable( assert not list(inputdata_root.rglob("*.tmp")) assert inner_file.read_text() == "clm2 data" - def test_mixed_validity_list_aborts_and_stages_nothing( - self, rimport_script, test_env, rimport_env - ): - """Test the pre-flight gate end to end: a --list with one valid entry and two entries - that are invalid in DIFFERENT ways (missing, and a directory) aborts the whole batch - with rc 2, reports every failure reason, gets the "N of M" count right, and — the - assertion that matters most — never stages or relinks the valid entry. - - The list file lives OUTSIDE the inputdata tree with absolute entries (this configuration - previously had no end-to-end coverage at all).""" + def _run_mixed_validity_list(self, rimport_script, test_env, rimport_env, *, check): + """Set up a --list with one valid entry (good.nc) and two entries that are invalid + in DIFFERENT ways (missing.nc, and a directory named adir), all as absolute paths in + a list file OUTSIDE the inputdata tree, then run rimport.py against it -- with + --check when `check` is True (which also requires deleting + RIMPORT_SKIP_USER_CHECK, since --check needs ensure_running_as() to actually run), + without it otherwise. + + Shared setup and invocation for test_mixed_validity_list_aborts_and_stages_nothing + and test_check_mode_is_gated_too_and_reports_nothing_for_valid_entry. The two tests + differ in `check` and, more importantly, in what each one's own payoff assertions + check afterward -- see each test's docstring. This helper asserts only the rc-2 and + failure-reason outcome that is IDENTICAL for both callers and that neither test + discriminates on; the payoff assertions that make each test meaningful stay in the + tests, not here. + + Returns (result, valid_file, staging_root) for the caller to assert against. + """ inputdata_root = test_env["inputdata_root"] staging_root = test_env["staging_root"] tmp_path = test_env["tmp_path"] @@ -1115,6 +1123,10 @@ def test_mixed_validity_list_aborts_and_stages_nothing( "-inputdata", str(inputdata_root), ] + if check: + # Make sure --check skips ensure_running_as() + del rimport_env["RIMPORT_SKIP_USER_CHECK"] + command.append("--check") result = subprocess.run( command, @@ -1124,12 +1136,29 @@ def test_mixed_validity_list_aborts_and_stages_nothing( env=rimport_env, ) - # Verify failure: rc 2, all reasons present, correct "N of M" count + # Verify failure: rc 2, all reasons present, correct "N of M" count. Identical for + # both callers; not what either test discriminates on. assert result.returncode == 2, f"Command unexpectedly passed: {result.stdout}" assert "2 of 3 file(s) failed pre-flight validation" in result.stderr assert f"source not found: {missing_file}" in result.stderr assert f"source is a directory, not a file: {bad_dir}" in result.stderr + return result, valid_file, staging_root + + def test_mixed_validity_list_aborts_and_stages_nothing( + self, rimport_script, test_env, rimport_env + ): + """Test the pre-flight gate end to end: a --list with one valid entry and two entries + that are invalid in DIFFERENT ways (missing, and a directory) aborts the whole batch + with rc 2, reports every failure reason, gets the "N of M" count right, and — the + assertion that matters most — never stages or relinks the valid entry. + + The list file lives OUTSIDE the inputdata tree with absolute entries (this configuration + previously had no end-to-end coverage at all).""" + _result, valid_file, staging_root = self._run_mixed_validity_list( + rimport_script, test_env, rimport_env, check=False + ) + # Verify the valid file was NOT staged and NOT turned into a symlink assert not (staging_root / "good.nc").exists() assert not valid_file.is_symlink() @@ -1149,49 +1178,10 @@ def test_check_mode_is_gated_too_and_reports_nothing_for_valid_entry( reporting, even though it means a --check run tells you nothing about the files that would have been fine) — a future reader should not "fix" this into per-file --check reporting without first re-litigating that choice with the repo owner.""" - inputdata_root = test_env["inputdata_root"] - staging_root = test_env["staging_root"] - tmp_path = test_env["tmp_path"] - - valid_file = inputdata_root / "good.nc" - valid_file.write_text("good data") - - missing_file = inputdata_root / "missing.nc" - - bad_dir = inputdata_root / "adir" - bad_dir.mkdir() - - # List file OUTSIDE the tree, with absolute entries. - filelist = tmp_path / "filelist.txt" - filelist.write_text(f"{valid_file}\n{missing_file}\n{bad_dir}\n") - - # Make sure --check skips ensure_running_as() - del rimport_env["RIMPORT_SKIP_USER_CHECK"] - - command = [ - sys.executable, - rimport_script, - "-list", - str(filelist), - "-inputdata", - str(inputdata_root), - "--check", - ] - - result = subprocess.run( - command, - capture_output=True, - text=True, - check=False, - env=rimport_env, + result, valid_file, staging_root = self._run_mixed_validity_list( + rimport_script, test_env, rimport_env, check=True ) - # Verify failure: rc 2, all reasons present - assert result.returncode == 2, f"Command unexpectedly passed: {result.stdout}" - assert "2 of 3 file(s) failed pre-flight validation" in result.stderr - assert f"source not found: {missing_file}" in result.stderr - assert f"source is a directory, not a file: {bad_dir}" in result.stderr - # Verify the valid entry's check status is NOT reported: --check never gets to run # per-file, so neither the "already published" nor "not already published" messages # appear anywhere, for any file. From 8f15cf116b40e34ce8e702693080583bc66e57a5 Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Mon, 31 Aug 2026 12:10:43 -0600 Subject: [PATCH 21/21] tests: make comments and docstrings stand on their own Comments and docstrings across the branch referenced the development process rather than the code: "the deleted root-fallback", "a later task", "see the brief", "as it did before the guard", "previously had no end-to-end coverage". None of that means anything to someone reading these files without knowing this PR existed. Replace each with the rationale it was standing in for, or cut it where the surrounding sentence already carried the useful fact. Kept the passages that explain why the code is shaped the way it is rather than how it got there -- the "file must exist, or the source-not-found check fires before the guardrail" comments, and normalize_paths' note that its root-join is unreachable and deliberately-kept dead code, without which a reader would delete the branch or conclude root-relative resolution is still supported. Also corrects two inaccuracies rather than only trimming: a vague reference to "the existing e2e list test" now names it, and _run_mixed_validity_list's docstring no longer claims its hoisted rc-2 assertions are something "neither test discriminates on" -- they do guard against the pre-flight gate not firing at all, they just aren't what tells the two tests apart. Fixes the script name in that same docstring: it is rimport, not rimport.py. Text only; no executable line changed and the suite is unmoved at 287. Co-Authored-By: Claude Sonnet 5 Co-Authored-By: Claude Opus 5 (1M context) --- tests/rimport/test_cmdline.py | 49 +++++++++++----------- tests/rimport/test_get_files_to_process.py | 30 ++++++------- 2 files changed, 40 insertions(+), 39 deletions(-) diff --git a/tests/rimport/test_cmdline.py b/tests/rimport/test_cmdline.py index e7382b6..d08d89e 100644 --- a/tests/rimport/test_cmdline.py +++ b/tests/rimport/test_cmdline.py @@ -200,9 +200,10 @@ def test_list_inside_tree_relative_entries_anchor_to_list_dir_not_cwd( ): """Test that a list file's relative entries anchor to the list file's own directory, not the cwd, even when rimport is run with its cwd inside the tree at a DIFFERENT - location. The existing e2e list test passes no cwd= to subprocess.run, so pytest's own - (outside-the-tree) cwd applies and cwd-anchoring and list-dir-anchoring agree; this test - sets cwd= explicitly so the two schemes can be told apart.""" + location. test_list_inside_tree_relative_entries above passes no cwd= to + subprocess.run, so pytest's own (outside-the-tree) cwd applies and cwd-anchoring and + list-dir-anchoring agree; this test sets cwd= explicitly so the two schemes can be + told apart.""" inputdata_root = test_env["inputdata_root"] staging_root = test_env["staging_root"] @@ -737,12 +738,12 @@ def test_relative_file_from_outside_tree_errors_no_root_fallback( """Test that a relative positional filename run from OUTSIDE the inputdata tree anchors to cwd and errors, rather than falling back to a same-named file at the inputdata root. - This is the configuration the deleted root-fallback actually operated in: with cwd - inside the tree the old dual-mode code already anchored to cwd, so the sibling tests - above would have passed against it unmodified. Only an outside-the-tree cwd - discriminates the old behavior (silently stage the root file, rc 0) from the new one - (error, stage nothing). The decoy is what makes it discriminating: without a file at - the root-anchored path there would be nothing for a regression to wrongly publish. + An outside-the-tree cwd is essential here: with cwd inside the tree, root-anchored and + cwd-anchored resolution agree, so the sibling tests above would pass either way. Only + placing cwd outside the tree discriminates root-anchored resolution (which would + silently stage the root file, rc 0) from cwd-anchored resolution (error, stage + nothing). The decoy is what makes it discriminating: without a file at the + root-anchored path there would be nothing for a regression to wrongly publish. """ inputdata_root = test_env["inputdata_root"] staging_root = test_env["staging_root"] @@ -773,8 +774,8 @@ def test_relative_file_from_outside_tree_errors_no_root_fallback( cwd=outside, ) - # Verify failure. Deliberately not pinning the exact code: pre-flight validation - # (a later task) shifts this class of user error from 1 to 2. + # Verify failure. Deliberately not pinning the exact return code, since it may change + # in the future for any reason. assert result.returncode != 0, f"Command unexpectedly passed: {result.stdout}" # Verify the decoy at the inputdata root was NOT published @@ -990,8 +991,8 @@ def test_empty_string_argument_errors_and_leaves_tree_intact( itself and running that root through the destructive replace-with-symlink path.""" inputdata_root = test_env["inputdata_root"] # staging_root itself need not be assigned here — the fixture already created it, and - # its mere existence is what makes dst.exists() true for rel="." (see the brief: this is - # what turns an unset shell variable into a whole-tree rename). + # its mere existence is what makes dst.exists() true for rel="." — the same thing that + # turns an unset shell variable into a whole-tree rename. marker_file = inputdata_root / "marker.nc" marker_file.write_text("root marker") @@ -1035,8 +1036,8 @@ def test_check_directory_argument_reports_error_not_publishable( self, rimport_script, test_env, rimport_env ): """Test that --check on a directory argument reports it as an error, rather than - claiming (as it did before the guard) that the directory is already published but not - linked and available for download.""" + misreporting the directory as already published but not linked and available for + download.""" inputdata_root = test_env["inputdata_root"] staging_root = test_env["staging_root"] @@ -1084,7 +1085,7 @@ def test_check_directory_argument_reports_error_not_publishable( def _run_mixed_validity_list(self, rimport_script, test_env, rimport_env, *, check): """Set up a --list with one valid entry (good.nc) and two entries that are invalid in DIFFERENT ways (missing.nc, and a directory named adir), all as absolute paths in - a list file OUTSIDE the inputdata tree, then run rimport.py against it -- with + a list file OUTSIDE the inputdata tree, then run rimport against it -- with --check when `check` is True (which also requires deleting RIMPORT_SKIP_USER_CHECK, since --check needs ensure_running_as() to actually run), without it otherwise. @@ -1093,9 +1094,10 @@ def _run_mixed_validity_list(self, rimport_script, test_env, rimport_env, *, che and test_check_mode_is_gated_too_and_reports_nothing_for_valid_entry. The two tests differ in `check` and, more importantly, in what each one's own payoff assertions check afterward -- see each test's docstring. This helper asserts only the rc-2 and - failure-reason outcome that is IDENTICAL for both callers and that neither test - discriminates on; the payoff assertions that make each test meaningful stay in the - tests, not here. + failure-reason outcome that is IDENTICAL for both callers; those assertions still + matter (they would catch the pre-flight gate failing to fire at all) but are not what + tells the two tests apart -- the payoff assertions that make each test meaningful stay + in the tests, not here. Returns (result, valid_file, staging_root) for the caller to assert against. """ @@ -1153,8 +1155,7 @@ def test_mixed_validity_list_aborts_and_stages_nothing( with rc 2, reports every failure reason, gets the "N of M" count right, and — the assertion that matters most — never stages or relinks the valid entry. - The list file lives OUTSIDE the inputdata tree with absolute entries (this configuration - previously had no end-to-end coverage at all).""" + The list file lives OUTSIDE the inputdata tree with absolute entries.""" _result, valid_file, staging_root = self._run_mixed_validity_list( rimport_script, test_env, rimport_env, check=False ) @@ -1174,10 +1175,10 @@ def test_check_mode_is_gated_too_and_reports_nothing_for_valid_entry( valid and invalid entries aborts with rc 2 and the valid entry's status is NOT reported. - This pins a deliberate design decision (uniform abort, chosen over per-file --check + This pins a deliberate design decision: uniform abort, chosen over per-file --check reporting, even though it means a --check run tells you nothing about the files that - would have been fine) — a future reader should not "fix" this into per-file --check - reporting without first re-litigating that choice with the repo owner.""" + would have been fine. A future reader should not silently "fix" this into per-file + --check reporting -- that would be a deliberate change in behavior, not a bug fix.""" result, valid_file, staging_root = self._run_mixed_validity_list( rimport_script, test_env, rimport_env, check=True ) diff --git a/tests/rimport/test_get_files_to_process.py b/tests/rimport/test_get_files_to_process.py index c10665a..e709e5e 100644 --- a/tests/rimport/test_get_files_to_process.py +++ b/tests/rimport/test_get_files_to_process.py @@ -222,11 +222,10 @@ def test_list_at_root_relative_entries_anchored_to_root(self, tmp_path): assert files_to_process == [str(root_resolved / f) for f in filenames] def test_list_outside_tree_relative_entry_anchors_to_list_dir(self, tmp_path, monkeypatch): - """Test that a relative entry in a list file OUTSIDE the tree now anchors to the list - file's own directory instead of erroring (the deleted root-fallback used to make this a - fatal error). Discriminating setup: cwd is neither the list dir nor the inputdata root, - so the assertion can distinguish list-dir-anchoring from cwd-anchoring and from the - (now-impossible) root-anchoring.""" + """Test that a relative entry in a list file OUTSIDE the tree anchors to the list + file's own directory rather than erroring. Discriminating setup: cwd is neither the + list dir nor the inputdata root, so the assertion can distinguish list-dir-anchoring + from cwd-anchoring and from root-anchoring.""" # Setup list_dir = tmp_path / "outside" / "listdir" list_dir.mkdir(parents=True) @@ -482,8 +481,8 @@ def test_cli_items_relative_cwd_inside_tree(self, tmp_path, monkeypatch): def test_cli_relative_cwd_outside_tree_still_anchors_to_cwd(self, tmp_path, monkeypatch): """Test that a relative --file name anchors to cwd even when cwd is outside the - inputdata tree: there is no inside/outside distinction any more, one rule applies - everywhere, with no root-relative fallback.""" + inputdata tree: there is no inside/outside distinction, one rule applies everywhere, + with no root-relative fallback.""" outside = tmp_path / "outside" outside.mkdir() monkeypatch.chdir(outside) @@ -567,8 +566,8 @@ def test_cli_cwd_inside_tree_via_symlink(self, tmp_path, monkeypatch): def test_deleted_cwd_with_absolute_names_still_works(self, tmp_path, monkeypatch, caplog): """Test that a deleted cwd does not raise: absolute names are returned unchanged since cwd is irrelevant to resolving them. Simulates a deleted cwd for real (not mocked): chdir - into a directory, then remove it out from under the process (confirmed to actually raise - FileNotFoundError from Path.cwd() on this platform before writing this test).""" + into a directory, then remove it out from under the process, which raises + FileNotFoundError from Path.cwd() on this platform.""" inputdata_root = tmp_path / "inputdata" inputdata_root.mkdir() abs_file = str(inputdata_root / "test.nc") @@ -593,10 +592,10 @@ def test_deleted_cwd_with_absolute_names_still_works(self, tmp_path, monkeypatch assert "working directory" not in caplog.text.lower() def test_deleted_cwd_with_relative_name_errors(self, tmp_path, monkeypatch, caplog): - """Test that a deleted cwd is now a FATAL error for relative names, rather than falling - back to root-relative resolution: with no fallback, there is nothing left to anchor a - relative name against. Confirms rc 2, files_to_process is None, and that the error - message names the offending relative name.""" + """Test that a deleted cwd is a fatal error for relative names: with no root-relative + fallback, there is nothing left to anchor a relative name against. Confirms rc 2, + files_to_process is None, and that the error message names the offending relative + name.""" filename = "test.nc" deleted_dir = tmp_path / "deleted" @@ -622,8 +621,9 @@ def test_deleted_cwd_with_multiple_relative_names_reports_all( self, tmp_path, monkeypatch, caplog ): """Test that when a deleted cwd leaves several relative names unresolvable, the error - message names ALL of them, not just the first -- Sam's stated preference is to fail - before doing anything and name every offending input, not just one.""" + message names ALL of them, not just the first -- failing before doing anything and + naming every offending input at once gives the user everything they need to fix in one + pass, rather than fixing inputs one at a time across repeated runs.""" filename = "test.nc" item_names = ["a.txt", "b.txt"]