From 918d9de5e1a4d9b06ae9d3d19b06b72c3d21df7e Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Mon, 31 Aug 2026 13:52:42 -0600 Subject: [PATCH 01/47] chore: git-ignore the .superpowers scratch directory Holds the subagent-driven-development ledger and per-task artifacts for the in-flight plan. Scratch, not source. Co-Authored-By: Claude Opus 5 (1M context) --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 3bdf40f..1066029 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,4 @@ __pycache__ .coverage .vscode +.superpowers/ From ff40fca57fa7f48d1b90e156fb614c50791a01fb Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Mon, 31 Aug 2026 13:54:18 -0600 Subject: [PATCH 02/47] tests(rimport): red tests for walk_files walk_files does not exist yet; these fail by design. They pin the enumeration contract: recursive, sorted for determinism, symlinks yielded as entries, no descent through a directory symlink (which would let the walk leave the named tree or loop on a cycle), dotfiles included, and an unreadable directory returned as a Skip rather than raised. Co-Authored-By: Claude Opus 5 (1M context) --- tests/rimport/test_walk_files.py | 119 +++++++++++++++++++++++++++++++ 1 file changed, 119 insertions(+) create mode 100644 tests/rimport/test_walk_files.py diff --git a/tests/rimport/test_walk_files.py b/tests/rimport/test_walk_files.py new file mode 100644 index 0000000..92269a3 --- /dev/null +++ b/tests/rimport/test_walk_files.py @@ -0,0 +1,119 @@ +""" +Tests for walk_files() 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_finds_files_recursively(tmp_path): + """Files at every depth are returned, and directories themselves are not.""" + (tmp_path / "a").mkdir() + (tmp_path / "a" / "b").mkdir() + (tmp_path / "top.nc").write_text("top") + (tmp_path / "a" / "mid.nc").write_text("mid") + (tmp_path / "a" / "b" / "deep.nc").write_text("deep") + + files, skips = rimport.walk_files(tmp_path) + + assert skips == [] + assert files == [ + tmp_path / "a" / "b" / "deep.nc", + tmp_path / "a" / "mid.nc", + tmp_path / "top.nc", + ] + + +def test_entries_are_sorted_at_each_level(tmp_path): + """Order is deterministic, so output and downstream assertions are stable.""" + for name in ["zebra.nc", "apple.nc", "mango.nc"]: + (tmp_path / name).write_text(name) + + files, _skips = rimport.walk_files(tmp_path) + + assert [f.name for f in files] == ["apple.nc", "mango.nc", "zebra.nc"] + + +def test_symlinks_to_files_are_yielded(tmp_path): + """A symlink is an entry to act on, not something to pass over.""" + real = tmp_path / "real.nc" + real.write_text("data") + link = tmp_path / "link.nc" + link.symlink_to(real) + + files, _skips = rimport.walk_files(tmp_path) + + assert link in files + + +def test_does_not_descend_through_directory_symlink(tmp_path): + """A symlink to a directory is yielded as ONE entry; the walk must not follow it. + + Following it would let the walk leave the directory the user named, and would let a + cyclic link loop forever. + """ + tree = tmp_path / "tree" + tree.mkdir() + outside = tmp_path / "outside" + outside.mkdir() + (outside / "hidden_away.nc").write_text("must not be found") + link = tree / "dirlink" + link.symlink_to(outside) + + files, _skips = rimport.walk_files(tree) + + assert files == [link] + assert not any(f.name == "hidden_away.nc" for f in files) + + +def test_dotfiles_are_included(tmp_path): + """No hidden-file filter: rimport publishes what is there.""" + (tmp_path / ".hidden.nc").write_text("data") + + files, _skips = rimport.walk_files(tmp_path) + + assert [f.name for f in files] == [".hidden.nc"] + + +def test_empty_directory_yields_nothing(tmp_path): + """An empty tree is not an error.""" + files, skips = rimport.walk_files(tmp_path) + + assert files == [] + assert skips == [] + + +def test_unreadable_directory_is_returned_as_a_skip(tmp_path): + """A directory that cannot be read is reported, not raised, and does not abort the walk.""" + readable = tmp_path / "readable" + readable.mkdir() + (readable / "good.nc").write_text("data") + locked = tmp_path / "locked" + locked.mkdir() + (locked / "unreachable.nc").write_text("data") + os.chmod(locked, 0o000) + + try: + files, skips = rimport.walk_files(tmp_path) + finally: + os.chmod(locked, 0o700) + + assert files == [readable / "good.nc"] + assert len(skips) == 1 + assert skips[0].path == locked + assert isinstance(skips[0].reason, OSError) From 8b1b94e77d62dc1cc3967bfb8b198f928ad736d9 Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Mon, 31 Aug 2026 13:58:14 -0600 Subject: [PATCH 03/47] tests(rimport): red tests for expand_directories expand_directories does not exist yet; these fail by design. They pin named-vs-discovered provenance, which is what lets a discovered failure warn-and-skip while a path the user typed still aborts the batch. Also pinned: a named symlink-to-directory is not expanded, duplicates collapse with named winning, an empty directory warns without becoming a skip, and the expansion count line only appears when something actually expanded. Co-Authored-By: Claude Opus 5 (1M context) --- tests/rimport/test_expand_directories.py | 159 +++++++++++++++++++++++ 1 file changed, 159 insertions(+) create mode 100644 tests/rimport/test_expand_directories.py diff --git a/tests/rimport/test_expand_directories.py b/tests/rimport/test_expand_directories.py new file mode 100644 index 0000000..7c855ea --- /dev/null +++ b/tests/rimport/test_expand_directories.py @@ -0,0 +1,159 @@ +""" +Tests for expand_directories() function in rimport script. +""" + +import os +import logging +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_directory_expands_to_discovered_files(tmp_path): + """Files found by walking a named directory are tagged discovered, not named.""" + d = tmp_path / "d" + d.mkdir() + (d / "a.nc").write_text("a") + (d / "b.nc").write_text("b") + + entries, skips = rimport.expand_directories([d]) + + assert skips == [] + assert entries == [ + rimport.Entry(d / "a.nc", False), + rimport.Entry(d / "b.nc", False), + ] + + +def test_plain_file_passes_through_as_named(tmp_path): + """A file the user typed stays named, so its failures stay fatal.""" + f = tmp_path / "f.nc" + f.write_text("data") + + entries, _skips = rimport.expand_directories([f]) + + assert entries == [rimport.Entry(f, True)] + + +def test_nonexistent_path_passes_through_as_named(tmp_path): + """Expansion does not validate. A missing path stays named so pre-flight can reject it.""" + missing = tmp_path / "missing.nc" + + entries, _skips = rimport.expand_directories([missing]) + + assert entries == [rimport.Entry(missing, True)] + + +def test_symlink_to_directory_is_not_expanded(tmp_path): + """A named symlink-to-directory stays ONE named entry under the per-file rules. + + It is not walked, matching walk_files' refusal to descend through a directory symlink. + """ + real_dir = tmp_path / "real_dir" + real_dir.mkdir() + (real_dir / "inside.nc").write_text("data") + link = tmp_path / "dirlink" + link.symlink_to(real_dir) + + entries, _skips = rimport.expand_directories([link]) + + assert entries == [rimport.Entry(link, True)] + + +def test_duplicates_collapse_and_named_wins(tmp_path): + """Naming a file that also lives inside a named directory keeps it named. + + Otherwise the user's own explicitly typed path would be demoted to a discovered + entry, and its validation failure would warn-and-skip instead of aborting. + """ + d = tmp_path / "d" + d.mkdir() + inner = d / "inner.nc" + inner.write_text("data") + + entries, _skips = rimport.expand_directories([d, inner]) + + assert entries == [rimport.Entry(inner, True)] + + +def test_duplicate_order_is_first_seen(tmp_path): + """De-duplication preserves the order a path was first encountered.""" + d = tmp_path / "d" + d.mkdir() + (d / "a.nc").write_text("a") + (d / "b.nc").write_text("b") + + entries, _skips = rimport.expand_directories([d, d]) + + assert [e.path.name for e in entries] == ["a.nc", "b.nc"] + + +def test_walk_skips_are_passed_through(tmp_path): + """An unreadable directory found during expansion surfaces as a Skip.""" + d = tmp_path / "d" + d.mkdir() + locked = d / "locked" + locked.mkdir() + os.chmod(locked, 0o000) + + try: + _entries, skips = rimport.expand_directories([d]) + finally: + os.chmod(locked, 0o700) + + assert len(skips) == 1 + assert skips[0].path == locked + + +def test_empty_directory_warns_but_is_not_a_skip(tmp_path, caplog): + """A named directory containing nothing is worth saying out loud, but is not an error + and must not affect the exit code.""" + d = tmp_path / "empty" + d.mkdir() + + with caplog.at_level(logging.WARNING, logger="rimport_relink"): + entries, skips = rimport.expand_directories([d]) + + assert entries == [] + assert skips == [] + assert f"no files found under {d}" in caplog.text + + +def test_logs_expansion_counts(tmp_path, caplog): + """The blast radius is visible before anything is staged.""" + d1 = tmp_path / "d1" + d1.mkdir() + (d1 / "a.nc").write_text("a") + d2 = tmp_path / "d2" + d2.mkdir() + (d2 / "b.nc").write_text("b") + (d2 / "c.nc").write_text("c") + + with caplog.at_level(logging.INFO, logger="rimport_relink"): + rimport.expand_directories([d1, d2]) + + assert "expanded 2 director(ies) to 3 file(s)" in caplog.text + + +def test_no_expansion_logs_no_count_line(tmp_path, caplog): + """A batch of plain files should not emit a confusing 'expanded 0' line.""" + f = tmp_path / "f.nc" + f.write_text("data") + + with caplog.at_level(logging.INFO, logger="rimport_relink"): + rimport.expand_directories([f]) + + assert "expanded" not in caplog.text From 5a50396d49aa5a7d7a687a0390cccebb9490c736 Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Mon, 31 Aug 2026 14:02:55 -0600 Subject: [PATCH 04/47] tests(rimport): red tests for empty-argument rejection Fail by design. Today an empty name anchors to cwd and yields the cwd itself, which was harmless only because a directory was then rejected. Once directories expand, `rimport ""` from an unset shell variable would recursively publish the subtree the user is standing in -- the hazard named in 75c79cd. Pin the rejection before the behavior that needs it. Co-Authored-By: Claude Opus 5 (1M context) --- tests/rimport/test_get_files_to_process.py | 38 ++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/tests/rimport/test_get_files_to_process.py b/tests/rimport/test_get_files_to_process.py index e709e5e..7d0c6ce 100644 --- a/tests/rimport/test_get_files_to_process.py +++ b/tests/rimport/test_get_files_to_process.py @@ -646,3 +646,41 @@ def test_deleted_cwd_with_multiple_relative_names_reports_all( assert filename in caplog.text for item_name in item_names: assert item_name in caplog.text + + def test_empty_positional_is_rejected(self, caplog): + """An empty positional must not anchor to cwd and become a whole-tree expansion.""" + with caplog.at_level(logging.ERROR, logger="rimport_relink"): + files, status = rimport.get_files_to_process(None, None, [""]) + + assert files is None + assert status == 2 + assert "empty filename" in caplog.text + + def test_empty_file_option_is_rejected(self, caplog): + """Same guard on --file.""" + with caplog.at_level(logging.ERROR, logger="rimport_relink"): + files, status = rimport.get_files_to_process("", None, []) + + assert files is None + assert status == 2 + assert "empty filename" in caplog.text + + def test_whitespace_only_argument_is_rejected(self, caplog): + """A name that is empty after stripping is just as dangerous as ''.""" + with caplog.at_level(logging.ERROR, logger="rimport_relink"): + files, status = rimport.get_files_to_process(None, None, [" "]) + + assert files is None + assert status == 2 + assert "empty filename" in caplog.text + + def test_empty_argument_rejected_even_alongside_valid_ones(self, tmp_path, caplog): + """One empty name poisons the batch; nothing is resolved.""" + good = tmp_path / "good.nc" + good.write_text("data") + + with caplog.at_level(logging.ERROR, logger="rimport_relink"): + files, status = rimport.get_files_to_process(None, None, [str(good), ""]) + + assert files is None + assert status == 2 From 7fcc0d11866dadf94d198d5a9d82baaff69ea7da Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Mon, 31 Aug 2026 14:13:07 -0600 Subject: [PATCH 05/47] tests(rimport): red tests for main's provenance split and exit codes Fail by design. Pin the asymmetry the design turns on: a path the user typed is still fatal and aborts everything, while a bad file found by a walk warns, is skipped, and lets its neighbours publish. Also pin exit 3 for completed-with-skips, precedence 1 > 3 so a staging failure is never masked, --check sharing the same codes, and the skip being reported twice -- inline on stdout, then repeated on stderr at the very end. Swap the empty directory in the pre-flight gate test for a broken symlink: an empty directory is no longer a failure once directories expand, so leaving it there would quietly weaken the test to a single failure mode. Co-Authored-By: Claude Opus 5 (1M context) --- tests/rimport/test_main.py | 133 +++++++++++++++++++++++++++++++++++-- 1 file changed, 127 insertions(+), 6 deletions(-) diff --git a/tests/rimport/test_main.py b/tests/rimport/test_main.py index 04ba444..fb98bfc 100644 --- a/tests/rimport/test_main.py +++ b/tests/rimport/test_main.py @@ -517,8 +517,9 @@ def test_preflight_gate_rejects_whole_batch_and_never_calls_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. + """Test main()'s pre-flight gate: a batch with a mix of valid and invalid paths + (missing, and a broken symlink) 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 @@ -534,8 +535,8 @@ def test_preflight_gate_rejects_whole_batch_and_never_calls_stage_data( valid = inputdata_root / "good.nc" valid.write_text("data") missing = inputdata_root / "missing.nc" - bad_dir = inputdata_root / "adir" - bad_dir.mkdir() + broken = inputdata_root / "broken.nc" + broken.symlink_to(inputdata_root / "nonexistent_target.nc") result = rimport.main( [ @@ -543,7 +544,7 @@ def test_preflight_gate_rejects_whole_batch_and_never_calls_stage_data( str(inputdata_root), str(valid), str(missing), - str(bad_dir), + str(broken), ] ) @@ -553,8 +554,128 @@ def test_preflight_gate_rejects_whole_batch_and_never_calls_stage_data( 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 + assert f"Source is a broken symlink: {broken}" 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() + + @patch.object(rimport, "get_staging_root") + @patch.object(rimport, "ensure_running_as") + def test_directory_argument_stages_the_files_inside_it( + self, _mock_ensure_running_as, mock_get_staging_root, tmp_path, capsys + ): + """A directory argument publishes the files beneath it, and is never itself staged.""" + inputdata_root = tmp_path / "inputdata" + subdir = inputdata_root / "lnd" + subdir.mkdir(parents=True) + staging_root = tmp_path / "staging" + staging_root.mkdir() + mock_get_staging_root.return_value = staging_root + (subdir / "a.nc").write_text("a") + (subdir / "b.nc").write_text("b") + + result = rimport.main(["-inputdata", str(inputdata_root), str(subdir)]) + + assert result == 0 + assert (staging_root / "lnd" / "a.nc").read_text() == "a" + assert (staging_root / "lnd" / "b.nc").read_text() == "b" + assert (subdir / "a.nc").is_symlink() + assert subdir.is_dir() and not subdir.is_symlink() + assert not (staging_root / "lnd").is_symlink() + + @patch.object(rimport, "get_staging_root") + @patch.object(rimport, "ensure_running_as") + def test_discovered_failure_warns_skips_and_returns_3( + self, _mock_ensure_running_as, mock_get_staging_root, tmp_path, capsys + ): + """A bad file FOUND BY A WALK must not abort the batch: the good files still + publish, the bad one is skipped, and the run reports 3.""" + inputdata_root = tmp_path / "inputdata" + subdir = inputdata_root / "lnd" + subdir.mkdir(parents=True) + staging_root = tmp_path / "staging" + staging_root.mkdir() + mock_get_staging_root.return_value = staging_root + (subdir / "good.nc").write_text("good") + (subdir / "broken.nc").symlink_to(inputdata_root / "nonexistent.nc") + + result = rimport.main(["-inputdata", str(inputdata_root), str(subdir)]) + + assert result == 3 + assert (staging_root / "lnd" / "good.nc").read_text() == "good" + captured = capsys.readouterr() + # Reported inline at WARNING (stdout) as the run reaches it... + assert "skipping" in captured.out + # ...and repeated at ERROR (stderr) at the very end, where it cannot be scrolled past. + assert "1 file(s) skipped (not stageable)" in captured.err + assert "broken.nc" in captured.err + + @patch.object(rimport, "get_staging_root") + @patch.object(rimport, "ensure_running_as") + def test_named_failure_still_aborts_everything_including_discovered_files( + self, _mock_ensure_running_as, mock_get_staging_root, tmp_path, capsys + ): + """Provenance asymmetry: a path the USER typed is still fatal, and it takes the + whole batch down with it -- including good files discovered under a good + directory.""" + inputdata_root = tmp_path / "inputdata" + subdir = inputdata_root / "lnd" + subdir.mkdir(parents=True) + staging_root = tmp_path / "staging" + staging_root.mkdir() + mock_get_staging_root.return_value = staging_root + (subdir / "good.nc").write_text("good") + missing = inputdata_root / "missing.nc" + + result = rimport.main( + ["-inputdata", str(inputdata_root), str(subdir), str(missing)] + ) + + assert result == 2 + assert not any(staging_root.rglob("*")) + assert not (subdir / "good.nc").is_symlink() + captured = capsys.readouterr() + assert "nothing was published" in captured.err + + @patch.object(rimport, "get_staging_root") + @patch.object(rimport, "ensure_running_as") + def test_staging_error_outranks_skip_in_exit_code( + self, _mock_ensure_running_as, mock_get_staging_root, tmp_path + ): + """Precedence 1 > 3: a real staging failure must not be masked by 'completed with + skips'.""" + inputdata_root = tmp_path / "inputdata" + subdir = inputdata_root / "lnd" + subdir.mkdir(parents=True) + staging_root = tmp_path / "staging" + staging_root.mkdir() + mock_get_staging_root.return_value = staging_root + (subdir / "good.nc").write_text("good") + (subdir / "broken.nc").symlink_to(inputdata_root / "nonexistent.nc") + + with patch.object(rimport, "stage_data", side_effect=RuntimeError("boom")): + result = rimport.main(["-inputdata", str(inputdata_root), str(subdir)]) + + assert result == 1 + + @patch.object(rimport, "get_staging_root") + @patch.object(rimport, "ensure_running_as") + def test_check_mode_also_returns_3_for_skips( + self, _mock_ensure_running_as, mock_get_staging_root, tmp_path + ): + """--check uses the same exit codes, including 3.""" + inputdata_root = tmp_path / "inputdata" + subdir = inputdata_root / "lnd" + subdir.mkdir(parents=True) + staging_root = tmp_path / "staging" + staging_root.mkdir() + mock_get_staging_root.return_value = staging_root + (subdir / "good.nc").write_text("good") + (subdir / "broken.nc").symlink_to(inputdata_root / "nonexistent.nc") + + result = rimport.main( + ["-inputdata", str(inputdata_root), str(subdir), "--check"] + ) + + assert result == 3 From 65f9de632340358ab4518b7f47060373984ada66 Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Mon, 31 Aug 2026 14:16:54 -0600 Subject: [PATCH 06/47] tests(rimport): pin the named-vs-discovered count in the abort-everything test test_named_failure_still_aborts_everything_including_discovered_files passed today for the wrong reason: a named directory is currently rejected outright, so it and the named `missing` path both fail pre-flight ("2 of 2"), giving the same externally-observable result the test already checked for. That made the test pass both before and after directory enumeration, pinning nothing. Add an assertion on the pre-flight count itself: after enumeration only the named `missing` path is fatal, while good.nc (discovered under subdir) is not counted against it, so the message must read "1 of 2", not "2 of 2". This makes the test red now and green only once the named/discovered split is implemented. Co-Authored-By: Claude Opus 5 (1M context) --- tests/rimport/test_main.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/rimport/test_main.py b/tests/rimport/test_main.py index fb98bfc..616f969 100644 --- a/tests/rimport/test_main.py +++ b/tests/rimport/test_main.py @@ -637,6 +637,10 @@ def test_named_failure_still_aborts_everything_including_discovered_files( assert not (subdir / "good.nc").is_symlink() captured = capsys.readouterr() assert "nothing was published" in captured.err + # Pins the named-vs-discovered split itself: only the named `missing` is fatal, + # so the count is 1 of 2 (good.nc, discovered under subdir, does not count against + # it) -- not 2 of 2, which is what today's un-enumerated pre-flight gate reports. + assert "1 of 2 file(s) failed pre-flight validation" in captured.err @patch.object(rimport, "get_staging_root") @patch.object(rimport, "ensure_running_as") From 7405fce95fcf2b7c374b028d40dac84dda5aab81 Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Mon, 31 Aug 2026 14:24:03 -0600 Subject: [PATCH 07/47] tests(rimport): assert the relink half of staging, not just the copy test_discovered_failure_warns_skips_and_returns_3 checked that good.nc's content landed under staging_root, but never that the original was replaced with a symlink pointing at it. Staging has two halves -- copy, then relink -- and an implementation that copied but forgot to relink would have passed. Red at HEAD by design: the directory guard still rejects a directory argument outright. Co-Authored-By: Claude Opus 5 (1M context) --- tests/rimport/test_main.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/rimport/test_main.py b/tests/rimport/test_main.py index 616f969..9fc249f 100644 --- a/tests/rimport/test_main.py +++ b/tests/rimport/test_main.py @@ -604,6 +604,7 @@ def test_discovered_failure_warns_skips_and_returns_3( assert result == 3 assert (staging_root / "lnd" / "good.nc").read_text() == "good" + assert (subdir / "good.nc").is_symlink() captured = capsys.readouterr() # Reported inline at WARNING (stdout) as the run reaches it... assert "skipping" in captured.out From f3e66a5da299f022d570c91aed53706f105f011b Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Mon, 31 Aug 2026 14:27:26 -0600 Subject: [PATCH 08/47] tests(rimport): red tests for directory and exit-code help text Fail by design. rimport's --help is meant to stand on its own (69b3e47), so the new behavior has to be discoverable there: that a directory argument is enumerated recursively, that a symlink to a directory is the carve-out and is not expanded, and that there are now four exit codes rather than three. Co-Authored-By: Claude Opus 5 (1M context) --- tests/rimport/test_build_parser.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/rimport/test_build_parser.py b/tests/rimport/test_build_parser.py index b54d710..61ead5c 100644 --- a/tests/rimport/test_build_parser.py +++ b/tests/rimport/test_build_parser.py @@ -199,3 +199,24 @@ def test_quiet_and_verbose_mutually_exclusive(self, capsys): captured = capsys.readouterr() stderr_lines = captured.err.strip().split("\n") assert "not allowed with argument" in stderr_lines[-1] + + def test_help_documents_directory_expansion(self): + """A user reading --help must learn that a directory argument is enumerated.""" + help_text = rimport.build_parser().format_help() + + assert "director" in help_text.lower() + assert "recursiv" in help_text.lower() + + def test_help_documents_that_directory_symlinks_are_not_expanded(self): + """The one surprising carve-out belongs in the help, not just the source.""" + help_text = rimport.build_parser().format_help() + + assert "symlink to a directory is not expanded" in help_text + + def test_help_documents_all_four_exit_codes(self): + """Exit 3 is new and scriptable; all four codes must be discoverable.""" + help_text = rimport.build_parser().format_help() + + for code in ["0", "1", "2", "3"]: + assert f"{code}:" in help_text + assert "skipped" in help_text.lower() From 3905824c6b80555c4a3c6572b7e1e3f0045e527c Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Mon, 31 Aug 2026 14:32:11 -0600 Subject: [PATCH 09/47] =?UTF-8?q?tests(rimport):=20fix=20round=201=20?= =?UTF-8?q?=E2=80=93=20strengthen=20discriminators=20in=20help-text=20test?= =?UTF-8?q?s?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two initial red tests had loose assertions: - test_help_documents_directory_expansion only required "director" (already present in 'inputdata directory') and "recursiv" separately, allowing either to pass independently without documenting recursive enumeration together. - test_help_documents_all_four_exit_codes looked for "0:", "1:", "2:", "3:" and "skipped" anywhere in the help text, not anchored to an "exit codes:" section. Both defects defeated the point of these red tests: to force later implementation of actual help text. Fixed: - test_help_documents_directory_expansion now checks both "directory" and "enumerated recursively" appear together via whitespace-normalized text (to account for argparse line wrapping). - test_help_documents_all_four_exit_codes now anchors all four codes and "skipped" to the "exit codes:" section. Both tests still fail (red by design) because today's help contains neither "enumerated recursively" nor an "exit codes:" section. Co-Authored-By: Claude Opus 5 (1M context) --- tests/rimport/test_build_parser.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/tests/rimport/test_build_parser.py b/tests/rimport/test_build_parser.py index 61ead5c..1c93dd3 100644 --- a/tests/rimport/test_build_parser.py +++ b/tests/rimport/test_build_parser.py @@ -203,9 +203,11 @@ def test_quiet_and_verbose_mutually_exclusive(self, capsys): def test_help_documents_directory_expansion(self): """A user reading --help must learn that a directory argument is enumerated.""" help_text = rimport.build_parser().format_help() + # argparse WRAPS argument help across lines, so match on whitespace-normalized text + normalized = " ".join(help_text.split()).lower() - assert "director" in help_text.lower() - assert "recursiv" in help_text.lower() + assert "directory" in normalized + assert "enumerated recursively" in normalized def test_help_documents_that_directory_symlinks_are_not_expanded(self): """The one surprising carve-out belongs in the help, not just the source.""" @@ -216,7 +218,10 @@ def test_help_documents_that_directory_symlinks_are_not_expanded(self): def test_help_documents_all_four_exit_codes(self): """Exit 3 is new and scriptable; all four codes must be discoverable.""" help_text = rimport.build_parser().format_help() + help_lower = help_text.lower() - for code in ["0", "1", "2", "3"]: - assert f"{code}:" in help_text - assert "skipped" in help_text.lower() + assert "exit codes:" in help_lower + exit_section = help_lower.split("exit codes:", 1)[1] + for code in ["0:", "1:", "2:", "3:"]: + assert code in exit_section + assert "skipped" in exit_section From cdeea465c1c25cf19c2f5d7d164b3ad9e2928f80 Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Mon, 31 Aug 2026 14:40:32 -0600 Subject: [PATCH 10/47] tests(rimport): red end-to-end tests for directory enumeration Fail by design. Flip the three tests that asserted the old directory-is-an-error contract, keeping the assertion that matters from 75c79cd: the directory itself is still never renamed, symlinked away, or left with a failed-rollback '.tmp'. That was always the real invariant; the error was only how it was enforced. The empty-string test keeps failing but for the new reason, and the mixed-validity list swaps its empty directory for a broken symlink, since an empty directory is no longer a failure. New coverage: recursion into subdirectories, the skip summary landing last on stderr and surviving -q, and the expansion count printing before anything is staged. Co-Authored-By: Claude Opus 5 (1M context) --- tests/rimport/test_cmdline.py | 206 ++++++++++++++++++++++++++-------- 1 file changed, 160 insertions(+), 46 deletions(-) diff --git a/tests/rimport/test_cmdline.py b/tests/rimport/test_cmdline.py index d08d89e..ff26c96 100644 --- a/tests/rimport/test_cmdline.py +++ b/tests/rimport/test_cmdline.py @@ -928,13 +928,13 @@ def test_check_doesnt_relink_published(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_directory_argument_from_subdir_errors_and_leaves_tree_intact( + def test_directory_argument_from_subdir_stages_contents_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.""" + inside an inputdata subdir) enumerates the files beneath it and stages them, instead of + falling into the destructive replace-with-symlink path, which would rename the directory + itself to '.tmp', symlink it away, and then fail to roll back.""" inputdata_root = test_env["inputdata_root"] staging_root = test_env["staging_root"] @@ -967,28 +967,28 @@ def test_directory_argument_from_subdir_errors_and_leaves_tree_intact( 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 + assert result.returncode == 0, f"Command unexpectedly failed: {result.stderr}" + + # The file inside was published and relinked. + assert (staging_mirror / "data.nc").read_text() == "clm2 data" + assert inner_file.is_symlink() - # 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. + # The 75c79cd anti-corruption assertion, preserved: the DIRECTORY itself was never + # renamed, never symlinked away, and no failed-rollback '.tmp' was left behind. assert subdir.is_dir() and not subdir.is_symlink(), ( - f"clm2 should still be a plain, non-symlink directory after the error; " + f"clm2 should still be a plain, non-symlink directory; " 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.""" + itself and, now that directories are enumerated, recursively publishing the whole + subtree.""" 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="." — the same thing that @@ -1018,26 +1018,19 @@ def test_empty_string_argument_errors_and_leaves_tree_intact( ) # 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" + assert result.returncode == 2, f"Command unexpectedly passed: {result.stdout}" + assert "empty filename" in result.stderr - def test_check_directory_argument_reports_error_not_publishable( + # The inputdata root was NOT expanded and published wholesale. + assert not list(test_env["staging_root"].rglob("*")) + assert not marker_file.is_symlink() + + def test_check_directory_argument_reports_each_file_inside( self, rimport_script, test_env, rimport_env ): - """Test that --check on a directory argument reports it as an error, rather than - misreporting the directory as already published but not linked and available for - download.""" + """Test that --check on a directory argument enumerates the files beneath it and + reports on each one individually, rather than describing the directory itself as + already published or available for download.""" inputdata_root = test_env["inputdata_root"] staging_root = test_env["staging_root"] @@ -1068,24 +1061,25 @@ def test_check_directory_argument_reports_error_not_publishable( 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 + assert result.returncode == 0, f"Command unexpectedly failed: {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() + # --check reports on the file inside, and stages nothing. + assert "data.nc" in result.stdout + assert not (staging_mirror / "data.nc").exists() + assert not inner_file.is_symlink() - # Verify the tree is intact + # The file is reported on its own terms, and the directory itself is never + # described as published or downloadable. + assert "not already published" in result.stdout + assert "available for download" not in result.stdout.lower() assert subdir.is_dir() and not subdir.is_symlink() assert not list(inputdata_root.rglob("*.tmp")) assert inner_file.read_text() == "clm2 data" 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 against it -- with + in DIFFERENT ways (missing.nc, and a broken symlink named broken.nc), all as absolute + paths in 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. @@ -1110,12 +1104,12 @@ def _run_mixed_validity_list(self, rimport_script, test_env, rimport_env, *, che missing_file = inputdata_root / "missing.nc" - bad_dir = inputdata_root / "adir" - bad_dir.mkdir() + broken_link = inputdata_root / "broken.nc" + broken_link.symlink_to(inputdata_root / "nonexistent_target.nc") # 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") + filelist.write_text(f"{valid_file}\n{missing_file}\n{broken_link}\n") command = [ sys.executable, @@ -1143,7 +1137,7 @@ def _run_mixed_validity_list(self, rimport_script, test_env, rimport_env, *, che 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 + assert f"Source is a broken symlink: {broken_link}" in result.stderr return result, valid_file, staging_root @@ -1151,7 +1145,7 @@ 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 + that are invalid in DIFFERENT ways (missing, and a broken symlink) 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. @@ -1193,3 +1187,123 @@ def test_check_mode_is_gated_too_and_reports_nothing_for_valid_entry( # Verify nothing was staged assert not any(staging_root.rglob("*")) assert not valid_file.is_symlink() + + def test_directory_argument_recurses_into_subdirectories( + self, rimport_script, test_env, rimport_env + ): + """Enumeration is recursive, and the mirrored staging structure is preserved.""" + inputdata_root = test_env["inputdata_root"] + staging_root = test_env["staging_root"] + + deep = inputdata_root / "lnd" / "clm2" / "paramdata" + deep.mkdir(parents=True) + (inputdata_root / "lnd" / "top.nc").write_text("top") + (deep / "deep.nc").write_text("deep") + + result = subprocess.run( + [ + sys.executable, + rimport_script, + str(inputdata_root / "lnd"), + "-inputdata", + str(inputdata_root), + ], + capture_output=True, + text=True, + check=False, + env=rimport_env, + ) + + assert result.returncode == 0, f"Command unexpectedly failed: {result.stderr}" + assert (staging_root / "lnd" / "top.nc").read_text() == "top" + assert (staging_root / "lnd" / "clm2" / "paramdata" / "deep.nc").read_text() == "deep" + + def test_skip_summary_repeats_skipped_files_on_stderr_at_the_end( + self, rimport_script, test_env, rimport_env + ): + """A skipped file must survive a long scroll: reported inline on stdout, then + repeated in a summary block on stderr after everything else.""" + inputdata_root = test_env["inputdata_root"] + + subdir = inputdata_root / "lnd" + subdir.mkdir() + (subdir / "good.nc").write_text("good") + (subdir / "broken.nc").symlink_to(inputdata_root / "nonexistent.nc") + + result = subprocess.run( + [ + sys.executable, + rimport_script, + str(subdir), + "-inputdata", + str(inputdata_root), + ], + capture_output=True, + text=True, + check=False, + env=rimport_env, + ) + + assert result.returncode == 3 + assert "1 file(s) skipped (not stageable)" in result.stderr + assert "broken.nc" in result.stderr + # The summary is the LAST thing on stderr. The broken-symlink message names the + # link itself, not its target, so the final line ends with broken.nc. + assert result.stderr.rstrip().endswith("broken.nc") + + def test_skip_summary_survives_quiet_mode( + self, rimport_script, test_env, rimport_env + ): + """-q hides progress but must not hide what went unpublished.""" + inputdata_root = test_env["inputdata_root"] + + subdir = inputdata_root / "lnd" + subdir.mkdir() + (subdir / "good.nc").write_text("good") + (subdir / "broken.nc").symlink_to(inputdata_root / "nonexistent.nc") + + result = subprocess.run( + [ + sys.executable, + rimport_script, + str(subdir), + "-inputdata", + str(inputdata_root), + "-q", + ], + capture_output=True, + text=True, + check=False, + env=rimport_env, + ) + + assert result.returncode == 3 + assert "1 file(s) skipped (not stageable)" in result.stderr + + def test_expansion_count_is_logged_before_staging( + self, rimport_script, test_env, rimport_env + ): + """The blast radius is visible before anything is written.""" + inputdata_root = test_env["inputdata_root"] + + subdir = inputdata_root / "lnd" + subdir.mkdir() + (subdir / "a.nc").write_text("a") + (subdir / "b.nc").write_text("b") + + result = subprocess.run( + [ + sys.executable, + rimport_script, + str(subdir), + "-inputdata", + str(inputdata_root), + ], + capture_output=True, + text=True, + check=False, + env=rimport_env, + ) + + assert result.returncode == 0 + assert "expanded 1 director(ies) to 2 file(s)" in result.stdout From b80f41dc67010dadcbe34aefa22c06e56aeb89d6 Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Mon, 31 Aug 2026 14:49:46 -0600 Subject: [PATCH 11/47] rimport: Add walk_files and the Entry/Skip types Turns tests/rimport/test_walk_files.py green. The walk never descends through a symlink, so it cannot leave the tree the user named and cannot loop on a cycle; a symlink is always a leaf entry and validate_source_path decides what it means. Unlike relink's walker there is no owner filter and symlinks are not skipped -- rimport runs as the staging owner and needs to see every entry. Per-level sorting makes output and tests deterministic. Co-Authored-By: Claude Opus 5 (1M context) --- rimport | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 69 insertions(+), 1 deletion(-) diff --git a/rimport b/rimport index 408287e..ee8b61a 100755 --- a/rimport +++ b/rimport @@ -15,7 +15,7 @@ import pwd import shutil import sys from pathlib import Path -from typing import Iterable, List +from typing import Iterable, List, NamedTuple from urllib.request import Request, urlopen from urllib.error import HTTPError @@ -163,6 +163,74 @@ def normalize_paths(root: Path, relnames: Iterable[str]) -> List[Path]: return paths +class Entry(NamedTuple): + """One path to consider staging, and how it got into the batch. + + `named` is True when the user typed this path (positional, `--file`, or a `--list` + entry) and False when a directory walk discovered it. The distinction is what lets a + bad file inside a large tree warn-and-skip while a bad path the user asked for by name + still aborts the whole batch. + """ + + path: Path + named: bool + + +class Skip(NamedTuple): + """One path that will not be staged, and the reason why. + + Both validation failures on discovered files and directories that could not be read + become Skips, so `report_skips` can present them in one block. + """ + + path: Path + reason: Exception + + +def walk_files(root: Path) -> tuple[List[Path], List[Skip]]: + """Recursively list the non-directory entries under `root`. + + Descends only into real directories (`follow_symlinks=False`), so the walk cannot + leave the tree the user named and cannot loop on a cyclic link. A symlink is therefore + always a leaf: it is returned as an entry to act on, including when its target happens + to be a directory, and `validate_source_path` decides what that means. + + Every non-directory entry is returned -- regular files, symlinks, dotfiles alike. There + is no owner filter (unlike relink's walker): rimport runs as the staging owner and the + point is to publish what is there. + + Entries are sorted at each level so output and tests are deterministic; `os.scandir` + order is otherwise arbitrary. + + Args: + root: Directory to walk. + + Returns: + (files, skips). `files` are absolute paths in depth-first, per-level sorted order. + `skips` are directories that could not be read; an unreadable directory does not + raise and does not abort the rest of the walk. + """ + files: List[Path] = [] + skips: List[Skip] = [] + + try: + with os.scandir(root) as scan: + children = sorted(scan, key=lambda entry: entry.name) + except OSError as exc: + return [], [Skip(Path(root), exc)] + + for child in children: + child_path = Path(child.path) + if child.is_dir(follow_symlinks=False): + sub_files, sub_skips = walk_files(child_path) + files.extend(sub_files) + skips.extend(sub_skips) + else: + files.append(child_path) + + return files, skips + + def check_relink_worked(src: Path, dst: Path) -> None: """Check whether relink worked From bc7679d57f1cf3d79f04c7575199104c27c24eb6 Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Mon, 31 Aug 2026 14:55:25 -0600 Subject: [PATCH 12/47] rimport: Add expand_directories with named-vs-discovered provenance Turns tests/rimport/test_expand_directories.py green. Expansion deliberately does no validation, so a nonexistent named path still reaches the pre-flight gate and gets its usual message. A symlink to a directory is left as one named entry, matching walk_files. When a path is both named and discovered, named wins: demoting a path the user typed would turn its failure from fatal into a skip. The expansion count is logged here rather than in main -- this is the only place holding both numbers. Co-Authored-By: Claude Opus 5 (1M context) --- rimport | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/rimport b/rimport index ee8b61a..a3abc11 100755 --- a/rimport +++ b/rimport @@ -231,6 +231,52 @@ def walk_files(root: Path) -> tuple[List[Path], List[Skip]]: return files, skips +def expand_directories(paths: Iterable[Path]) -> tuple[List[Entry], List[Skip]]: + """Replace each directory in `paths` with the files beneath it, tagging provenance. + + A path is expanded only if it is a real directory. A symlink to a directory is left + alone as a single named entry, matching `walk_files`' refusal to descend through one. + Nothing here validates: a nonexistent path passes straight through as named, so + `main`'s pre-flight gate can reject it with its usual message. + + Duplicates collapse in first-seen order. If the same path is both named and discovered + -- the user typed a file that also lives inside a directory they named -- `named` wins, + so the path the user asked for by name keeps its fatal-on-failure treatment. + + Args: + paths: Absolute paths from `normalize_paths`. + + Returns: + (entries, skips). `skips` carries directories that could not be read during a walk. + """ + named_by_path: dict[Path, bool] = {} + skips: List[Skip] = [] + n_dirs = 0 + n_expanded_files = 0 + + for path in paths: + if path.is_dir() and not path.is_symlink(): + n_dirs += 1 + found, walk_skips = walk_files(path) + skips.extend(walk_skips) + n_expanded_files += len(found) + if not found: + logger.warning("rimport: no files found under %s", path) + for found_path in found: + named_by_path.setdefault(found_path, False) + else: + # A named path always wins over the same path discovered by a walk. + named_by_path[path] = True + + if n_dirs: + logger.info( + "rimport: expanded %d director(ies) to %d file(s)", n_dirs, n_expanded_files + ) + + entries = [Entry(path, named) for path, named in named_by_path.items()] + return entries, skips + + def check_relink_worked(src: Path, dst: Path) -> None: """Check whether relink worked From 13e5854d6745ec5480212ad6944e069830485972 Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Mon, 31 Aug 2026 15:00:40 -0600 Subject: [PATCH 13/47] rimport: Fix expand_directories count to report distinct files, not sum The n_expanded_files counter was summing len(found) from each walk before the dict deduplication, which overstates the file count in the log message. When expand_directories receives overlapping directory arguments like [d, d], it would report "expanded 2 director(ies) to 4 file(s)" when only 2 distinct files would be staged. Since the expansion count's only purpose is to show the blast radius before staging, accuracy matters at the moment the operator reads it. Fix: use a set to track distinct found files; count len(expanded_files) in the log line instead of a running sum. This preserves the honest count of n_dirs (a directory argument named twice really is two arguments) while fixing the file count to match deduplication. Co-Authored-By: Claude Opus 5 (1M context) --- rimport | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/rimport b/rimport index a3abc11..c9daa9f 100755 --- a/rimport +++ b/rimport @@ -252,17 +252,17 @@ def expand_directories(paths: Iterable[Path]) -> tuple[List[Entry], List[Skip]]: named_by_path: dict[Path, bool] = {} skips: List[Skip] = [] n_dirs = 0 - n_expanded_files = 0 + expanded_files: set[Path] = set() for path in paths: if path.is_dir() and not path.is_symlink(): n_dirs += 1 found, walk_skips = walk_files(path) skips.extend(walk_skips) - n_expanded_files += len(found) if not found: logger.warning("rimport: no files found under %s", path) for found_path in found: + expanded_files.add(found_path) named_by_path.setdefault(found_path, False) else: # A named path always wins over the same path discovered by a walk. @@ -270,7 +270,7 @@ def expand_directories(paths: Iterable[Path]) -> tuple[List[Entry], List[Skip]]: if n_dirs: logger.info( - "rimport: expanded %d director(ies) to %d file(s)", n_dirs, n_expanded_files + "rimport: expanded %d director(ies) to %d file(s)", n_dirs, len(expanded_files) ) entries = [Entry(path, named) for path, named in named_by_path.items()] From 3722d364f9d011dbeaff196acbd3427ab7d0efe8 Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Mon, 31 Aug 2026 15:06:43 -0600 Subject: [PATCH 14/47] rimport: Reject empty filename arguments Turns the new tests in tests/rimport/test_get_files_to_process.py green. An empty name anchors to cwd and resolves to the cwd itself. That was survivable only because a directory was then rejected. Directory enumeration removes that accidental safety net, so `rimport "$unset_var"` would recursively publish the subtree the user is standing in -- the hazard 75c79cd called out. Refuse an empty name before anchoring. Co-Authored-By: Claude Opus 5 (1M context) --- rimport | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/rimport b/rimport index c9daa9f..01f9b50 100755 --- a/rimport +++ b/rimport @@ -549,6 +549,10 @@ def get_files_to_process(file: str, filelist: str, items_to_process: list): entries, and all `--list` entries (which never anchor to cwd), are unaffected by an undeterminable cwd. + An empty or whitespace-only `file`/`items_to_process` entry is rejected with `(None, 2)` + before any anchoring happens, because an empty name would otherwise resolve to the cwd + and be expanded. + Args: file (str): Single file to process. filelist (str): File containing list of files to process. @@ -559,6 +563,18 @@ def get_files_to_process(file: str, filelist: str, items_to_process: list): int: Result code """ cli_args = ([file] if file is not None else []) + list(items_to_process or []) + # An empty name would anchor to cwd and resolve to the cwd itself. That was harmless + # only while a directory was rejected outright; now that a directory expands, an unset + # shell variable (`rimport "$maybe_unset"`) would recursively publish the whole subtree + # the user is standing in. Refuse it. + empty_cli_args = [name for name in cli_args if not str(name).strip()] + if empty_cli_args: + logger.error( + "rimport: %d empty filename argument(s) given; refusing to resolve an empty " + "name to the current directory. Did a shell variable expand to nothing?", + len(empty_cli_args), + ) + return None, 2 relative_cli_args = [name for name in cli_args if not Path(name).is_absolute()] cwd = None From 1aa596ba4df4edfd79c9db3980beb733f5dddd80 Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Mon, 31 Aug 2026 15:16:29 -0600 Subject: [PATCH 15/47] rimport: Enumerate directory arguments instead of rejecting them Turns tests/rimport/test_main.py and the e2e tests in test_cmdline.py green. Pointing rimport at a directory now publishes the files beneath it. Pre-flight splits on provenance. A named bad path keeps today's fatal all-or-nothing behavior, so a batch containing one is never half completed. A discovered bad path warns, is skipped, and lets its neighbours publish -- one broken symlink in a large tree blocking thousands of good files would make directory arguments useless. Skips are reported inline where they happen, then repeated on stderr at the very end, and earn the new exit code 3. The is_dir() guard in validate_source_path stays. After expansion a directory should never reach it, which is exactly what makes it worth keeping as a backstop against the 75c79cd corruption. Co-Authored-By: Claude Opus 5 (1M context) --- rimport | 77 +++++++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 58 insertions(+), 19 deletions(-) diff --git a/rimport b/rimport index 01f9b50..e390e66 100755 --- a/rimport +++ b/rimport @@ -277,6 +277,25 @@ def expand_directories(paths: Iterable[Path]) -> tuple[List[Entry], List[Skip]]: return entries, skips +def report_skips(skips: List[Skip]) -> None: + """Re-list every skipped path at the very end of the run. + + Each skip was already reported inline, at WARNING level, where it happened. This block + repeats them at ERROR level -- so stderr, and so surviving `-q` -- because in a run over + a large tree the inline warnings scroll away and the whole point is that a file which + went unpublished cannot be missed. + + Args: + skips: Every skipped path with its reason. An empty list prints nothing. + """ + if not skips: + return + + logger.error("rimport: %d file(s) skipped (not stageable):", len(skips)) + for skip in skips: + logger.error("%s%s: %s", INDENT, skip.path, skip.reason) + + def check_relink_worked(src: Path, dst: Path) -> None: """Check whether relink worked @@ -694,39 +713,59 @@ def main(argv: List[str] | None = None) -> int: 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: + # Expand any directory argument into the files beneath it, tagging each path as named + # (the user asked for it) or discovered (a walk found it). + entries, skips = expand_directories(paths) + + # Pre-flight: validate everything before staging anything. A NAMED bad path is fatal and + # aborts the whole batch, so a batch containing one is never half-completed. A DISCOVERED + # bad path is only a skip: one unstageable file inside a large tree must not block the + # thousands of good ones around it. + named_failures = [] + to_stage = [] + for entry in entries: + error = validate_source_path(entry.path, root, staging_root) + if error is None: + to_stage.append(entry.path) + elif entry.named: + named_failures.append((entry.path, error)) + else: + logger.warning("%srimport: skipping '%s': %s", INDENT, entry.path, error) + skips.append(Skip(entry.path, error)) + + if named_failures: logger.error( "rimport: %d of %d file(s) failed pre-flight validation; nothing was published:", - len(failures), - len(paths), + len(named_failures), + len(entries), ) - for p, error in failures: - logger.error("%srimport: '%s': %s", INDENT, p, error) + for path, error in named_failures: + logger.error("%srimport: '%s': %s", INDENT, path, error) return 2 # Execute the new action per file errors = 0 - for p in paths: - logger.info("'%s':", p) + for path in to_stage: + logger.info("'%s':", path) try: - stage_data(p, root, staging_root, args.check) + stage_data(path, root, staging_root, args.check) except Exception as e: # pylint: disable=broad-exception-caught # General Exception keeps CLI robust for batch runs errors += 1 - logger.error("%srimport: error processing %s: %s", INDENT, p, e) + logger.error("%srimport: error processing %s: %s", INDENT, path, e) + if not errors and not args.check: + logger.info("\nNo need to run relink.py") + + # Last, so it cannot be scrolled past. + report_skips(skips) + + # Precedence 2 > 1 > 3 > 0; 2 already returned above. A real staging failure must never + # be masked by "completed with skips". if errors: return 1 - if not args.check: - logger.info("\nNo need to run relink.py") + if skips: + return 3 return 0 From 8a9e245ecc1d66f6022581beb37b0b707054e622 Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Mon, 31 Aug 2026 15:25:26 -0600 Subject: [PATCH 16/47] rimport: Document exit code 3 and annotate two locals in main main's docstring described only exit codes 0-2, leaving 3 (completed with skips) undocumented and making the 0 entry read as though a run with skips were unqualified success. Also annotates named_failures and to_stage, matching the file's existing convention of typing local collections. Co-Authored-By: Claude Opus 5 (1M context) --- rimport | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/rimport b/rimport index e390e66..b8fd074 100755 --- a/rimport +++ b/rimport @@ -656,14 +656,17 @@ def main(argv: List[str] | None = None) -> int: argv: Command-line arguments to parse. If None, uses sys.argv. Returns: - int: Exit code (0 for success, 1 if any files had errors, 2 for fatal errors). + int: Exit code (0 for success with nothing skipped, 1 if any files had errors, + 2 for fatal errors, or 3 if the run completed but one or more paths were + skipped). Environment Variables: RIMPORT_SKIP_USER_CHECK: Set to "1" to skip automatic user switching. RIMPORT_STAGING: Override the default staging root directory. Exit Codes: - 0: All files staged successfully (or, under --check, checked without error). + 0: All files staged successfully (or, under --check, checked without error), and + nothing was skipped. 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). @@ -683,6 +686,11 @@ def main(argv: List[str] | None = None) -> int: 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. + 3: The run finished -- nothing was rejected outright and no file failed while being + staged or relinked -- but one or more discovered paths (files found by expanding + a directory argument) failed pre-flight validation and were skipped rather than + staged. Each skip is warned about inline where it happens and the full list is + repeated on stderr at the very end, so a skipped file cannot be missed. """ parser = build_parser() args = parser.parse_args(argv) @@ -721,8 +729,8 @@ def main(argv: List[str] | None = None) -> int: # aborts the whole batch, so a batch containing one is never half-completed. A DISCOVERED # bad path is only a skip: one unstageable file inside a large tree must not block the # thousands of good ones around it. - named_failures = [] - to_stage = [] + named_failures: List[tuple[Path, Exception]] = [] + to_stage: List[Path] = [] for entry in entries: error = validate_source_path(entry.path, root, staging_root) if error is None: From 4685ca104f1623b62ce2d48b003caaa89affb6f4 Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Mon, 31 Aug 2026 15:30:03 -0600 Subject: [PATCH 17/47] rimport: Document directory expansion and exit codes in --help Turns tests/rimport/test_build_parser.py green. rimport's --help is meant to stand on its own (69b3e47). Say that a directory argument is enumerated recursively and that a symlink to a directory is the carve-out, list all four exit codes now that 3 exists, and narrow --check's all-or-nothing claim to the names the user gave -- a file found by enumeration is skipped individually, not fatal. Co-Authored-By: Claude Opus 5 (1M context) --- rimport | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/rimport b/rimport index b8fd074..2e5f828 100755 --- a/rimport +++ b/rimport @@ -44,6 +44,14 @@ def build_parser() -> argparse.ArgumentParser: f"Copy files from CESM inputdata directory ({DEFAULT_INPUTDATA_ROOT}) to a publishing" " directory, then replace the original with a symlink to the copy." ), + epilog=( + "exit codes:\n" + " 0: everything staged or checked, nothing skipped\n" + " 1: a file could not be staged\n" + " 2: a name you gave failed validation; nothing was published\n" + " 3: finished, but one or more files were skipped (listed at the end)\n" + ), + formatter_class=argparse.RawDescriptionHelpFormatter, add_help=False, # Disable automatic help to add custom -help flag ) @@ -55,7 +63,8 @@ def build_parser() -> argparse.ArgumentParser: help=( "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." + " inputdata root. If the name is a directory, every file beneath it is enumerated recursively and" + " acted on; a symlink to a directory is not expanded." ), ) @@ -67,7 +76,8 @@ def build_parser() -> argparse.ArgumentParser: help=( "Provide a file that contains a list of filenames to import. All filenames in the list" " must be in the CESM inputdata directory. A relative entry is resolved against the" - " list file's own directory, wherever that directory is." + " list file's own directory, wherever that directory is. A list entry naming a directory is enumerated recursively, the same as a name given" + " on the command line." ), ) @@ -77,7 +87,8 @@ def build_parser() -> argparse.ArgumentParser: help=( "One or more files to process. (Optional; can use --file instead to process just one.)" " A relative name is resolved against the current directory; there is no fallback to" - " the inputdata root." + " the inputdata root. If the name is a directory, every file beneath it is enumerated recursively and" + " acted on; a symlink to a directory is not expanded." ), ) @@ -91,8 +102,9 @@ def build_parser() -> argparse.ArgumentParser: action="store_true", 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." + " name that you gave aborts before any file is checked, reporting all bad names" + " at once; a file found by enumerating a directory is instead reported and" + " skipped individually." ), ) From 166e3fc205283ae0ed17d1a78d9ec5b826a95f33 Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Mon, 31 Aug 2026 15:36:21 -0600 Subject: [PATCH 18/47] rimport: Use custom formatter to wrap description while preserving epilog line breaks RawDescriptionHelpFormatter preserves the epilog's exit-code list, but it also stops re-wrapping the description, which renders as one long line on a narrow terminal. Replace it with a subclass that only raw-formats the epilog. Co-Authored-By: Claude Opus 5 (1M context) --- rimport | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/rimport b/rimport index 2e5f828..ca4db9e 100755 --- a/rimport +++ b/rimport @@ -33,6 +33,20 @@ INPUTDATA_URL = "https://osdf-data.gdex.ucar.edu/ncar/gdex/d651077/cesmdata/inpu logger = shared.logger +class _EpilogRawFormatter(argparse.HelpFormatter): + """Wrap the description as argparse normally would, but leave the epilog verbatim. + + RawDescriptionHelpFormatter would preserve the epilog's exit-code list, but it also + stops re-wrapping the description, which then renders as one long line on a narrow + terminal. Only the epilog needs its line breaks kept. + """ + + def _fill_text(self, text, width, indent): + if text.lstrip().startswith("exit codes:"): + return "".join(indent + line for line in text.splitlines(keepends=True)) + return super()._fill_text(text, width, indent) + + def build_parser() -> argparse.ArgumentParser: """Build and configure the argument parser for rimport. @@ -51,7 +65,7 @@ def build_parser() -> argparse.ArgumentParser: " 2: a name you gave failed validation; nothing was published\n" " 3: finished, but one or more files were skipped (listed at the end)\n" ), - formatter_class=argparse.RawDescriptionHelpFormatter, + formatter_class=_EpilogRawFormatter, add_help=False, # Disable automatic help to add custom -help flag ) From 24d9eae229eed3ef43642dc382c896ae9ab5eb15 Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Mon, 31 Aug 2026 15:49:33 -0600 Subject: [PATCH 19/47] tests(rimport): red test for a named unreadable directory `rimport ` exits 3 and publishes ok-dir anyway. A path the user NAMED that fails validation must be fatal -- exit 2, nothing published -- so this turns a hard stop into a partial publish. Skip carries no provenance, so main's gate cannot tell a named unreadable directory from one discovered inside a named tree. The second test pins what must NOT change: an unreadable directory discovered BENEATH a named one stays a warn-and-skip, so the fix cannot over-reach. It passes today and must keep passing. Red at HEAD by design. Co-Authored-By: Claude Opus 5 (1M context) --- tests/rimport/test_main.py | 70 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/tests/rimport/test_main.py b/tests/rimport/test_main.py index 9fc249f..c1680ab 100644 --- a/tests/rimport/test_main.py +++ b/tests/rimport/test_main.py @@ -684,3 +684,73 @@ def test_check_mode_also_returns_3_for_skips( ) assert result == 3 + + @patch.object(rimport, "get_staging_root") + @patch.object(rimport, "ensure_running_as") + def test_named_unreadable_directory_is_fatal_not_a_skip( + self, _mock_ensure_running_as, mock_get_staging_root, tmp_path, capsys + ): + """A directory the USER NAMED that cannot be read is a named failure, so it aborts + the batch -- it must not be demoted to a skip that lets other named arguments + publish anyway. + + Skip carries no provenance, so without an explicit check main's gate cannot tell a + named unreadable directory from one discovered inside a named tree. Getting this + wrong turns a hard stop into a partial publish. + """ + inputdata_root = tmp_path / "inputdata" + locked = inputdata_root / "locked" + locked.mkdir(parents=True) + (locked / "unreachable.nc").write_text("data") + other = inputdata_root / "ok" + other.mkdir() + (other / "good.nc").write_text("good") + staging_root = tmp_path / "staging" + staging_root.mkdir() + mock_get_staging_root.return_value = staging_root + os.chmod(locked, 0o000) + + try: + result = rimport.main( + ["-inputdata", str(inputdata_root), str(locked), str(other)] + ) + finally: + os.chmod(locked, 0o700) + + assert result == 2 + captured = capsys.readouterr() + assert "nothing was published" in captured.err + + # The OTHER named argument must not have published. + assert not (staging_root / "ok" / "good.nc").exists() + assert not (other / "good.nc").is_symlink() + + @patch.object(rimport, "get_staging_root") + @patch.object(rimport, "ensure_running_as") + def test_unreadable_subdirectory_stays_a_skip( + self, _mock_ensure_running_as, mock_get_staging_root, tmp_path, capsys + ): + """The counterpart: an unreadable directory DISCOVERED beneath a named directory is + not a named failure, so it stays a warn-and-skip and its readable siblings still + publish. This is what stops the fix for the named case from over-reaching.""" + inputdata_root = tmp_path / "inputdata" + tree = inputdata_root / "tree" + tree.mkdir(parents=True) + (tree / "good.nc").write_text("good") + locked = tree / "locked" + locked.mkdir() + (locked / "unreachable.nc").write_text("data") + staging_root = tmp_path / "staging" + staging_root.mkdir() + mock_get_staging_root.return_value = staging_root + os.chmod(locked, 0o000) + + try: + result = rimport.main(["-inputdata", str(inputdata_root), str(tree)]) + finally: + os.chmod(locked, 0o700) + + assert result == 3 + assert (staging_root / "tree" / "good.nc").read_text() == "good" + captured = capsys.readouterr() + assert "skipped (not stageable)" in captured.err From 4d3040c8c835fc9624f6da1eccb396c7b28551dd Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Mon, 31 Aug 2026 16:01:43 -0600 Subject: [PATCH 20/47] tests(rimport): pin the pre-flight count for a named unreadable directory Seeding named_failures from skips while leaving the message's denominator at len(entries) reports "1 of 0 file(s) failed pre-flight validation" for a lone unreadable directory, because a walk skip never produced an Entry. The exact wording is already treated as contract by three other tests, so pin it before the fix rather than discovering it afterwards. Also widens the nothing-published assertion to the whole staging tree, matching its neighbour, so a fix that publishes anything at all is caught. Co-Authored-By: Claude Opus 5 (1M context) --- tests/rimport/test_main.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/rimport/test_main.py b/tests/rimport/test_main.py index c1680ab..ea7df74 100644 --- a/tests/rimport/test_main.py +++ b/tests/rimport/test_main.py @@ -720,9 +720,13 @@ def test_named_unreadable_directory_is_fatal_not_a_skip( assert result == 2 captured = capsys.readouterr() assert "nothing was published" in captured.err + # Pins the denominator too: `locked` never became an Entry, so a count taken from + # entries alone reports the nonsense "1 of 0". Two paths were considered here -- + # `locked` and the good.nc discovered under `ok`. + assert "1 of 2 file(s) failed pre-flight validation" in captured.err - # The OTHER named argument must not have published. - assert not (staging_root / "ok" / "good.nc").exists() + # No named argument may have published -- not just the one that failed. + assert not any(staging_root.rglob("*")) assert not (other / "good.nc").is_symlink() @patch.object(rimport, "get_staging_root") From bbfed345dfcee8073c8f4b1984eda43ec36bddb2 Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Mon, 31 Aug 2026 16:04:35 -0600 Subject: [PATCH 21/47] rimport: Treat a named unreadable directory as a named failure A directory the user names that cannot be read comes back from walk_files as a Skip, and Skip carries no provenance, so main's gate could not see it as named. The run exited 3 and published every other named argument, turning a hard stop into a partial publish. Recognise it in main, which still holds the list of names the user gave. An unreadable directory discovered BENEATH a named one is untouched: the user did not name it, so it stays a warn-and-skip. Count it in the pre-flight message too. That denominator was len(entries), and a directory that could not be read never became an entry, so a lone one reported "1 of 0". Co-Authored-By: Claude Opus 5 (1M context) --- rimport | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/rimport b/rimport index ca4db9e..98ae140 100755 --- a/rimport +++ b/rimport @@ -755,7 +755,18 @@ def main(argv: List[str] | None = None) -> int: # aborts the whole batch, so a batch containing one is never half-completed. A DISCOVERED # bad path is only a skip: one unstageable file inside a large tree must not block the # thousands of good ones around it. - named_failures: List[tuple[Path, Exception]] = [] + # A directory the user NAMED that could not be read is a named failure, not a skip. + # Skip carries no provenance, so recognise it here by matching against the paths the + # user actually gave. An unreadable directory found BENEATH a named one stays a skip: + # the user did not name it, so it is discovered, and discovered failures warn and skip. + named_paths = set(paths) + named_failures: List[tuple[Path, Exception]] = [ + (skip.path, skip.reason) for skip in skips if skip.path in named_paths + ] + skips = [skip for skip in skips if skip.path not in named_paths] + # An unreadable named directory never became an Entry, so it has to be added to the + # count of paths considered; otherwise a lone one reports "1 of 0". + n_considered = len(entries) + len(named_failures) to_stage: List[Path] = [] for entry in entries: error = validate_source_path(entry.path, root, staging_root) @@ -771,7 +782,7 @@ def main(argv: List[str] | None = None) -> int: logger.error( "rimport: %d of %d file(s) failed pre-flight validation; nothing was published:", len(named_failures), - len(entries), + n_considered, ) for path, error in named_failures: logger.error("%srimport: '%s': %s", INDENT, path, error) From 9798d27bfdf6a74aa9552f163956f63df5d5f400 Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Mon, 31 Aug 2026 16:19:40 -0600 Subject: [PATCH 22/47] tests(rimport): red tests for how walk skips are reported Every skip should be reported twice: where it happened, and again in the end-of-run summary. Walk skips get neither -- nothing reports them inline, and the summary is unreachable past the fatal return -- so a skip during an aborted run is reported zero times. The same function also claims "no files found" for a directory it could not read, contradicting the Permission denied printed for that path. The third test pins what must NOT change: a NAMED unreadable directory is fatal, so it must not also be warned about as "skipping". It passes today and must keep passing. Red at HEAD by design. Co-Authored-By: Claude Opus 5 (1M context) --- tests/rimport/test_expand_directories.py | 55 ++++++++++++++++++++++++ tests/rimport/test_main.py | 33 ++++++++++++++ 2 files changed, 88 insertions(+) diff --git a/tests/rimport/test_expand_directories.py b/tests/rimport/test_expand_directories.py index 7c855ea..df20763 100644 --- a/tests/rimport/test_expand_directories.py +++ b/tests/rimport/test_expand_directories.py @@ -157,3 +157,58 @@ def test_no_expansion_logs_no_count_line(tmp_path, caplog): rimport.expand_directories([f]) assert "expanded" not in caplog.text + + +def test_unreadable_directory_does_not_warn_that_it_is_empty(tmp_path, caplog): + """A directory that could not be read is not an empty directory. Saying "no files found" + for it contradicts the Permission denied reported for the same path.""" + locked = tmp_path / "locked" + locked.mkdir() + (locked / "hidden.nc").write_text("data") + os.chmod(locked, 0o000) + + try: + with caplog.at_level(logging.WARNING, logger="rimport_relink"): + entries, skips = rimport.expand_directories([locked]) + finally: + os.chmod(locked, 0o700) + + assert entries == [] + assert len(skips) == 1 + assert "no files found" not in caplog.text + + +def test_discovered_walk_skip_is_warned_where_it_happened(tmp_path, caplog): + """The spec promises every skip is reported twice. The end-of-run summary is the second + report; this is the first, and without it a skip during a fatal abort is reported zero + times.""" + d = tmp_path / "d" + d.mkdir() + (d / "a.nc").write_text("a") + locked = d / "locked" + locked.mkdir() + os.chmod(locked, 0o000) + + try: + with caplog.at_level(logging.WARNING, logger="rimport_relink"): + rimport.expand_directories([d]) + finally: + os.chmod(locked, 0o700) + + assert f"skipping '{locked}'" in caplog.text + + +def test_named_unreadable_directory_is_not_warned_as_skipped(tmp_path, caplog): + """The counterpart. Since Task 11c a NAMED unreadable directory is fatal, so main reports + it as a failure; warning "skipping" here too would contradict "nothing was published".""" + locked = tmp_path / "locked" + locked.mkdir() + os.chmod(locked, 0o000) + + try: + with caplog.at_level(logging.WARNING, logger="rimport_relink"): + rimport.expand_directories([locked]) + finally: + os.chmod(locked, 0o700) + + assert "skipping" not in caplog.text diff --git a/tests/rimport/test_main.py b/tests/rimport/test_main.py index ea7df74..9755d4c 100644 --- a/tests/rimport/test_main.py +++ b/tests/rimport/test_main.py @@ -758,3 +758,36 @@ def test_unreadable_subdirectory_stays_a_skip( assert (staging_root / "tree" / "good.nc").read_text() == "good" captured = capsys.readouterr() assert "skipped (not stageable)" in captured.err + + @patch.object(rimport, "get_staging_root") + @patch.object(rimport, "ensure_running_as") + def test_walk_skip_is_still_reported_when_the_run_aborts( + self, _mock_ensure_running_as, mock_get_staging_root, tmp_path, capsys + ): + """A named failure returns before the end-of-run summary, so a walk skip that is not + reported inline is never reported at all -- breaking the spec's promise that every + skip is reported twice.""" + inputdata_root = tmp_path / "inputdata" + tree = inputdata_root / "tree" + tree.mkdir(parents=True) + (tree / "good.nc").write_text("good") + locked = tree / "locked" + locked.mkdir() + (locked / "hidden.nc").write_text("data") + staging_root = tmp_path / "staging" + staging_root.mkdir() + mock_get_staging_root.return_value = staging_root + missing = inputdata_root / "missing.nc" + os.chmod(locked, 0o000) + + try: + result = rimport.main( + ["-inputdata", str(inputdata_root), str(tree), str(missing)] + ) + finally: + os.chmod(locked, 0o700) + + assert result == 2 + assert not any(staging_root.rglob("*")) + captured = capsys.readouterr() + assert str(locked) in captured.out + captured.err From bf26267d0e69956becdae7fa6c577d96c7d51c23 Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Mon, 31 Aug 2026 16:21:51 -0600 Subject: [PATCH 23/47] rimport: Report walk skips, and only claim emptiness when true Every skip should be reported twice: where it happened, and again in the end-of-run summary. Walk skips got neither -- nothing reported them inline, and the summary is unreachable past the fatal return -- so an unreadable subdirectory went unmentioned in any run that aborted on a named failure. Warn about a discovered walk skip in place, matching main's wording so both halves of a skip report read the same. A skip whose path is the directory being walked is the one the user named, which main reports as fatal, so it is left alone: calling it "skipping" would contradict "nothing was published" about the same path. Also stops claiming "no files found under X" for a directory that could not be read. A genuinely empty directory produces no skips and still gets the warning. Co-Authored-By: Claude Opus 5 (1M context) --- rimport | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/rimport b/rimport index 98ae140..9bb2d64 100755 --- a/rimport +++ b/rimport @@ -285,7 +285,16 @@ def expand_directories(paths: Iterable[Path]) -> tuple[List[Entry], List[Skip]]: n_dirs += 1 found, walk_skips = walk_files(path) skips.extend(walk_skips) - if not found: + for skip in walk_skips: + # Report a DISCOVERED skip where it happened; the end-of-run summary repeats + # it. A skip whose path is the directory being walked is the one the user + # NAMED, which main reports as a fatal failure -- calling that "skipping" + # here would contradict "nothing was published". + if skip.path != path: + logger.warning( + "%srimport: skipping '%s': %s", INDENT, skip.path, skip.reason + ) + if not found and not walk_skips: logger.warning("rimport: no files found under %s", path) for found_path in found: expanded_files.add(found_path) From 3e1df18f3fef9d2a3a358d0a92830d67f6c17673 Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Mon, 31 Aug 2026 16:32:57 -0600 Subject: [PATCH 24/47] tests(rimport): red tests for the skip-warning guard's real question The guard on walk skips asks "is this skip the directory this walk was rooted at?". The question that matters is "did the user name this path anywhere in the batch?" -- which is what main asks. Name a tree and an unreadable directory inside it, and the walk of the tree warns "skipping 'X'" for a path main then reports under "nothing was published": the contradiction the guard exists to prevent, reached from the other side. The second test pins the ordering property. Emitting warnings inside the walk loop pushes the expansion count -- the blast radius -- down one line per unreadable directory. Both fail at HEAD by design. Co-Authored-By: Claude Opus 5 (1M context) --- tests/rimport/test_expand_directories.py | 40 ++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/tests/rimport/test_expand_directories.py b/tests/rimport/test_expand_directories.py index df20763..b83c975 100644 --- a/tests/rimport/test_expand_directories.py +++ b/tests/rimport/test_expand_directories.py @@ -212,3 +212,43 @@ def test_named_unreadable_directory_is_not_warned_as_skipped(tmp_path, caplog): os.chmod(locked, 0o700) assert "skipping" not in caplog.text + + +def test_named_unreadable_directory_is_not_warned_even_when_also_discovered(tmp_path, caplog): + """The guard must ask "did the user name this path?", not "is this the directory I am + walking right now?". Naming both a tree and an unreadable directory inside it made the + walk of the tree warn "skipping" for a path main then reports as a fatal failure -- the + self-contradiction the guard exists to prevent, reached by a different route.""" + d = tmp_path / "d" + d.mkdir() + (d / "a.nc").write_text("a") + locked = d / "locked" + locked.mkdir() + os.chmod(locked, 0o000) + + try: + with caplog.at_level(logging.WARNING, logger="rimport_relink"): + rimport.expand_directories([d, locked]) + finally: + os.chmod(locked, 0o700) + + assert "skipping" not in caplog.text + + +def test_expansion_count_is_logged_before_any_skip_warning(tmp_path, caplog): + """The count line is the blast radius, and it belongs at the top where it cannot be + pushed down the screen by one warning per unreadable directory.""" + d = tmp_path / "d" + d.mkdir() + (d / "a.nc").write_text("a") + locked = d / "locked" + locked.mkdir() + os.chmod(locked, 0o000) + + try: + with caplog.at_level(logging.INFO, logger="rimport_relink"): + rimport.expand_directories([d]) + finally: + os.chmod(locked, 0o700) + + assert caplog.text.index("expanded 1 director(ies)") < caplog.text.index("skipping") From 666daa9cc34e9496c0594f923dc4928c48839a1a Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Mon, 31 Aug 2026 16:34:48 -0600 Subject: [PATCH 25/47] rimport: Ask whether a skip was named, not whether it is the walk root The guard on walk skips compared each one against the directory being walked. That is the right question only when a path is discovered or named, never both. Name a tree and an unreadable directory inside it and the walk of the tree fired "skipping 'X'" for a path main then listed under "nothing was published" -- the contradiction the guard was added to prevent, arrived at from the other side. It reproduced in both argument orders, so ordering could not fix it. Ask instead the question main asks: was this path named? Hoisting the loop out of the per-path walk makes that possible, and puts the expansion count -- the blast radius -- back at the top, where one warning per unreadable directory no longer pushes it down the screen. `paths` is now read twice, so it is materialised; the signature still says Iterable and a generator would otherwise be consumed by the first pass. Suite: 328 passed. Verified through the real CLI: naming both tree and tree/locked exits 2 with no "skipping" line in either order; naming tree alone still warns inline and exits 3; naming tree/locked alone exits 2. Co-Authored-By: Claude Opus 5 (1M context) --- rimport | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/rimport b/rimport index 9bb2d64..d5c98ee 100755 --- a/rimport +++ b/rimport @@ -275,6 +275,9 @@ def expand_directories(paths: Iterable[Path]) -> tuple[List[Entry], List[Skip]]: Returns: (entries, skips). `skips` carries directories that could not be read during a walk. """ + # Consumed twice -- once to walk, once to test provenance -- so a generator will not do. + paths = list(paths) + named_paths = set(paths) named_by_path: dict[Path, bool] = {} skips: List[Skip] = [] n_dirs = 0 @@ -285,15 +288,6 @@ def expand_directories(paths: Iterable[Path]) -> tuple[List[Entry], List[Skip]]: n_dirs += 1 found, walk_skips = walk_files(path) skips.extend(walk_skips) - for skip in walk_skips: - # Report a DISCOVERED skip where it happened; the end-of-run summary repeats - # it. A skip whose path is the directory being walked is the one the user - # NAMED, which main reports as a fatal failure -- calling that "skipping" - # here would contradict "nothing was published". - if skip.path != path: - logger.warning( - "%srimport: skipping '%s': %s", INDENT, skip.path, skip.reason - ) if not found and not walk_skips: logger.warning("rimport: no files found under %s", path) for found_path in found: @@ -308,6 +302,16 @@ def expand_directories(paths: Iterable[Path]) -> tuple[List[Entry], List[Skip]]: "rimport: expanded %d director(ies) to %d file(s)", n_dirs, len(expanded_files) ) + for skip in skips: + # Report a DISCOVERED skip here, during expansion, where the run reached it; the + # end-of-run summary repeats it. A skip the user NAMED is not reported here at all: + # main treats it as a fatal failure, and calling it "skipping" would contradict the + # "nothing was published" that follows. Ask the question main asks -- was this path + # named? -- not "is this the directory being walked": a directory can be named AND + # discovered beneath another named one, and then both are true of it. + if skip.path not in named_paths: + logger.warning("%srimport: skipping '%s': %s", INDENT, skip.path, skip.reason) + entries = [Entry(path, named) for path, named in named_by_path.items()] return entries, skips From 8bf484f5927f9b0e74f58431a762aed1122ca4c8 Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Mon, 31 Aug 2026 16:35:29 -0600 Subject: [PATCH 26/47] docs(rimport): Exit 3 also means a directory could not be read main's docstring described exit 3 as discovered paths that "failed pre-flight validation". It also covers an unreadable directory beneath a named one, either cause alone being enough, which is what the code has done since walk skips were introduced. Verified: a directory whose only problem is an unreadable subdirectory exits 3. Also states the provenance rule the docstring left implicit: only discovered paths are skipped; the same failure on a named path is fatal and exits 2. Co-Authored-By: Claude Opus 5 (1M context) --- rimport | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/rimport b/rimport index d5c98ee..ce8cb51 100755 --- a/rimport +++ b/rimport @@ -726,10 +726,13 @@ def main(argv: List[str] | None = None) -> int: 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. 3: The run finished -- nothing was rejected outright and no file failed while being - staged or relinked -- but one or more discovered paths (files found by expanding - a directory argument) failed pre-flight validation and were skipped rather than - staged. Each skip is warned about inline where it happens and the full list is - repeated on stderr at the very end, so a skipped file cannot be missed. + staged or relinked -- but at least one discovered path was skipped rather than + staged. Two things cause that, and either alone is enough: a file found by + expanding a directory argument failed pre-flight validation, or a directory + beneath a named one could not be read. Each skip is warned about inline during + expansion or pre-flight and the full list is repeated on stderr at the very end, + so a skipped path cannot be missed. Note that only DISCOVERED paths are skipped; + the same failure on a path the user named is fatal, and exits 2. """ parser = build_parser() args = parser.parse_args(argv) From bc326e35e5ca6310a24a047d74cd6b4f0895f3b1 Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Mon, 31 Aug 2026 16:38:25 -0600 Subject: [PATCH 27/47] tests(rimport): red test for a path under an unreadable parent Path.is_dir() propagates EACCES rather than returning False -- it ignores only ENOENT, ENOTDIR, EBADF and ELOOP -- so naming a file under an unreadable directory escapes main as a traceback and exits 1, where --help promises exit 2. Red at HEAD by design. Co-Authored-By: Claude Haiku 4.5 --- tests/rimport/test_main.py | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/tests/rimport/test_main.py b/tests/rimport/test_main.py index 9755d4c..8a59dae 100644 --- a/tests/rimport/test_main.py +++ b/tests/rimport/test_main.py @@ -791,3 +791,30 @@ def test_walk_skip_is_still_reported_when_the_run_aborts( assert not any(staging_root.rglob("*")) captured = capsys.readouterr() assert str(locked) in captured.out + captured.err + + @patch.object(rimport, "get_staging_root") + @patch.object(rimport, "ensure_running_as") + def test_unreadable_parent_directory_is_an_error_not_a_traceback( + self, _mock_ensure_running_as, mock_get_staging_root, tmp_path + ): + """Naming a file inside a directory that cannot be read is a user error, and the help + text promises exit 2 for one. Python 3.13's Path.is_dir() raises EACCES instead of + returning False, so an unguarded probe turns that into a stack trace.""" + inputdata_root = tmp_path / "inputdata" + locked = inputdata_root / "locked" + locked.mkdir(parents=True) + (locked / "hidden.nc").write_text("data") + staging_root = tmp_path / "staging" + staging_root.mkdir() + mock_get_staging_root.return_value = staging_root + os.chmod(locked, 0o000) + + try: + result = rimport.main( + ["-inputdata", str(inputdata_root), str(locked / "hidden.nc")] + ) + finally: + os.chmod(locked, 0o700) + + assert result == 2 + assert not any(staging_root.rglob("*")) From 6fe8f09bd305086433b29d6efb646dfd48e4b2d3 Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Mon, 31 Aug 2026 16:40:53 -0600 Subject: [PATCH 28/47] rimport: Do not crash probing a path under an unreadable parent Path.is_dir() propagates EACCES rather than returning False, so naming a file under an unreadable directory escaped main as a traceback and exited 1, where --help promises exit 2. The probe now catches OSError and records it as a Skip, letting main decide how to route it. Every path reaching expand_directories was NAMED by the user, so main's provenance split sends it to the fatal pre-flight block: the user gets exit 2 and a message naming the path and the reason. Suite: 329 passed. Co-Authored-By: Claude Haiku 4.5 --- rimport | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/rimport b/rimport index ce8cb51..2ef2318 100755 --- a/rimport +++ b/rimport @@ -284,7 +284,16 @@ def expand_directories(paths: Iterable[Path]) -> tuple[List[Entry], List[Skip]]: expanded_files: set[Path] = set() for path in paths: - if path.is_dir() and not path.is_symlink(): + try: + is_real_dir = path.is_dir() and not path.is_symlink() + except OSError as exc: + # Python 3.13's is_dir() re-raises EACCES instead of returning False, so a path + # under an unreadable parent would abort the run with a traceback. Record it and + # let main decide: every path here was NAMED by the user, so main routes it to + # the fatal pre-flight block and the run exits 2 having published nothing. + skips.append(Skip(path, exc)) + continue + if is_real_dir: n_dirs += 1 found, walk_skips = walk_files(path) skips.extend(walk_skips) From 9c45f8253709a9dd4e890a41f5eaa7aa44a6046d Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Mon, 31 Aug 2026 16:57:05 -0600 Subject: [PATCH 29/47] rimport: Correct a false claim about which Python versions crash The guard on is_dir() was justified on the grounds that Path.is_dir() swallows EACCES on 3.9-3.12 and only 3.13 re-raises, making the crash invisible to CI. That is false. pathlib._IGNORED_ERRNOS is {ENOENT, ENOTDIR, EBADF, ELOOP} on 3.11.9 and 3.13.2 alike; EACCES is in neither, and the pre-fix code reproduces the traceback and exit 1 under both. The fix itself was already right and is unchanged. What changes is the reason recorded beside it: a maintainer reading "Python 3.13's is_dir()" in a repo whose CI pins 3.9-3.12 would reasonably read the guard as a local-interpreter workaround CI cannot exercise, and delete it. The truth is better news -- the bug affects every supported version, and the test is red without the fix across the whole CI matrix. Corrected in the code comment and the test docstring. Also fixes expand_directories' Returns docstring, which described skips as only walk failures; it also carries a named path whose is_dir() probe raised. Suite: 329 passed. Co-Authored-By: Claude Opus 5 (1M context) --- rimport | 7 +++++-- tests/rimport/test_main.py | 5 +++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/rimport b/rimport index 2ef2318..fba8288 100755 --- a/rimport +++ b/rimport @@ -273,7 +273,9 @@ def expand_directories(paths: Iterable[Path]) -> tuple[List[Entry], List[Skip]]: paths: Absolute paths from `normalize_paths`. Returns: - (entries, skips). `skips` carries directories that could not be read during a walk. + (entries, skips). `skips` carries directories that could not be read during a walk, + and any named path whose is_dir() probe itself raised -- a file under an unreadable + parent, say. Both are named, so `main` reports them as fatal pre-flight failures. """ # Consumed twice -- once to walk, once to test provenance -- so a generator will not do. paths = list(paths) @@ -287,7 +289,8 @@ def expand_directories(paths: Iterable[Path]) -> tuple[List[Entry], List[Skip]]: try: is_real_dir = path.is_dir() and not path.is_symlink() except OSError as exc: - # Python 3.13's is_dir() re-raises EACCES instead of returning False, so a path + # Path.is_dir() ignores only ENOENT, ENOTDIR, EBADF and ELOOP. It propagates + # everything else -- EACCES included, on every supported version -- so a path # under an unreadable parent would abort the run with a traceback. Record it and # let main decide: every path here was NAMED by the user, so main routes it to # the fatal pre-flight block and the run exits 2 having published nothing. diff --git a/tests/rimport/test_main.py b/tests/rimport/test_main.py index 8a59dae..75ede67 100644 --- a/tests/rimport/test_main.py +++ b/tests/rimport/test_main.py @@ -798,8 +798,9 @@ def test_unreadable_parent_directory_is_an_error_not_a_traceback( self, _mock_ensure_running_as, mock_get_staging_root, tmp_path ): """Naming a file inside a directory that cannot be read is a user error, and the help - text promises exit 2 for one. Python 3.13's Path.is_dir() raises EACCES instead of - returning False, so an unguarded probe turns that into a stack trace.""" + text promises exit 2 for one. Path.is_dir() propagates EACCES rather than returning + False -- it ignores only ENOENT, ENOTDIR, EBADF and ELOOP -- so an unguarded probe + turns that into a stack trace on every supported version.""" inputdata_root = tmp_path / "inputdata" locked = inputdata_root / "locked" locked.mkdir(parents=True) From 455ffa0763367fdade8a27d53453de38aadb495a Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Mon, 31 Aug 2026 16:59:43 -0600 Subject: [PATCH 30/47] docs: Describe rimport directory enumeration in the README MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Directory arguments are enumerated recursively, with the directory itself never staged or symlinked. A symlink to a directory is not expanded. Discovered failures are skipped and summarised while a name you gave directly is still fatal. This commit also removes "a directory" from the pre-flight failure list—which this branch made false—and replaces the prose exit code documentation with a structured table. Co-Authored-By: Claude Haiku 4.5 --- README.md | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 742922f..8abb522 100644 --- a/README.md +++ b/README.md @@ -12,9 +12,25 @@ Notes: - 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 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). +- 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 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. + +### Directory arguments + +Any name you give `rimport` -- positional, `--file`, or a `--list` entry -- may be a directory. Every file beneath it is enumerated recursively and acted on. The directory itself is never copied to staging or replaced with a symlink. A symlink to a directory is the one carve-out: it is not expanded, and is treated as a single entry. + +A file found by enumeration that cannot be staged does not abort the run. It is reported, skipped, and repeated in a summary at the end so it does not scroll away. A bad name you gave directly is still fatal, and nothing is published. + +Exit codes: + +| Code | Meaning | +|------|---------| +| 0 | Everything staged or checked, nothing skipped | +| 1 | A file could not be staged | +| 2 | A name you gave failed validation; nothing was published | +| 3 | Finished, but one or more files were skipped (listed at the end) | + +When more than one code applies, the precedence is 2 > 1 > 3 > 0. ## Filenames and metadata: From 0ce3e6faedb4b988bd982697260973f685c30ede Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Mon, 31 Aug 2026 17:00:32 -0600 Subject: [PATCH 31/47] docs: Match the README's em-dash voice The directory-arguments section used -- where the rest of the README uses an em dash. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 8abb522..fbfa4b2 100644 --- a/README.md +++ b/README.md @@ -17,7 +17,7 @@ Notes: ### Directory arguments -Any name you give `rimport` -- positional, `--file`, or a `--list` entry -- may be a directory. Every file beneath it is enumerated recursively and acted on. The directory itself is never copied to staging or replaced with a symlink. A symlink to a directory is the one carve-out: it is not expanded, and is treated as a single entry. +Any name you give `rimport` — positional, `--file`, or a `--list` entry — may be a directory. Every file beneath it is enumerated recursively and acted on. The directory itself is never copied to staging or replaced with a symlink. A symlink to a directory is the one carve-out: it is not expanded, and is treated as a single entry. A file found by enumeration that cannot be staged does not abort the run. It is reported, skipped, and repeated in a summary at the end so it does not scroll away. A bad name you gave directly is still fatal, and nothing is published. From f5e37a0b50ed433e2ffbaeb071bee22889d04c24 Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Mon, 31 Aug 2026 17:15:52 -0600 Subject: [PATCH 32/47] docs: Scope the all-or-nothing promise to named paths expand_directories' Returns docstring said "Both are named, so main reports them as fatal pre-flight failures". That is false for the dominant case: a directory that could not be read DURING A WALK is discovered unless the user also typed it, and stays a warn-and-skip with exit 3, which is what test_unreadable_subdirectory_stays_a_skip pins. Verified: --check on a tree with an unreadable subdirectory exits 3. The README also still promised all-or-nothing over every file processed. That is false for discovered files. The equivalent --check blurb in --help had already been rescoped to named paths; its README counterpart was missed. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 4 ++-- rimport | 8 +++++--- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index fbfa4b2..5d815b3 100644 --- a/README.md +++ b/README.md @@ -12,8 +12,8 @@ Notes: - 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 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 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. +- Before staging anything, `rimport` validates every path you named (all `--file`/`--list`/positional entries together). If any of them fail — missing, 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. A file found by expanding a directory you named is not covered by this promise; it is skipped on its own, and the rest of the run continues (see "Directory arguments" below). 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 path you named 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. ### Directory arguments diff --git a/rimport b/rimport index fba8288..68c6693 100755 --- a/rimport +++ b/rimport @@ -273,9 +273,11 @@ def expand_directories(paths: Iterable[Path]) -> tuple[List[Entry], List[Skip]]: paths: Absolute paths from `normalize_paths`. Returns: - (entries, skips). `skips` carries directories that could not be read during a walk, - and any named path whose is_dir() probe itself raised -- a file under an unreadable - parent, say. Both are named, so `main` reports them as fatal pre-flight failures. + (entries, skips). `skips` carries two things: directories that could not be read + during a walk, and named paths whose is_dir() probe itself raised (a file under an + unreadable parent, say). Provenance is NOT recorded here -- `main` recovers it by + testing each skip's path against the paths the user named, and a walk skip is + usually discovered, so it warns and skips with exit 3 rather than aborting. """ # Consumed twice -- once to walk, once to test provenance -- so a generator will not do. paths = list(paths) From 5f1cb23b8676bdd0fbcb3f47b7e09c7c85d8c5e8 Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Mon, 31 Aug 2026 17:16:55 -0600 Subject: [PATCH 33/47] tests(rimport): red tests for skips that duplicate themselves expand_directories de-duplicates the files it discovers but not the skips, and it re-walks a directory named twice. So naming the same directory twice, or a directory alongside its own parent, records the same unreadable subdirectory twice: warned twice inline, listed twice in the end-of-run summary, counted twice in "N file(s) skipped", and -- when the user named the path -- counted twice in the fatal block's numerator and denominator. n_dirs has the same shape of bug: it counts walks, so a directory named twice reports "expanded 2 director(ies)". Four tests: the walk count, the duplicated skip by both routes, and the inflated fatal count end to end. Red at HEAD by design. Co-Authored-By: Claude Opus 5 (1M context) --- tests/rimport/test_expand_directories.py | 52 ++++++++++++++++++++++++ tests/rimport/test_main.py | 32 +++++++++++++++ 2 files changed, 84 insertions(+) diff --git a/tests/rimport/test_expand_directories.py b/tests/rimport/test_expand_directories.py index b83c975..4cc7c07 100644 --- a/tests/rimport/test_expand_directories.py +++ b/tests/rimport/test_expand_directories.py @@ -252,3 +252,55 @@ def test_expansion_count_is_logged_before_any_skip_warning(tmp_path, caplog): os.chmod(locked, 0o700) assert caplog.text.index("expanded 1 director(ies)") < caplog.text.index("skipping") + + +def test_directory_named_twice_is_walked_once(tmp_path, caplog): + """Naming the same directory twice is one directory, not two. Counting the walks instead + of the directories made the blast-radius line overstate itself.""" + d = tmp_path / "d" + d.mkdir() + (d / "a.nc").write_text("a") + + with caplog.at_level(logging.INFO, logger="rimport_relink"): + rimport.expand_directories([d, d]) + + assert "expanded 1 director(ies) to 1 file(s)" in caplog.text + + +def test_duplicate_arguments_do_not_duplicate_a_skip(tmp_path, caplog): + """Files de-duplicate; skips did not. Naming a directory twice walked it twice and + recorded the same unreadable subdirectory twice, so it was warned twice, listed twice in + the end-of-run summary, and counted twice.""" + d = tmp_path / "d" + d.mkdir() + locked = d / "locked" + locked.mkdir() + os.chmod(locked, 0o000) + + try: + with caplog.at_level(logging.WARNING, logger="rimport_relink"): + _entries, skips = rimport.expand_directories([d, d]) + finally: + os.chmod(locked, 0o700) + + assert len(skips) == 1 + assert caplog.text.count("skipping") == 1 + + +def test_overlapping_named_directories_do_not_duplicate_a_skip(tmp_path): + """Same defect by another route: a subdirectory named alongside its parent is walked + twice, so anything unreadable beneath it is recorded twice.""" + d = tmp_path / "d" + d.mkdir() + sub = d / "sub" + sub.mkdir() + locked = sub / "locked" + locked.mkdir() + os.chmod(locked, 0o000) + + try: + _entries, skips = rimport.expand_directories([d, sub]) + finally: + os.chmod(locked, 0o700) + + assert len(skips) == 1 diff --git a/tests/rimport/test_main.py b/tests/rimport/test_main.py index 75ede67..5403c71 100644 --- a/tests/rimport/test_main.py +++ b/tests/rimport/test_main.py @@ -729,6 +729,38 @@ def test_named_unreadable_directory_is_fatal_not_a_skip( assert not any(staging_root.rglob("*")) assert not (other / "good.nc").is_symlink() + @patch.object(rimport, "get_staging_root") + @patch.object(rimport, "ensure_running_as") + def test_naming_an_unreadable_directory_twice_reports_it_once( + self, _mock_ensure_running_as, mock_get_staging_root, tmp_path, capsys + ): + """The same path given twice is one bad path, not two. Walking it twice recorded two + identical Skips, which inflated both the failure list and the denominator: the user + typed two arguments and got "2 of 3 file(s) failed", with one path printed twice.""" + inputdata_root = tmp_path / "inputdata" + locked = inputdata_root / "locked" + locked.mkdir(parents=True) + other = inputdata_root / "ok" + other.mkdir() + (other / "good.nc").write_text("good") + staging_root = tmp_path / "staging" + staging_root.mkdir() + mock_get_staging_root.return_value = staging_root + os.chmod(locked, 0o000) + + try: + result = rimport.main( + ["-inputdata", str(inputdata_root), str(locked), str(locked), str(other)] + ) + finally: + os.chmod(locked, 0o700) + + assert result == 2 + captured = capsys.readouterr() + # Two distinct paths were considered: `locked` and the good.nc discovered under `ok`. + assert "1 of 2 file(s) failed pre-flight validation" in captured.err + assert captured.err.count(str(locked)) == 1 + @patch.object(rimport, "get_staging_root") @patch.object(rimport, "ensure_running_as") def test_unreadable_subdirectory_stays_a_skip( From 7b64004506a05b0c773810341ca3d18d430382b8 Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Mon, 31 Aug 2026 17:19:02 -0600 Subject: [PATCH 34/47] rimport: Give Skip its provenance, and collapse duplicate skips Skip gains a `named` field, defaulting False so walk_files can still build one with two arguments. expand_directories is the only place that knows the whole batch, so it is the only place that fills it in: it collapses skips by path and stamps each with whole-batch membership. main then reads the field instead of rebuilding `set(paths)` for itself. That second copy was the hazard. The inline-warning guard was correct only because expand_directories' set and main's set were guaranteed identical, an invariant nothing enforced and no test would have caught breaking. Deriving it once removes the possibility. Duplicate arguments now collapse before the walk, so a directory named twice is walked once rather than reporting "expanded 2 director(ies)" and finding everything beneath it twice. Verified through the real CLI: ` ` and ` ` each warn once, list once, and say "1 file(s) skipped" instead of two of each; naming an unreadable directory twice reports "1 of 2 file(s) failed" listing it once, where it said "2 of 3" and printed the path twice. Suite: 333 passed. Co-Authored-By: Claude Opus 5 (1M context) --- rimport | 51 +++++++++++++++++++++++++------------- tests/rimport/test_main.py | 4 ++- 2 files changed, 37 insertions(+), 18 deletions(-) diff --git a/rimport b/rimport index 68c6693..2023d47 100755 --- a/rimport +++ b/rimport @@ -203,14 +203,20 @@ class Entry(NamedTuple): class Skip(NamedTuple): - """One path that will not be staged, and the reason why. + """One path that will not be staged, the reason why, and whether the user named it. Both validation failures on discovered files and directories that could not be read become Skips, so `report_skips` can present them in one block. + + `named` defaults to False so a walker that has no idea what the user typed can build a + Skip with two arguments. `expand_directories` is the one place that knows the whole + batch, so it is the one place that fills this in; `main` then trusts it rather than + re-deriving it, which previously meant two copies of the same set having to agree. """ path: Path reason: Exception + named: bool = False def walk_files(root: Path) -> tuple[List[Path], List[Skip]]: @@ -273,14 +279,17 @@ def expand_directories(paths: Iterable[Path]) -> tuple[List[Entry], List[Skip]]: paths: Absolute paths from `normalize_paths`. Returns: - (entries, skips). `skips` carries two things: directories that could not be read - during a walk, and named paths whose is_dir() probe itself raised (a file under an - unreadable parent, say). Provenance is NOT recorded here -- `main` recovers it by - testing each skip's path against the paths the user named, and a walk skip is - usually discovered, so it warns and skips with exit 3 rather than aborting. + (entries, skips), each skip carrying its own provenance. `skips` holds two things: + directories that could not be read during a walk, and named paths whose is_dir() + probe itself raised (a file under an unreadable parent, say). A walk skip is usually + discovered, and a discovered skip warns and continues with exit 3 rather than + aborting; only a skip the user named is fatal. Skips are collapsed by path, so a + directory named twice yields one. """ # Consumed twice -- once to walk, once to test provenance -- so a generator will not do. - paths = list(paths) + # Collapsing here means a directory named twice is walked once, so it counts once toward + # the expansion total and reports anything unreadable beneath it once. + paths = list(dict.fromkeys(paths)) named_paths = set(paths) named_by_path: dict[Path, bool] = {} skips: List[Skip] = [] @@ -311,6 +320,14 @@ def expand_directories(paths: Iterable[Path]) -> tuple[List[Entry], List[Skip]]: # A named path always wins over the same path discovered by a walk. named_by_path[path] = True + # Collapse by path and stamp provenance before anything counts or prints these. Two + # named directories can overlap -- `` and `/sub` -- and then the same + # unreadable grandchild is found by both walks. + deduped: dict[Path, Skip] = {} + for skip in skips: + deduped.setdefault(skip.path, Skip(skip.path, skip.reason, skip.path in named_paths)) + skips = list(deduped.values()) + if n_dirs: logger.info( "rimport: expanded %d director(ies) to %d file(s)", n_dirs, len(expanded_files) @@ -320,10 +337,10 @@ def expand_directories(paths: Iterable[Path]) -> tuple[List[Entry], List[Skip]]: # Report a DISCOVERED skip here, during expansion, where the run reached it; the # end-of-run summary repeats it. A skip the user NAMED is not reported here at all: # main treats it as a fatal failure, and calling it "skipping" would contradict the - # "nothing was published" that follows. Ask the question main asks -- was this path - # named? -- not "is this the directory being walked": a directory can be named AND - # discovered beneath another named one, and then both are true of it. - if skip.path not in named_paths: + # "nothing was published" that follows. Note this asks about the whole batch, not + # about the directory being walked: a directory can be named AND discovered beneath + # another named one, and then both are true of it. + if not skip.named: logger.warning("%srimport: skipping '%s': %s", INDENT, skip.path, skip.reason) entries = [Entry(path, named) for path, named in named_by_path.items()] @@ -786,14 +803,14 @@ def main(argv: List[str] | None = None) -> int: # bad path is only a skip: one unstageable file inside a large tree must not block the # thousands of good ones around it. # A directory the user NAMED that could not be read is a named failure, not a skip. - # Skip carries no provenance, so recognise it here by matching against the paths the - # user actually gave. An unreadable directory found BENEATH a named one stays a skip: - # the user did not name it, so it is discovered, and discovered failures warn and skip. - named_paths = set(paths) + # `expand_directories` stamped that on each Skip; deriving it a second time here would + # mean two sets that have to agree, and nothing enforcing it. An unreadable directory + # found BENEATH a named one stays a skip: the user did not name it, so it is discovered, + # and discovered failures warn and skip. named_failures: List[tuple[Path, Exception]] = [ - (skip.path, skip.reason) for skip in skips if skip.path in named_paths + (skip.path, skip.reason) for skip in skips if skip.named ] - skips = [skip for skip in skips if skip.path not in named_paths] + skips = [skip for skip in skips if not skip.named] # An unreadable named directory never became an Entry, so it has to be added to the # count of paths considered; otherwise a lone one reports "1 of 0". n_considered = len(entries) + len(named_failures) diff --git a/tests/rimport/test_main.py b/tests/rimport/test_main.py index 5403c71..acc4027 100644 --- a/tests/rimport/test_main.py +++ b/tests/rimport/test_main.py @@ -759,7 +759,9 @@ def test_naming_an_unreadable_directory_twice_reports_it_once( captured = capsys.readouterr() # Two distinct paths were considered: `locked` and the good.nc discovered under `ok`. assert "1 of 2 file(s) failed pre-flight validation" in captured.err - assert captured.err.count(str(locked)) == 1 + # Count list ENTRIES, not path occurrences: the OSError repr repeats the path within + # a single line, so a raw substring count sees two even when one entry is printed. + assert captured.err.count(f"rimport: '{locked}'") == 1 @patch.object(rimport, "get_staging_root") @patch.object(rimport, "ensure_running_as") From 3bd5a06c1e6a649395ab320c9f16dabd14ce640c Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Mon, 31 Aug 2026 17:20:15 -0600 Subject: [PATCH 35/47] rimport: Name exit 2's real triggers in --help, and state the precedence The epilog described exit 2 as "a name you gave failed validation", which is one of several triggers -- a bad command line, a missing or empty --list file and an undeterminable working directory all produce it too, as main's own docstring has always said. And nothing in --help stated that the codes are not mutually exclusive or which wins, though the README documents 2 > 1 > 3 > 0. The epilog is where a script author looks, and exit 3 is new and scriptable. Adds the named-vs-discovered rule in one line, since that is the mental model the exit codes turn on. Also rewraps the three help strings that ran past 100 characters, which is why they are in this commit rather than left to a formatting pass: they are the same strings this change is about. Rendered output is unchanged -- argparse rewraps them anyway -- and no line of --help exceeds 78 columns at COLUMNS=80, verified on 3.13.2 and on 3.11.9 inside the CI range. test_help_documents_all_four_exit_codes still passes: it asserts the section exists and lists 0: through 3:, not the wording. Suite: 333 passed. Co-Authored-By: Claude Opus 5 (1M context) --- rimport | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/rimport b/rimport index 2023d47..1659f70 100755 --- a/rimport +++ b/rimport @@ -62,8 +62,13 @@ def build_parser() -> argparse.ArgumentParser: "exit codes:\n" " 0: everything staged or checked, nothing skipped\n" " 1: a file could not be staged\n" - " 2: a name you gave failed validation; nothing was published\n" + " 2: nothing was published -- a bad command line, a missing or empty\n" + " --list file, or a name you gave failing validation\n" " 3: finished, but one or more files were skipped (listed at the end)\n" + "\n" + "a name you gave failing is fatal (2). a file found by expanding a\n" + "directory you named is skipped instead, and the run continues (3).\n" + "when several apply, the precedence is 2 > 1 > 3 > 0.\n" ), formatter_class=_EpilogRawFormatter, add_help=False, # Disable automatic help to add custom -help flag @@ -77,8 +82,8 @@ def build_parser() -> argparse.ArgumentParser: help=( "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. If the name is a directory, every file beneath it is enumerated recursively and" - " acted on; a symlink to a directory is not expanded." + " inputdata root. If the name is a directory, every file beneath it is" + " enumerated recursively and acted on; a symlink to a directory is not expanded." ), ) @@ -90,8 +95,9 @@ def build_parser() -> argparse.ArgumentParser: help=( "Provide a file that contains a list of filenames to import. All filenames in the list" " must be in the CESM inputdata directory. A relative entry is resolved against the" - " list file's own directory, wherever that directory is. A list entry naming a directory is enumerated recursively, the same as a name given" - " on the command line." + " list file's own directory, wherever that directory is. A list entry naming a" + " directory is enumerated recursively, the same as a name given on the command" + " line." ), ) @@ -101,8 +107,8 @@ def build_parser() -> argparse.ArgumentParser: help=( "One or more files to process. (Optional; can use --file instead to process just one.)" " A relative name is resolved against the current directory; there is no fallback to" - " the inputdata root. If the name is a directory, every file beneath it is enumerated recursively and" - " acted on; a symlink to a directory is not expanded." + " the inputdata root. If the name is a directory, every file beneath it is" + " enumerated recursively and acted on; a symlink to a directory is not expanded." ), ) From 5ea1a5e975c580c440b2c44d88ead4b22f149732 Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Mon, 31 Aug 2026 17:21:17 -0600 Subject: [PATCH 36/47] tests(rimport): Pin directory expansion on --file and --list --file's and --list's help text both promise that a name may be a directory, but only the positional channel was tested. All three funnel through get_files_to_process -> normalize_paths -> expand_directories, so the other two work by construction -- but nothing stopped a future change to read_filelist or to anchoring from silently breaking the --list half. These pass at HEAD rather than starting red, because the behaviour is already correct and the gap is coverage. So they were mutation-tested instead: with expansion disabled in a scratch copy, both fail. The --list case uses a relative entry, which also pins that a directory entry anchors to the list file's own directory exactly as a file entry does. Both assert the directory itself is still a real directory afterwards, the invariant 75c79cd was added to protect. Suite: 335 passed. Co-Authored-By: Claude Opus 5 (1M context) --- tests/rimport/test_cmdline.py | 70 +++++++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/tests/rimport/test_cmdline.py b/tests/rimport/test_cmdline.py index ff26c96..e33bed1 100644 --- a/tests/rimport/test_cmdline.py +++ b/tests/rimport/test_cmdline.py @@ -1218,6 +1218,76 @@ def test_directory_argument_recurses_into_subdirectories( assert (staging_root / "lnd" / "top.nc").read_text() == "top" assert (staging_root / "lnd" / "clm2" / "paramdata" / "deep.nc").read_text() == "deep" + def test_file_option_expands_a_directory(self, rimport_script, test_env, rimport_env): + """--help promises expansion on all three input channels. The positional channel is + covered above; this is --file, which nothing else pins.""" + inputdata_root = test_env["inputdata_root"] + staging_root = test_env["staging_root"] + + d = inputdata_root / "lnd" / "clm2" + d.mkdir(parents=True) + (d / "a.nc").write_text("a") + (d / "sub").mkdir() + (d / "sub" / "b.nc").write_text("b") + + result = subprocess.run( + [ + sys.executable, + rimport_script, + "--file", + str(d), + "-inputdata", + str(inputdata_root), + ], + capture_output=True, + text=True, + check=False, + env=rimport_env, + ) + + assert result.returncode == 0, f"Command unexpectedly failed: {result.stderr}" + assert (staging_root / "lnd" / "clm2" / "a.nc").read_text() == "a" + assert (staging_root / "lnd" / "clm2" / "sub" / "b.nc").read_text() == "b" + # The directory itself must never be staged or replaced -- the corruption 75c79cd + # was added to prevent. + assert d.is_dir() and not d.is_symlink() + + def test_list_entry_expands_a_directory(self, rimport_script, test_env, rimport_env): + """The third channel. A --list entry naming a directory is enumerated the same way, + including when the entry is relative and anchors to the list file's own directory.""" + inputdata_root = test_env["inputdata_root"] + staging_root = test_env["staging_root"] + + d = inputdata_root / "lnd" / "clm2" + d.mkdir(parents=True) + (d / "a.nc").write_text("a") + (d / "sub").mkdir() + (d / "sub" / "b.nc").write_text("b") + + # Relative, so this also pins that directory entries anchor like file entries do. + list_file = inputdata_root / "lnd" / "todo.txt" + list_file.write_text("clm2\n") + + result = subprocess.run( + [ + sys.executable, + rimport_script, + "--list", + str(list_file), + "-inputdata", + str(inputdata_root), + ], + capture_output=True, + text=True, + check=False, + env=rimport_env, + ) + + assert result.returncode == 0, f"Command unexpectedly failed: {result.stderr}" + assert (staging_root / "lnd" / "clm2" / "a.nc").read_text() == "a" + assert (staging_root / "lnd" / "clm2" / "sub" / "b.nc").read_text() == "b" + assert d.is_dir() and not d.is_symlink() + def test_skip_summary_repeats_skipped_files_on_stderr_at_the_end( self, rimport_script, test_env, rimport_env ): From 4d41034c8918a12e2f399a22171fc1e4600be90e Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Wed, 9 Sep 2026 11:31:24 -0600 Subject: [PATCH 37/47] tests(rimport): red test for a named directory outside the root A named file outside the inputdata root exits 2. A named directory outside it is expanded first, so its contents become discovered failures and the run exits 3 -- a softer verdict for the same user error, and one that contradicts the README's "a bad name you gave directly is still fatal". It is also walked before being rejected, so a mistyped `rimport ~` recurses an arbitrary tree and reports every file in it. Red at HEAD by design. Co-Authored-By: Claude Opus 5 (1M context) --- tests/rimport/test_main.py | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/tests/rimport/test_main.py b/tests/rimport/test_main.py index acc4027..34ded9c 100644 --- a/tests/rimport/test_main.py +++ b/tests/rimport/test_main.py @@ -729,6 +729,40 @@ def test_named_unreadable_directory_is_fatal_not_a_skip( assert not any(staging_root.rglob("*")) assert not (other / "good.nc").is_symlink() + @patch.object(rimport, "get_staging_root") + @patch.object(rimport, "ensure_running_as") + def test_named_directory_outside_the_inputdata_root_is_rejected_not_walked( + self, _mock_ensure_running_as, mock_get_staging_root, tmp_path, capsys + ): + """A named FILE outside the inputdata root has always been fatal. A directory must not + get a softer verdict just because it can now be expanded: walking it turns the user's + own bad argument into a pile of discovered skips and a "finished" exit 3. + + It must also not be walked at all. Expansion recurses, so a mistyped `rimport ~` + would otherwise stat an arbitrarily large tree before rejecting every file in it. + """ + inputdata_root = tmp_path / "inputdata" + inputdata_root.mkdir() + outside = tmp_path / "elsewhere" + (outside / "deep").mkdir(parents=True) + (outside / "a.nc").write_text("a") + (outside / "deep" / "b.nc").write_text("b") + staging_root = tmp_path / "staging" + staging_root.mkdir() + mock_get_staging_root.return_value = staging_root + + result = rimport.main(["-inputdata", str(inputdata_root), str(outside)]) + + assert result == 2 + captured = capsys.readouterr() + assert "nothing was published" in captured.err + # The directory itself is the failure, named once. Not its contents. + assert "1 of 1 file(s) failed pre-flight validation" in captured.err + assert str(outside / "a.nc") not in captured.err + # Nothing beneath it may have been enumerated. + assert "expanded" not in captured.out + assert not any(staging_root.rglob("*")) + @patch.object(rimport, "get_staging_root") @patch.object(rimport, "ensure_running_as") def test_naming_an_unreadable_directory_twice_reports_it_once( From 402dbf3362548d82a9f637ed82855674b2e7d5bc Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Wed, 9 Sep 2026 11:33:58 -0600 Subject: [PATCH 38/47] rimport: Scope directory expansion to the inputdata tree A named directory outside the inputdata root was expanded and walked before anything rejected it, so the user's own bad argument became a heap of discovered skips and the run "finished" with exit 3. A named file outside the root has always exited 2. Same error, two verdicts, and the softer one contradicts the README. expand_directories now takes the root and declines to expand anything outside it, leaving the path as a named entry for the pre-flight gate that already knows how to judge it. So `rimport ~` no longer recurses an arbitrary tree as cesmdata before rejecting every file in it. validate_source_path checks containment before the is-a-directory backstop, so an out-of-root directory now names the reason the user can act on rather than "source is a directory, not a file", which invites the reply that directories are supported now. The backstop itself is unchanged for a directory inside the tree, and the messages for a missing file, a file outside the root, and a file already under staging are all byte-identical. Verified through the real CLI across those five shapes. The two scoping tests were mutation-tested: deferring the check until after the walk fails both. Suite: 339 passed. Co-Authored-By: Claude Opus 5 (1M context) --- rimport | 36 ++++++++--- tests/rimport/test_expand_directories.py | 78 ++++++++++++++++++------ 2 files changed, 87 insertions(+), 27 deletions(-) diff --git a/rimport b/rimport index 1659f70..9b06299 100755 --- a/rimport +++ b/rimport @@ -269,13 +269,22 @@ def walk_files(root: Path) -> tuple[List[Path], List[Skip]]: return files, skips -def expand_directories(paths: Iterable[Path]) -> tuple[List[Entry], List[Skip]]: +def expand_directories( + paths: Iterable[Path], inputdata_root: Path +) -> tuple[List[Entry], List[Skip]]: """Replace each directory in `paths` with the files beneath it, tagging provenance. - A path is expanded only if it is a real directory. A symlink to a directory is left - alone as a single named entry, matching `walk_files`' refusal to descend through one. - Nothing here validates: a nonexistent path passes straight through as named, so - `main`'s pre-flight gate can reject it with its usual message. + A path is expanded only if it is a real directory INSIDE `inputdata_root`. A symlink to + a directory is left alone as a single named entry, matching `walk_files`' refusal to + descend through one. Nothing here validates: a nonexistent path, or a directory outside + the tree, passes straight through as named, so `main`'s pre-flight gate can reject it + with its usual message. Declining to expand is not a verdict -- it just leaves the path + for the gate that already knows how to judge it. + + Scoping expansion to the tree matters for more than tidiness. Walking recurses, so + expanding a directory this tool has no business in -- a mistyped `rimport ~` -- would + stat an arbitrarily large tree before rejecting every file in it, and would downgrade + the user's own bad argument from a fatal named failure to a heap of discovered skips. Duplicates collapse in first-seen order. If the same path is both named and discovered -- the user typed a file that also lives inside a directory they named -- `named` wins, @@ -283,6 +292,7 @@ def expand_directories(paths: Iterable[Path]) -> tuple[List[Entry], List[Skip]]: Args: paths: Absolute paths from `normalize_paths`. + inputdata_root: Root of the inputdata tree; only directories under it are expanded. Returns: (entries, skips), each skip carrying its own provenance. `skips` holds two things: @@ -313,6 +323,11 @@ def expand_directories(paths: Iterable[Path]) -> tuple[List[Entry], List[Skip]]: # the fatal pre-flight block and the run exits 2 having published nothing. skips.append(Skip(path, exc)) continue + if is_real_dir and not path.resolve().is_relative_to(inputdata_root.resolve()): + # Outside the tree: hand it to the pre-flight gate unexpanded. Resolve both + # sides, as validate_source_path does, so the two agree about what "outside" + # means for a path reached through a symlinked parent. + is_real_dir = False if is_real_dir: n_dirs += 1 found, walk_skips = walk_files(path) @@ -430,9 +445,9 @@ def validate_source_path( 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}") - + # Containment is checked before the is-a-directory backstop so that a DIRECTORY outside + # the tree names the reason the user can act on. Told "source is a directory, not a + # file", they would reasonably reply that directories are supported now. try: src.resolve().relative_to(inputdata_root.resolve()) except ValueError: @@ -444,6 +459,9 @@ def validate_source_path( f"source not under inputdata root: {src} not in {inputdata_root}" ) + if src.is_dir(): + return RuntimeError(f"source is a directory, not a file: {src}") + return None @@ -802,7 +820,7 @@ def main(argv: List[str] | None = None) -> int: # Expand any directory argument into the files beneath it, tagging each path as named # (the user asked for it) or discovered (a walk found it). - entries, skips = expand_directories(paths) + entries, skips = expand_directories(paths, root) # Pre-flight: validate everything before staging anything. A NAMED bad path is fatal and # aborts the whole batch, so a batch containing one is never half-completed. A DISCOVERED diff --git a/tests/rimport/test_expand_directories.py b/tests/rimport/test_expand_directories.py index 4cc7c07..e07ad8d 100644 --- a/tests/rimport/test_expand_directories.py +++ b/tests/rimport/test_expand_directories.py @@ -29,7 +29,7 @@ def test_directory_expands_to_discovered_files(tmp_path): (d / "a.nc").write_text("a") (d / "b.nc").write_text("b") - entries, skips = rimport.expand_directories([d]) + entries, skips = rimport.expand_directories([d], tmp_path) assert skips == [] assert entries == [ @@ -43,7 +43,7 @@ def test_plain_file_passes_through_as_named(tmp_path): f = tmp_path / "f.nc" f.write_text("data") - entries, _skips = rimport.expand_directories([f]) + entries, _skips = rimport.expand_directories([f], tmp_path) assert entries == [rimport.Entry(f, True)] @@ -52,7 +52,7 @@ def test_nonexistent_path_passes_through_as_named(tmp_path): """Expansion does not validate. A missing path stays named so pre-flight can reject it.""" missing = tmp_path / "missing.nc" - entries, _skips = rimport.expand_directories([missing]) + entries, _skips = rimport.expand_directories([missing], tmp_path) assert entries == [rimport.Entry(missing, True)] @@ -68,7 +68,7 @@ def test_symlink_to_directory_is_not_expanded(tmp_path): link = tmp_path / "dirlink" link.symlink_to(real_dir) - entries, _skips = rimport.expand_directories([link]) + entries, _skips = rimport.expand_directories([link], tmp_path) assert entries == [rimport.Entry(link, True)] @@ -84,7 +84,7 @@ def test_duplicates_collapse_and_named_wins(tmp_path): inner = d / "inner.nc" inner.write_text("data") - entries, _skips = rimport.expand_directories([d, inner]) + entries, _skips = rimport.expand_directories([d, inner], tmp_path) assert entries == [rimport.Entry(inner, True)] @@ -96,7 +96,7 @@ def test_duplicate_order_is_first_seen(tmp_path): (d / "a.nc").write_text("a") (d / "b.nc").write_text("b") - entries, _skips = rimport.expand_directories([d, d]) + entries, _skips = rimport.expand_directories([d, d], tmp_path) assert [e.path.name for e in entries] == ["a.nc", "b.nc"] @@ -110,7 +110,7 @@ def test_walk_skips_are_passed_through(tmp_path): os.chmod(locked, 0o000) try: - _entries, skips = rimport.expand_directories([d]) + _entries, skips = rimport.expand_directories([d], tmp_path) finally: os.chmod(locked, 0o700) @@ -125,7 +125,7 @@ def test_empty_directory_warns_but_is_not_a_skip(tmp_path, caplog): d.mkdir() with caplog.at_level(logging.WARNING, logger="rimport_relink"): - entries, skips = rimport.expand_directories([d]) + entries, skips = rimport.expand_directories([d], tmp_path) assert entries == [] assert skips == [] @@ -143,7 +143,7 @@ def test_logs_expansion_counts(tmp_path, caplog): (d2 / "c.nc").write_text("c") with caplog.at_level(logging.INFO, logger="rimport_relink"): - rimport.expand_directories([d1, d2]) + rimport.expand_directories([d1, d2], tmp_path) assert "expanded 2 director(ies) to 3 file(s)" in caplog.text @@ -154,7 +154,7 @@ def test_no_expansion_logs_no_count_line(tmp_path, caplog): f.write_text("data") with caplog.at_level(logging.INFO, logger="rimport_relink"): - rimport.expand_directories([f]) + rimport.expand_directories([f], tmp_path) assert "expanded" not in caplog.text @@ -169,7 +169,7 @@ def test_unreadable_directory_does_not_warn_that_it_is_empty(tmp_path, caplog): try: with caplog.at_level(logging.WARNING, logger="rimport_relink"): - entries, skips = rimport.expand_directories([locked]) + entries, skips = rimport.expand_directories([locked], tmp_path) finally: os.chmod(locked, 0o700) @@ -191,7 +191,7 @@ def test_discovered_walk_skip_is_warned_where_it_happened(tmp_path, caplog): try: with caplog.at_level(logging.WARNING, logger="rimport_relink"): - rimport.expand_directories([d]) + rimport.expand_directories([d], tmp_path) finally: os.chmod(locked, 0o700) @@ -207,7 +207,7 @@ def test_named_unreadable_directory_is_not_warned_as_skipped(tmp_path, caplog): try: with caplog.at_level(logging.WARNING, logger="rimport_relink"): - rimport.expand_directories([locked]) + rimport.expand_directories([locked], tmp_path) finally: os.chmod(locked, 0o700) @@ -228,7 +228,7 @@ def test_named_unreadable_directory_is_not_warned_even_when_also_discovered(tmp_ try: with caplog.at_level(logging.WARNING, logger="rimport_relink"): - rimport.expand_directories([d, locked]) + rimport.expand_directories([d, locked], tmp_path) finally: os.chmod(locked, 0o700) @@ -247,7 +247,7 @@ def test_expansion_count_is_logged_before_any_skip_warning(tmp_path, caplog): try: with caplog.at_level(logging.INFO, logger="rimport_relink"): - rimport.expand_directories([d]) + rimport.expand_directories([d], tmp_path) finally: os.chmod(locked, 0o700) @@ -262,7 +262,7 @@ def test_directory_named_twice_is_walked_once(tmp_path, caplog): (d / "a.nc").write_text("a") with caplog.at_level(logging.INFO, logger="rimport_relink"): - rimport.expand_directories([d, d]) + rimport.expand_directories([d, d], tmp_path) assert "expanded 1 director(ies) to 1 file(s)" in caplog.text @@ -279,7 +279,7 @@ def test_duplicate_arguments_do_not_duplicate_a_skip(tmp_path, caplog): try: with caplog.at_level(logging.WARNING, logger="rimport_relink"): - _entries, skips = rimport.expand_directories([d, d]) + _entries, skips = rimport.expand_directories([d, d], tmp_path) finally: os.chmod(locked, 0o700) @@ -299,8 +299,50 @@ def test_overlapping_named_directories_do_not_duplicate_a_skip(tmp_path): os.chmod(locked, 0o000) try: - _entries, skips = rimport.expand_directories([d, sub]) + _entries, skips = rimport.expand_directories([d, sub], tmp_path) finally: os.chmod(locked, 0o700) assert len(skips) == 1 + + +def test_directory_outside_the_root_is_not_expanded(tmp_path): + """Expansion is scoped to the inputdata tree. A directory outside it passes through as a + named entry for the pre-flight gate to reject, exactly as a nonexistent path does.""" + root = tmp_path / "inputdata" + root.mkdir() + outside = tmp_path / "elsewhere" + outside.mkdir() + (outside / "a.nc").write_text("a") + + entries, skips = rimport.expand_directories([outside], root) + + assert entries == [rimport.Entry(outside, True)] + assert skips == [] + + +def test_directory_outside_the_root_is_not_even_walked(tmp_path, monkeypatch): + """Declining to expand must happen BEFORE the walk, not by discarding its results: + walking recurses, and the whole point is not to stat a tree we have no business in.""" + root = tmp_path / "inputdata" + root.mkdir() + outside = tmp_path / "elsewhere" + outside.mkdir() + + def _fail(_path): + raise AssertionError("walk_files must not be called for a path outside the root") + + monkeypatch.setattr(rimport, "walk_files", _fail) + + rimport.expand_directories([outside], root) + + +def test_directory_at_the_root_itself_is_expanded(tmp_path): + """The boundary case: the root is not outside itself.""" + root = tmp_path / "inputdata" + root.mkdir() + (root / "a.nc").write_text("a") + + entries, _skips = rimport.expand_directories([root], root) + + assert entries == [rimport.Entry(root / "a.nc", False)] From c3c7a63f94baf9cc09b3bf5a1638e4f610c15a84 Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Wed, 9 Sep 2026 11:48:32 -0600 Subject: [PATCH 39/47] rimport: Pin the check order, and scope the enumeration promise The reorder of validate_source_path -- the riskiest part of the previous change, since that function gates every path on every run -- was pinned by nothing: reverting it left the whole suite green. The existing directory test uses a directory inside the root, so it exercises the backstop but cannot tell the two orderings apart. A directory outside the root now has its own test, and the main-level test asserts the reason string rather than just the exit code. Both fail against the reverted order. The resolve-rather-than-lexical scope test was likewise unpinned; a directory reached through a symlinked parent now covers it. README and the positional help both still promised that any name may be a directory and everything beneath it is enumerated, which stopped being true when expansion was scoped to the tree. The --file help already carried the containment qualifier, so the two channels documented the same behaviour differently. validate_source_path's docstring listed its checks in the pre-reorder order. Suite: 341 passed. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 2 +- rimport | 17 +++++++++-------- tests/rimport/test_expand_directories.py | 18 ++++++++++++++++++ tests/rimport/test_main.py | 2 ++ tests/rimport/test_validate_source_path.py | 22 ++++++++++++++++++++++ 5 files changed, 52 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index 5d815b3..0e4d1d5 100644 --- a/README.md +++ b/README.md @@ -17,7 +17,7 @@ Notes: ### Directory arguments -Any name you give `rimport` — positional, `--file`, or a `--list` entry — may be a directory. Every file beneath it is enumerated recursively and acted on. The directory itself is never copied to staging or replaced with a symlink. A symlink to a directory is the one carve-out: it is not expanded, and is treated as a single entry. +Any name you give `rimport` — positional, `--file`, or a `--list` entry — may be a directory inside the inputdata tree. Every file beneath it is enumerated recursively and acted on. A directory outside the tree is rejected without being enumerated, exactly as a file outside it is. The directory itself is never copied to staging or replaced with a symlink. A symlink to a directory is the one carve-out: it is not expanded, and is treated as a single entry. A file found by enumeration that cannot be staged does not abort the run. It is reported, skipped, and repeated in a summary at the end so it does not scroll away. A bad name you gave directly is still fatal, and nothing is published. diff --git a/rimport b/rimport index 9b06299..38dc7a7 100755 --- a/rimport +++ b/rimport @@ -106,9 +106,10 @@ def build_parser() -> argparse.ArgumentParser: nargs="*", help=( "One or more files to process. (Optional; can use --file instead to process just one.)" - " A relative name is resolved against the current directory; there is no fallback to" - " the inputdata root. If the name is a directory, every file beneath it is" - " enumerated recursively and acted on; a symlink to a directory is not expanded." + " Must be in the CESM inputdata directory. A relative name is resolved against the" + " current directory; there is no fallback to the inputdata root. If the name is a" + " directory, every file beneath it is enumerated recursively and acted on; a symlink" + " to a directory is not expanded." ), ) @@ -406,11 +407,11 @@ def validate_source_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. + This is the pre-flight half of `stage_data`'s checks, in the order they run: broken + symlink, live symlink whose target is outside staging, missing file, outside the inputdata + root, already under the staging directory, and 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 diff --git a/tests/rimport/test_expand_directories.py b/tests/rimport/test_expand_directories.py index e07ad8d..cfc3b87 100644 --- a/tests/rimport/test_expand_directories.py +++ b/tests/rimport/test_expand_directories.py @@ -346,3 +346,21 @@ def test_directory_at_the_root_itself_is_expanded(tmp_path): entries, _skips = rimport.expand_directories([root], root) assert entries == [rimport.Entry(root / "a.nc", False)] + + +def test_scope_is_decided_after_resolving_symlinks(tmp_path): + """The scope test resolves both sides, matching validate_source_path, so a directory + reached through a symlinked parent is judged by where it really is rather than by how it + was spelled. A lexical test would call this one outside the root and decline to expand + it; the two checks would then disagree about the same path.""" + root = tmp_path / "inputdata" + (root / "lnd").mkdir(parents=True) + (root / "lnd" / "a.nc").write_text("a") + # A door into the tree from outside it. Spelled through here, the path is lexically + # outside `root` but resolves inside. + door = tmp_path / "door" + door.symlink_to(root) + + entries, _skips = rimport.expand_directories([door / "lnd"], root) + + assert entries == [rimport.Entry(door / "lnd" / "a.nc", False)] diff --git a/tests/rimport/test_main.py b/tests/rimport/test_main.py index 34ded9c..7c6312e 100644 --- a/tests/rimport/test_main.py +++ b/tests/rimport/test_main.py @@ -758,6 +758,8 @@ def test_named_directory_outside_the_inputdata_root_is_rejected_not_walked( assert "nothing was published" in captured.err # The directory itself is the failure, named once. Not its contents. assert "1 of 1 file(s) failed pre-flight validation" in captured.err + # The reason must be the actionable one, not the is-a-directory backstop. + assert "source not under inputdata root" in captured.err assert str(outside / "a.nc") not in captured.err # Nothing beneath it may have been enumerated. assert "expanded" not in captured.out diff --git a/tests/rimport/test_validate_source_path.py b/tests/rimport/test_validate_source_path.py index 7f136b3..f92088c 100644 --- a/tests/rimport/test_validate_source_path.py +++ b/tests/rimport/test_validate_source_path.py @@ -114,6 +114,28 @@ def test_error_directory(tmp_path): assert "source is a directory, not a file" in str(result) +def test_error_directory_outside_root_names_containment_not_directoryness(tmp_path): + """Containment is diagnosed before the is-a-directory backstop, so the message names the + reason the user can act on. Told "source is a directory, not a file" they would + reasonably answer that directories are supported now. + + Directories inside the root never reach this in a normal run -- they are expanded -- so + the backstop above and this case are the two orderings that have to stay distinguished. + """ + inputdata_root = tmp_path / "inputdata" + inputdata_root.mkdir() + staging_root = tmp_path / "staging" + staging_root.mkdir() + + src = tmp_path / "elsewhere" + src.mkdir() + + result = rimport.validate_source_path(src, inputdata_root, staging_root) + assert isinstance(result, RuntimeError) + assert "source not under inputdata root" in str(result) + assert "not a file" not 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" From 0d1491755666b5bb40187cbc6a069d5da95f56f1 Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Wed, 9 Sep 2026 12:39:23 -0600 Subject: [PATCH 40/47] docs: Say why a symlink to a directory is not enumerated The README stated the carve-out without its reasons, so it read as an arbitrary exception rather than a consequence of what a symlink means in a published inputdata tree. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/README.md b/README.md index 0e4d1d5..21647c7 100644 --- a/README.md +++ b/README.md @@ -19,6 +19,12 @@ Notes: Any name you give `rimport` — positional, `--file`, or a `--list` entry — may be a directory inside the inputdata tree. Every file beneath it is enumerated recursively and acted on. A directory outside the tree is rejected without being enumerated, exactly as a file outside it is. The directory itself is never copied to staging or replaced with a symlink. A symlink to a directory is the one carve-out: it is not expanded, and is treated as a single entry. +Why symlinks to directories are left alone, rather than enumerated: + +- Naming one already meant something before directories could be named at all, and that meaning is unchanged: if it points into the staging directory it is reported as already published, and otherwise it is an error. +- A symlink into staging is what `rimport` itself creates, so a symlink here is usually a published file rather than a detour to follow. Treating it as a single entry is what lets you re-run `rimport` over a tree it has already published. +- Enumeration never follows a directory symlink either, so it cannot loop on a link that points at its own ancestor, and cannot wander outside the directory you named and publish files you did not ask for. The rule for a name you give matches the rule used while walking, so a path behaves the same whichever way `rimport` reaches it. + A file found by enumeration that cannot be staged does not abort the run. It is reported, skipped, and repeated in a summary at the end so it does not scroll away. A bad name you gave directly is still fatal, and nothing is published. Exit codes: From 8df280e14e7f7e7d90fedbbaba8a6a1d02815e82 Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Wed, 9 Sep 2026 18:50:13 -0600 Subject: [PATCH 41/47] rimport: Name the expansion flag for what it means, and drop a rename `is_real_dir` stopped describing its variable once an out-of-root directory started clearing it: it is true only for a real directory inside the tree, which is what "expandable" says. Reverts an unrelated rename of the staging loop's `p`, which had no reason beyond churn. Co-Authored-By: Claude Opus 5 (1M context) --- rimport | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/rimport b/rimport index 38dc7a7..af09bfd 100755 --- a/rimport +++ b/rimport @@ -315,7 +315,7 @@ def expand_directories( for path in paths: try: - is_real_dir = path.is_dir() and not path.is_symlink() + is_expandable_dir = path.is_dir() and not path.is_symlink() except OSError as exc: # Path.is_dir() ignores only ENOENT, ENOTDIR, EBADF and ELOOP. It propagates # everything else -- EACCES included, on every supported version -- so a path @@ -324,12 +324,12 @@ def expand_directories( # the fatal pre-flight block and the run exits 2 having published nothing. skips.append(Skip(path, exc)) continue - if is_real_dir and not path.resolve().is_relative_to(inputdata_root.resolve()): + if is_expandable_dir and not path.resolve().is_relative_to(inputdata_root.resolve()): # Outside the tree: hand it to the pre-flight gate unexpanded. Resolve both # sides, as validate_source_path does, so the two agree about what "outside" # means for a path reached through a symlinked parent. - is_real_dir = False - if is_real_dir: + is_expandable_dir = False + if is_expandable_dir: n_dirs += 1 found, walk_skips = walk_files(path) skips.extend(walk_skips) @@ -862,14 +862,14 @@ def main(argv: List[str] | None = None) -> int: # Execute the new action per file errors = 0 - for path in to_stage: - logger.info("'%s':", path) + for p in to_stage: + logger.info("'%s':", p) try: - stage_data(path, root, staging_root, args.check) + stage_data(p, root, staging_root, args.check) except Exception as e: # pylint: disable=broad-exception-caught # General Exception keeps CLI robust for batch runs errors += 1 - logger.error("%srimport: error processing %s: %s", INDENT, path, e) + logger.error("%srimport: error processing %s: %s", INDENT, p, e) if not errors and not args.check: logger.info("\nNo need to run relink.py") From faa6a0cbae0fcb44c68d9658f4d23eb8cb2c7cb2 Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Wed, 9 Sep 2026 18:51:39 -0600 Subject: [PATCH 42/47] rimport: Say "item(s)", and stop printing the path twice A directory can be the thing that failed pre-flight or was skipped, so the fatal line, the skip summary and the two --help strings that describe them now say "item(s)". The expansion count keeps "file(s)": it only ever counts files, since a walk yields no directories. Reasons now render through reason_text(), which uses OSError.strerror rather than str(). Both messages already name the path, and OSError's str() appends it again, so every unreadable-directory line carried it twice. Co-Authored-By: Claude Opus 5 (1M context) --- rimport | 32 ++++++++++++++++++++++++-------- tests/rimport/test_cmdline.py | 8 ++++---- tests/rimport/test_main.py | 14 ++++++-------- 3 files changed, 34 insertions(+), 20 deletions(-) diff --git a/rimport b/rimport index af09bfd..f382a98 100755 --- a/rimport +++ b/rimport @@ -64,7 +64,7 @@ def build_parser() -> argparse.ArgumentParser: " 1: a file could not be staged\n" " 2: nothing was published -- a bad command line, a missing or empty\n" " --list file, or a name you gave failing validation\n" - " 3: finished, but one or more files were skipped (listed at the end)\n" + " 3: finished, but one or more items were skipped (listed at the end)\n" "\n" "a name you gave failing is fatal (2). a file found by expanding a\n" "directory you named is skipped instead, and the run continues (3).\n" @@ -122,7 +122,7 @@ def build_parser() -> argparse.ArgumentParser: "-c", action="store_true", help=( - "Check whether file(s) is/are already published, without staging anything. A bad" + "Check whether item(s) is/are already published, without staging anything. A bad" " name that you gave aborts before any file is checked, reporting all bad names" " at once; a file found by enumerating a directory is instead reported and" " skipped individually." @@ -363,7 +363,9 @@ def expand_directories( # about the directory being walked: a directory can be named AND discovered beneath # another named one, and then both are true of it. if not skip.named: - logger.warning("%srimport: skipping '%s': %s", INDENT, skip.path, skip.reason) + logger.warning( + "%srimport: skipping '%s': %s", INDENT, skip.path, reason_text(skip.reason) + ) entries = [Entry(path, named) for path, named in named_by_path.items()] return entries, skips @@ -383,9 +385,21 @@ def report_skips(skips: List[Skip]) -> None: if not skips: return - logger.error("rimport: %d file(s) skipped (not stageable):", len(skips)) + logger.error("rimport: %d item(s) skipped (not stageable):", len(skips)) for skip in skips: - logger.error("%s%s: %s", INDENT, skip.path, skip.reason) + logger.error("%s%s: %s", INDENT, skip.path, reason_text(skip.reason)) + + +def reason_text(reason: Exception) -> str: + """Render a skip or failure reason for a message that already names the path. + + `OSError`'s str() appends the filename, so a line built as "'': " prints + the path twice. Its `strerror` is the same message without that, and errno is dropped + deliberately: "Permission denied" is what the user can act on. + """ + if isinstance(reason, OSError) and reason.strerror: + return reason.strerror + return str(reason) def check_relink_worked(src: Path, dst: Path) -> None: @@ -847,17 +861,19 @@ def main(argv: List[str] | None = None) -> int: elif entry.named: named_failures.append((entry.path, error)) else: - logger.warning("%srimport: skipping '%s': %s", INDENT, entry.path, error) + logger.warning( + "%srimport: skipping '%s': %s", INDENT, entry.path, reason_text(error) + ) skips.append(Skip(entry.path, error)) if named_failures: logger.error( - "rimport: %d of %d file(s) failed pre-flight validation; nothing was published:", + "rimport: %d of %d item(s) failed pre-flight validation; nothing was published:", len(named_failures), n_considered, ) for path, error in named_failures: - logger.error("%srimport: '%s': %s", INDENT, path, error) + logger.error("%srimport: '%s': %s", INDENT, path, reason_text(error)) return 2 # Execute the new action per file diff --git a/tests/rimport/test_cmdline.py b/tests/rimport/test_cmdline.py index e33bed1..030930a 100644 --- a/tests/rimport/test_cmdline.py +++ b/tests/rimport/test_cmdline.py @@ -322,7 +322,7 @@ def test_error_for_nonexistent_file(self, rimport_script, test_env, rimport_env) ) # 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 + # reports "N of M item(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 @@ -1135,7 +1135,7 @@ def _run_mixed_validity_list(self, rimport_script, test_env, rimport_env, *, che # 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 "2 of 3 item(s) failed pre-flight validation" in result.stderr assert f"source not found: {missing_file}" in result.stderr assert f"Source is a broken symlink: {broken_link}" in result.stderr @@ -1315,7 +1315,7 @@ def test_skip_summary_repeats_skipped_files_on_stderr_at_the_end( ) assert result.returncode == 3 - assert "1 file(s) skipped (not stageable)" in result.stderr + assert "1 item(s) skipped (not stageable)" in result.stderr assert "broken.nc" in result.stderr # The summary is the LAST thing on stderr. The broken-symlink message names the # link itself, not its target, so the final line ends with broken.nc. @@ -1348,7 +1348,7 @@ def test_skip_summary_survives_quiet_mode( ) assert result.returncode == 3 - assert "1 file(s) skipped (not stageable)" in result.stderr + assert "1 item(s) skipped (not stageable)" in result.stderr def test_expansion_count_is_logged_before_staging( self, rimport_script, test_env, rimport_env diff --git a/tests/rimport/test_main.py b/tests/rimport/test_main.py index 7c6312e..89b8d09 100644 --- a/tests/rimport/test_main.py +++ b/tests/rimport/test_main.py @@ -552,7 +552,7 @@ def test_preflight_gate_rejects_whole_batch_and_never_calls_stage_data( mock_stage_data.assert_not_called() captured = capsys.readouterr() - assert "2 of 3 file(s) failed pre-flight validation" in captured.err + assert "2 of 3 item(s) failed pre-flight validation" in captured.err assert f"source not found: {missing}" in captured.err assert f"Source is a broken symlink: {broken}" in captured.err @@ -609,7 +609,7 @@ def test_discovered_failure_warns_skips_and_returns_3( # Reported inline at WARNING (stdout) as the run reaches it... assert "skipping" in captured.out # ...and repeated at ERROR (stderr) at the very end, where it cannot be scrolled past. - assert "1 file(s) skipped (not stageable)" in captured.err + assert "1 item(s) skipped (not stageable)" in captured.err assert "broken.nc" in captured.err @patch.object(rimport, "get_staging_root") @@ -641,7 +641,7 @@ def test_named_failure_still_aborts_everything_including_discovered_files( # Pins the named-vs-discovered split itself: only the named `missing` is fatal, # so the count is 1 of 2 (good.nc, discovered under subdir, does not count against # it) -- not 2 of 2, which is what today's un-enumerated pre-flight gate reports. - assert "1 of 2 file(s) failed pre-flight validation" in captured.err + assert "1 of 2 item(s) failed pre-flight validation" in captured.err @patch.object(rimport, "get_staging_root") @patch.object(rimport, "ensure_running_as") @@ -723,7 +723,7 @@ def test_named_unreadable_directory_is_fatal_not_a_skip( # Pins the denominator too: `locked` never became an Entry, so a count taken from # entries alone reports the nonsense "1 of 0". Two paths were considered here -- # `locked` and the good.nc discovered under `ok`. - assert "1 of 2 file(s) failed pre-flight validation" in captured.err + assert "1 of 2 item(s) failed pre-flight validation" in captured.err # No named argument may have published -- not just the one that failed. assert not any(staging_root.rglob("*")) @@ -757,7 +757,7 @@ def test_named_directory_outside_the_inputdata_root_is_rejected_not_walked( captured = capsys.readouterr() assert "nothing was published" in captured.err # The directory itself is the failure, named once. Not its contents. - assert "1 of 1 file(s) failed pre-flight validation" in captured.err + assert "1 of 1 item(s) failed pre-flight validation" in captured.err # The reason must be the actionable one, not the is-a-directory backstop. assert "source not under inputdata root" in captured.err assert str(outside / "a.nc") not in captured.err @@ -794,9 +794,7 @@ def test_naming_an_unreadable_directory_twice_reports_it_once( assert result == 2 captured = capsys.readouterr() # Two distinct paths were considered: `locked` and the good.nc discovered under `ok`. - assert "1 of 2 file(s) failed pre-flight validation" in captured.err - # Count list ENTRIES, not path occurrences: the OSError repr repeats the path within - # a single line, so a raw substring count sees two even when one entry is printed. + assert "1 of 2 item(s) failed pre-flight validation" in captured.err assert captured.err.count(f"rimport: '{locked}'") == 1 @patch.object(rimport, "get_staging_root") From fff73d42a7b3770b73918de91602b3a621f3328b Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Wed, 9 Sep 2026 18:53:29 -0600 Subject: [PATCH 43/47] tests(rimport): State what each test pins, not how it got there Test docstrings had become a running commentary on the branch's own development -- "the counterpart", references to task numbers and commit hashes, and descriptions of behaviour that used to be wrong. None of that helps someone meeting a test cold, and "the counterpart" had no referent at all once the tests moved apart. Each now says, in the present tense, what the test pins. One was outright false: a docstring still explained that Skip carries no provenance, which stopped being true when it gained a `named` field. Also renames test_directory_named_twice_is_walked_once to ..._is_reported_once, and drops a docstring's list of which paths in a fixture are invalid, which would drift. Co-Authored-By: Claude Opus 5 (1M context) --- rimport | 7 ++-- tests/rimport/test_cmdline.py | 8 ++-- tests/rimport/test_expand_directories.py | 25 +++++++------ tests/rimport/test_main.py | 43 +++++++++------------- tests/rimport/test_validate_source_path.py | 2 +- 5 files changed, 38 insertions(+), 47 deletions(-) diff --git a/rimport b/rimport index f382a98..ff39843 100755 --- a/rimport +++ b/rimport @@ -691,10 +691,9 @@ def get_files_to_process(file: str, filelist: str, items_to_process: list): int: Result code """ cli_args = ([file] if file is not None else []) + list(items_to_process or []) - # An empty name would anchor to cwd and resolve to the cwd itself. That was harmless - # only while a directory was rejected outright; now that a directory expands, an unset - # shell variable (`rimport "$maybe_unset"`) would recursively publish the whole subtree - # the user is standing in. Refuse it. + # An empty name anchors to cwd and resolves to the cwd itself, which then expands: an + # unset shell variable (`rimport "$maybe_unset"`) would recursively publish the whole + # subtree the user is standing in. Refuse it. empty_cli_args = [name for name in cli_args if not str(name).strip()] if empty_cli_args: logger.error( diff --git a/tests/rimport/test_cmdline.py b/tests/rimport/test_cmdline.py index 030930a..a7f6bc5 100644 --- a/tests/rimport/test_cmdline.py +++ b/tests/rimport/test_cmdline.py @@ -973,8 +973,8 @@ def test_directory_argument_from_subdir_stages_contents_and_leaves_tree_intact( assert (staging_mirror / "data.nc").read_text() == "clm2 data" assert inner_file.is_symlink() - # The 75c79cd anti-corruption assertion, preserved: the DIRECTORY itself was never - # renamed, never symlinked away, and no failed-rollback '.tmp' was left behind. + # The directory itself must survive untouched: still a real directory, never + # symlinked away, and no '.tmp' left behind by a half-finished replacement. assert subdir.is_dir() and not subdir.is_symlink(), ( f"clm2 should still be a plain, non-symlink directory; " f"is_dir={subdir.is_dir()} is_symlink={subdir.is_symlink()}" @@ -1248,8 +1248,8 @@ def test_file_option_expands_a_directory(self, rimport_script, test_env, rimport assert result.returncode == 0, f"Command unexpectedly failed: {result.stderr}" assert (staging_root / "lnd" / "clm2" / "a.nc").read_text() == "a" assert (staging_root / "lnd" / "clm2" / "sub" / "b.nc").read_text() == "b" - # The directory itself must never be staged or replaced -- the corruption 75c79cd - # was added to prevent. + # The directory itself must survive untouched: still a real directory, never + # replaced by a symlink to a staged copy of itself. assert d.is_dir() and not d.is_symlink() def test_list_entry_expands_a_directory(self, rimport_script, test_env, rimport_env): diff --git a/tests/rimport/test_expand_directories.py b/tests/rimport/test_expand_directories.py index cfc3b87..833c6e5 100644 --- a/tests/rimport/test_expand_directories.py +++ b/tests/rimport/test_expand_directories.py @@ -133,7 +133,7 @@ def test_empty_directory_warns_but_is_not_a_skip(tmp_path, caplog): def test_logs_expansion_counts(tmp_path, caplog): - """The blast radius is visible before anything is staged.""" + """The blast radius is printed by `expand_directories()` -- i.e., before anything is staged.""" d1 = tmp_path / "d1" d1.mkdir() (d1 / "a.nc").write_text("a") @@ -199,7 +199,7 @@ def test_discovered_walk_skip_is_warned_where_it_happened(tmp_path, caplog): def test_named_unreadable_directory_is_not_warned_as_skipped(tmp_path, caplog): - """The counterpart. Since Task 11c a NAMED unreadable directory is fatal, so main reports + """A NAMED unreadable directory is fatal, and main reports it as such. Warning it as a failure; warning "skipping" here too would contradict "nothing was published".""" locked = tmp_path / "locked" locked.mkdir() @@ -215,10 +215,11 @@ def test_named_unreadable_directory_is_not_warned_as_skipped(tmp_path, caplog): def test_named_unreadable_directory_is_not_warned_even_when_also_discovered(tmp_path, caplog): - """The guard must ask "did the user name this path?", not "is this the directory I am - walking right now?". Naming both a tree and an unreadable directory inside it made the - walk of the tree warn "skipping" for a path main then reports as a fatal failure -- the - self-contradiction the guard exists to prevent, reached by a different route.""" + """A path can be named AND discovered at once: name a tree and an unreadable directory + inside it, and the walk of the tree finds what the user also typed. It is still named, + so main reports it as a fatal failure and nothing here may call it "skipping" -- the two + messages contradict each other. Provenance is whole-batch membership, not "is this the + directory being walked".""" d = tmp_path / "d" d.mkdir() (d / "a.nc").write_text("a") @@ -254,9 +255,9 @@ def test_expansion_count_is_logged_before_any_skip_warning(tmp_path, caplog): assert caplog.text.index("expanded 1 director(ies)") < caplog.text.index("skipping") -def test_directory_named_twice_is_walked_once(tmp_path, caplog): - """Naming the same directory twice is one directory, not two. Counting the walks instead - of the directories made the blast-radius line overstate itself.""" +def test_directory_named_twice_is_reported_once(tmp_path, caplog): + """Naming the same directory twice is one directory, not two, so the blast-radius line + counts it once.""" d = tmp_path / "d" d.mkdir() (d / "a.nc").write_text("a") @@ -268,9 +269,9 @@ def test_directory_named_twice_is_walked_once(tmp_path, caplog): def test_duplicate_arguments_do_not_duplicate_a_skip(tmp_path, caplog): - """Files de-duplicate; skips did not. Naming a directory twice walked it twice and - recorded the same unreadable subdirectory twice, so it was warned twice, listed twice in - the end-of-run summary, and counted twice.""" + """One unreadable directory is one skip, however many of the named arguments reach it. + Otherwise it is warned about twice, listed twice in the end-of-run summary, and counted + twice in the total.""" d = tmp_path / "d" d.mkdir() locked = d / "locked" diff --git a/tests/rimport/test_main.py b/tests/rimport/test_main.py index 89b8d09..0b289ef 100644 --- a/tests/rimport/test_main.py +++ b/tests/rimport/test_main.py @@ -517,9 +517,8 @@ def test_preflight_gate_rejects_whole_batch_and_never_calls_stage_data( tmp_path, capsys, ): - """Test main()'s pre-flight gate: a batch with a mix of valid and invalid paths - (missing, and a broken symlink) returns 2, logs every failure, and never calls - stage_data — not even for the valid path. + """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 @@ -638,9 +637,6 @@ def test_named_failure_still_aborts_everything_including_discovered_files( assert not (subdir / "good.nc").is_symlink() captured = capsys.readouterr() assert "nothing was published" in captured.err - # Pins the named-vs-discovered split itself: only the named `missing` is fatal, - # so the count is 1 of 2 (good.nc, discovered under subdir, does not count against - # it) -- not 2 of 2, which is what today's un-enumerated pre-flight gate reports. assert "1 of 2 item(s) failed pre-flight validation" in captured.err @patch.object(rimport, "get_staging_root") @@ -692,11 +688,7 @@ def test_named_unreadable_directory_is_fatal_not_a_skip( ): """A directory the USER NAMED that cannot be read is a named failure, so it aborts the batch -- it must not be demoted to a skip that lets other named arguments - publish anyway. - - Skip carries no provenance, so without an explicit check main's gate cannot tell a - named unreadable directory from one discovered inside a named tree. Getting this - wrong turns a hard stop into a partial publish. + publish anyway. Demoting it would turn a hard stop into a partial publish. """ inputdata_root = tmp_path / "inputdata" locked = inputdata_root / "locked" @@ -725,7 +717,7 @@ def test_named_unreadable_directory_is_fatal_not_a_skip( # `locked` and the good.nc discovered under `ok`. assert "1 of 2 item(s) failed pre-flight validation" in captured.err - # No named argument may have published -- not just the one that failed. + # No file may have published -- not just the one that failed. assert not any(staging_root.rglob("*")) assert not (other / "good.nc").is_symlink() @@ -734,12 +726,12 @@ def test_named_unreadable_directory_is_fatal_not_a_skip( def test_named_directory_outside_the_inputdata_root_is_rejected_not_walked( self, _mock_ensure_running_as, mock_get_staging_root, tmp_path, capsys ): - """A named FILE outside the inputdata root has always been fatal. A directory must not - get a softer verdict just because it can now be expanded: walking it turns the user's - own bad argument into a pile of discovered skips and a "finished" exit 3. + """A named path outside the inputdata root is fatal whether it is a file or a + directory. Expanding the directory instead would demote the user's own bad argument + to a pile of discovered skips and a "finished" exit 3. - It must also not be walked at all. Expansion recurses, so a mistyped `rimport ~` - would otherwise stat an arbitrarily large tree before rejecting every file in it. + It must also not be walked. Expansion recurses, so a mistyped `rimport ~` would + otherwise stat an arbitrarily large tree before rejecting every file in it. """ inputdata_root = tmp_path / "inputdata" inputdata_root.mkdir() @@ -770,9 +762,8 @@ def test_named_directory_outside_the_inputdata_root_is_rejected_not_walked( def test_naming_an_unreadable_directory_twice_reports_it_once( self, _mock_ensure_running_as, mock_get_staging_root, tmp_path, capsys ): - """The same path given twice is one bad path, not two. Walking it twice recorded two - identical Skips, which inflated both the failure list and the denominator: the user - typed two arguments and got "2 of 3 file(s) failed", with one path printed twice.""" + """The same path given twice is one bad path, not two: it is listed once, and + counted once in both halves of the "N of M" total.""" inputdata_root = tmp_path / "inputdata" locked = inputdata_root / "locked" locked.mkdir(parents=True) @@ -802,9 +793,9 @@ def test_naming_an_unreadable_directory_twice_reports_it_once( def test_unreadable_subdirectory_stays_a_skip( self, _mock_ensure_running_as, mock_get_staging_root, tmp_path, capsys ): - """The counterpart: an unreadable directory DISCOVERED beneath a named directory is - not a named failure, so it stays a warn-and-skip and its readable siblings still - publish. This is what stops the fix for the named case from over-reaching.""" + """An unreadable directory found BENEATH a named one was not named by the user, so + it is not a named failure: it is warned about, skipped, and its readable siblings + still publish. Only the path the user typed is allowed to abort the batch.""" inputdata_root = tmp_path / "inputdata" tree = inputdata_root / "tree" tree.mkdir(parents=True) @@ -832,9 +823,9 @@ def test_unreadable_subdirectory_stays_a_skip( def test_walk_skip_is_still_reported_when_the_run_aborts( self, _mock_ensure_running_as, mock_get_staging_root, tmp_path, capsys ): - """A named failure returns before the end-of-run summary, so a walk skip that is not - reported inline is never reported at all -- breaking the spec's promise that every - skip is reported twice.""" + """A skipped path must be reported even when the run goes on to abort. A named + failure returns before the end-of-run summary, so if the skip were not also + reported inline it would appear nowhere at all.""" inputdata_root = tmp_path / "inputdata" tree = inputdata_root / "tree" tree.mkdir(parents=True) diff --git a/tests/rimport/test_validate_source_path.py b/tests/rimport/test_validate_source_path.py index f92088c..a96869d 100644 --- a/tests/rimport/test_validate_source_path.py +++ b/tests/rimport/test_validate_source_path.py @@ -133,7 +133,7 @@ def test_error_directory_outside_root_names_containment_not_directoryness(tmp_pa result = rimport.validate_source_path(src, inputdata_root, staging_root) assert isinstance(result, RuntimeError) assert "source not under inputdata root" in str(result) - assert "not a file" not in str(result) + assert "is a directory" not in str(result) def test_error_file_outside_inputdata_root(tmp_path): From 6523dc616ce9ee888402b78966b574b92a8b6d9f Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Wed, 9 Sep 2026 18:55:16 -0600 Subject: [PATCH 44/47] tests(rimport): Make three tests check what they claim to test_expansion_count_is_logged_before_staging asserted only that the count line appeared, so it passed regardless of where it appeared; it now compares its position against the first staged-file line. The --check-on-a-directory test said in a comment that the directory itself is never described, and checked nothing of the sort. It now asserts on the per-item header, which is the only form that can distinguish the directory from the file beneath it -- the directory's name appears in that file's path either way. The named-unreadable-directory test had one good file, so its denominator matched the argument count and could not tell the two apart. A second good file separates them. Adds the reverse argument order for named-wins-over-discovered, which takes a different route through the accumulator, and gives the first-seen test a fixture where order is actually observable. Suite: 342 passed. Co-Authored-By: Claude Opus 5 (1M context) --- tests/rimport/test_cmdline.py | 14 +++++++++--- tests/rimport/test_expand_directories.py | 29 +++++++++++++++++++----- tests/rimport/test_main.py | 15 ++++++++---- 3 files changed, 44 insertions(+), 14 deletions(-) diff --git a/tests/rimport/test_cmdline.py b/tests/rimport/test_cmdline.py index a7f6bc5..5fb1d6f 100644 --- a/tests/rimport/test_cmdline.py +++ b/tests/rimport/test_cmdline.py @@ -1069,7 +1069,11 @@ def test_check_directory_argument_reports_each_file_inside( assert not inner_file.is_symlink() # The file is reported on its own terms, and the directory itself is never - # described as published or downloadable. + # described as published or downloadable. rimport heads each reported item with + # "'':", so the directory's own header is what must be absent -- its name + # appears anyway inside the path of the file beneath it. + assert f"'{inner_file}':" in result.stdout + assert f"'{subdir}':" not in result.stdout assert "not already published" in result.stdout assert "available for download" not in result.stdout.lower() assert subdir.is_dir() and not subdir.is_symlink() @@ -1353,7 +1357,8 @@ def test_skip_summary_survives_quiet_mode( def test_expansion_count_is_logged_before_staging( self, rimport_script, test_env, rimport_env ): - """The blast radius is visible before anything is written.""" + """The blast radius reaches the user before the first file is written, so a run over + an unexpectedly large tree can still be interrupted.""" inputdata_root = test_env["inputdata_root"] subdir = inputdata_root / "lnd" @@ -1376,4 +1381,7 @@ def test_expansion_count_is_logged_before_staging( ) assert result.returncode == 0 - assert "expanded 1 director(ies) to 2 file(s)" in result.stdout + assert ( + result.stdout.index("expanded 1 director(ies) to 2 file(s)") + < result.stdout.index("staged") + ) diff --git a/tests/rimport/test_expand_directories.py b/tests/rimport/test_expand_directories.py index 833c6e5..4f4a98c 100644 --- a/tests/rimport/test_expand_directories.py +++ b/tests/rimport/test_expand_directories.py @@ -89,16 +89,33 @@ def test_duplicates_collapse_and_named_wins(tmp_path): assert entries == [rimport.Entry(inner, True)] -def test_duplicate_order_is_first_seen(tmp_path): - """De-duplication preserves the order a path was first encountered.""" +def test_named_wins_over_discovered_whichever_comes_first(tmp_path): + """Naming the file before the directory that contains it must reach the same verdict. + The two orders take different routes -- one sets `named` and the walk must not clear it, + the other must upgrade an entry the walk already recorded.""" d = tmp_path / "d" d.mkdir() - (d / "a.nc").write_text("a") - (d / "b.nc").write_text("b") + inner = d / "inner.nc" + inner.write_text("data") + + entries, _skips = rimport.expand_directories([inner, d], tmp_path) + + assert entries == [rimport.Entry(inner, True)] + + +def test_duplicate_order_is_first_seen(tmp_path): + """De-duplication preserves the order a path was first encountered, across arguments.""" + d1 = tmp_path / "d1" + d1.mkdir() + (d1 / "b.nc").write_text("b") + d2 = tmp_path / "d2" + d2.mkdir() + (d2 / "a.nc").write_text("a") - entries, _skips = rimport.expand_directories([d, d], tmp_path) + # d1 first, so its file leads, even though "a.nc" sorts before "b.nc" within a walk. + entries, _skips = rimport.expand_directories([d1, d2, d1], tmp_path) - assert [e.path.name for e in entries] == ["a.nc", "b.nc"] + assert [e.path.name for e in entries] == ["b.nc", "a.nc"] def test_walk_skips_are_passed_through(tmp_path): diff --git a/tests/rimport/test_main.py b/tests/rimport/test_main.py index 0b289ef..825d082 100644 --- a/tests/rimport/test_main.py +++ b/tests/rimport/test_main.py @@ -697,6 +697,7 @@ def test_named_unreadable_directory_is_fatal_not_a_skip( other = inputdata_root / "ok" other.mkdir() (other / "good.nc").write_text("good") + (other / "also-good.nc").write_text("good") staging_root = tmp_path / "staging" staging_root.mkdir() mock_get_staging_root.return_value = staging_root @@ -712,10 +713,11 @@ def test_named_unreadable_directory_is_fatal_not_a_skip( assert result == 2 captured = capsys.readouterr() assert "nothing was published" in captured.err - # Pins the denominator too: `locked` never became an Entry, so a count taken from - # entries alone reports the nonsense "1 of 0". Two paths were considered here -- - # `locked` and the good.nc discovered under `ok`. - assert "1 of 2 item(s) failed pre-flight validation" in captured.err + # `locked` never became an Entry, so a denominator taken from entries alone would + # report "1 of 2". Three items were considered: `locked` and the two files + # discovered under `ok`. Two arguments were given, so a denominator that counted + # those instead would also read "1 of 2". + assert "1 of 3 item(s) failed pre-flight validation" in captured.err # No file may have published -- not just the one that failed. assert not any(staging_root.rglob("*")) @@ -859,7 +861,10 @@ def test_unreadable_parent_directory_is_an_error_not_a_traceback( """Naming a file inside a directory that cannot be read is a user error, and the help text promises exit 2 for one. Path.is_dir() propagates EACCES rather than returning False -- it ignores only ENOENT, ENOTDIR, EBADF and ELOOP -- so an unguarded probe - turns that into a stack trace on every supported version.""" + turns that into a stack trace on every supported version. + + An escaping exception errors this test rather than failing it, so reaching the + assertions below at all is half of what is being checked.""" inputdata_root = tmp_path / "inputdata" locked = inputdata_root / "locked" locked.mkdir(parents=True) From 2bcba2b64884b3d5e22c2fbe04b37cc4c7d5519a Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Wed, 9 Sep 2026 18:56:50 -0600 Subject: [PATCH 45/47] tests(rimport): Pair every exit-code test with its --check twin --check had one test, for exit 3, sitting well away from the run it mirrors, and its docstring claimed to cover "the same exit codes" while checking one. Each of the four exit-code paths now has a --check twin immediately after its non-check equivalent, so a divergence between the two modes shows up as an adjacent pair disagreeing. The exit-0 and exit-3 twins assert nothing was written, which is what makes them catch --check being ignored. The exit-2 twin instead pins that the good file is never reported on, since the gate runs before any checking; the exit-1 twin pins that precedence 1 > 3 holds when nothing was being written at all. Suite: 345 passed. Co-Authored-By: Claude Opus 5 (1M context) --- tests/rimport/test_main.py | 90 +++++++++++++++++++++++++++++++++++--- 1 file changed, 84 insertions(+), 6 deletions(-) diff --git a/tests/rimport/test_main.py b/tests/rimport/test_main.py index 825d082..dd954c0 100644 --- a/tests/rimport/test_main.py +++ b/tests/rimport/test_main.py @@ -583,6 +583,31 @@ def test_directory_argument_stages_the_files_inside_it( assert subdir.is_dir() and not subdir.is_symlink() assert not (staging_root / "lnd").is_symlink() + @patch.object(rimport, "get_staging_root") + @patch.object(rimport, "ensure_running_as") + def test_check_directory_argument_reports_each_file_and_stages_nothing( + self, _mock_ensure_running_as, mock_get_staging_root, tmp_path, capsys + ): + """--check reaches the same verdict on the same input without writing anything.""" + inputdata_root = tmp_path / "inputdata" + subdir = inputdata_root / "lnd" + subdir.mkdir(parents=True) + staging_root = tmp_path / "staging" + staging_root.mkdir() + mock_get_staging_root.return_value = staging_root + (subdir / "a.nc").write_text("a") + (subdir / "b.nc").write_text("b") + + result = rimport.main( + ["-inputdata", str(inputdata_root), str(subdir), "--check"] + ) + + assert result == 0 + captured = capsys.readouterr() + assert "a.nc" in captured.out and "b.nc" in captured.out + assert not any(staging_root.rglob("*")) + assert not (subdir / "a.nc").is_symlink() + @patch.object(rimport, "get_staging_root") @patch.object(rimport, "ensure_running_as") def test_discovered_failure_warns_skips_and_returns_3( @@ -611,6 +636,31 @@ def test_discovered_failure_warns_skips_and_returns_3( assert "1 item(s) skipped (not stageable)" in captured.err assert "broken.nc" in captured.err + @patch.object(rimport, "get_staging_root") + @patch.object(rimport, "ensure_running_as") + def test_check_discovered_failure_also_returns_3( + self, _mock_ensure_running_as, mock_get_staging_root, tmp_path, capsys + ): + """--check reports the same skip and the same exit code, staging nothing.""" + inputdata_root = tmp_path / "inputdata" + subdir = inputdata_root / "lnd" + subdir.mkdir(parents=True) + staging_root = tmp_path / "staging" + staging_root.mkdir() + mock_get_staging_root.return_value = staging_root + (subdir / "good.nc").write_text("good") + (subdir / "broken.nc").symlink_to(inputdata_root / "nonexistent.nc") + + result = rimport.main( + ["-inputdata", str(inputdata_root), str(subdir), "--check"] + ) + + assert result == 3 + captured = capsys.readouterr() + assert "1 item(s) skipped (not stageable)" in captured.err + assert not any(staging_root.rglob("*")) + assert not (subdir / "good.nc").is_symlink() + @patch.object(rimport, "get_staging_root") @patch.object(rimport, "ensure_running_as") def test_named_failure_still_aborts_everything_including_discovered_files( @@ -639,6 +689,32 @@ def test_named_failure_still_aborts_everything_including_discovered_files( assert "nothing was published" in captured.err assert "1 of 2 item(s) failed pre-flight validation" in captured.err + @patch.object(rimport, "get_staging_root") + @patch.object(rimport, "ensure_running_as") + def test_check_named_failure_also_aborts_before_checking_anything( + self, _mock_ensure_running_as, mock_get_staging_root, tmp_path, capsys + ): + """--check is gated by the same pre-flight, so a named failure aborts it too and the + good file is never reported on -- fix the bad name and re-run to see the rest.""" + inputdata_root = tmp_path / "inputdata" + subdir = inputdata_root / "lnd" + subdir.mkdir(parents=True) + staging_root = tmp_path / "staging" + staging_root.mkdir() + mock_get_staging_root.return_value = staging_root + (subdir / "good.nc").write_text("good") + missing = inputdata_root / "missing.nc" + + result = rimport.main( + ["-inputdata", str(inputdata_root), str(subdir), str(missing), "--check"] + ) + + assert result == 2 + captured = capsys.readouterr() + assert "nothing was published" in captured.err + assert "good.nc" not in captured.out + assert not any(staging_root.rglob("*")) + @patch.object(rimport, "get_staging_root") @patch.object(rimport, "ensure_running_as") def test_staging_error_outranks_skip_in_exit_code( @@ -662,10 +738,11 @@ def test_staging_error_outranks_skip_in_exit_code( @patch.object(rimport, "get_staging_root") @patch.object(rimport, "ensure_running_as") - def test_check_mode_also_returns_3_for_skips( + def test_check_error_also_outranks_skip_in_exit_code( self, _mock_ensure_running_as, mock_get_staging_root, tmp_path ): - """--check uses the same exit codes, including 3.""" + """Precedence 1 > 3 holds under --check: a file that fails while being checked is a + real failure, not a skip, even though nothing was being written.""" inputdata_root = tmp_path / "inputdata" subdir = inputdata_root / "lnd" subdir.mkdir(parents=True) @@ -675,11 +752,12 @@ def test_check_mode_also_returns_3_for_skips( (subdir / "good.nc").write_text("good") (subdir / "broken.nc").symlink_to(inputdata_root / "nonexistent.nc") - result = rimport.main( - ["-inputdata", str(inputdata_root), str(subdir), "--check"] - ) + with patch.object(rimport, "stage_data", side_effect=RuntimeError("boom")): + result = rimport.main( + ["-inputdata", str(inputdata_root), str(subdir), "--check"] + ) - assert result == 3 + assert result == 1 @patch.object(rimport, "get_staging_root") @patch.object(rimport, "ensure_running_as") From 9fa0db6522a6046d7e240fd8cb0c8c9a57997d9b Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Wed, 9 Sep 2026 18:57:33 -0600 Subject: [PATCH 46/47] docs: Give exit codes their own README section The table had been sitting inside "Directory arguments" because it replaced a bullet in that part of the file, but it describes every run. It now stands on its own, says "items" where a directory can be the thing skipped, and carries the named-vs-discovered rule that makes 2 and 3 predictable to a script. Drops the symlink bullet that explained the carve-out by reference to how things used to work. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 21647c7..f1bbb8f 100644 --- a/README.md +++ b/README.md @@ -12,7 +12,7 @@ Notes: - 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 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 path you named (all `--file`/`--list`/positional entries together). If any of them fail — missing, 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. A file found by expanding a directory you named is not covered by this promise; it is skipped on its own, and the rest of the run continues (see "Directory arguments" below). 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). +- Before staging anything, `rimport` validates every path you named (all `--file`/`--list`/positional entries together). If any of them fail — missing, 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. An invalid file found by expanding a directory you named is not covered by this promise; it is skipped on its own, and the rest of the run continues (see "Directory arguments" below). 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 path you named 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. ### Directory arguments @@ -21,23 +21,24 @@ Any name you give `rimport` — positional, `--file`, or a `--list` entry — ma Why symlinks to directories are left alone, rather than enumerated: -- Naming one already meant something before directories could be named at all, and that meaning is unchanged: if it points into the staging directory it is reported as already published, and otherwise it is an error. - A symlink into staging is what `rimport` itself creates, so a symlink here is usually a published file rather than a detour to follow. Treating it as a single entry is what lets you re-run `rimport` over a tree it has already published. - Enumeration never follows a directory symlink either, so it cannot loop on a link that points at its own ancestor, and cannot wander outside the directory you named and publish files you did not ask for. The rule for a name you give matches the rule used while walking, so a path behaves the same whichever way `rimport` reaches it. A file found by enumeration that cannot be staged does not abort the run. It is reported, skipped, and repeated in a summary at the end so it does not scroll away. A bad name you gave directly is still fatal, and nothing is published. -Exit codes: +## Exit codes | Code | Meaning | |------|---------| | 0 | Everything staged or checked, nothing skipped | | 1 | A file could not be staged | | 2 | A name you gave failed validation; nothing was published | -| 3 | Finished, but one or more files were skipped (listed at the end) | +| 3 | Finished, but one or more items were skipped (listed at the end) | When more than one code applies, the precedence is 2 > 1 > 3 > 0. +A name you gave failing is fatal (2). An invalid file found by expanding a directory you named is skipped instead, and the run continues (3). + ## Filenames and metadata: There is a good description of metadata that should be included in inputdata files here: https://www.cesm.ucar.edu/models/cam/metadata From d1d89922d56e3740f55882739c04b5199b2a1358 Mon Sep 17 00:00:00 2001 From: Sam Rabin Date: Wed, 9 Sep 2026 19:14:36 -0600 Subject: [PATCH 47/47] Pin reason_text, and say "anything" where a directory can be skipped reason_text changed the text of every skip and every pre-flight failure line, and nothing asserted the new form: reverting it to str() left the suite green. The walk-skip test now pins that the reason names the error without repeating the path the line already carries. The --check twin for exit 1 mocks stage_data, so --check was swallowed by the mock and the test passed identically without the flag. It now asserts the flag reaches stage_data, which is the only place it changes anything. The exit-3 table says "item(s)" because a directory can be the thing skipped, but every sentence explaining exit 3 still said "file" -- including one added two lines under that table. They now say "anything found by expanding a directory", and name the unreadable-subdirectory case that motivated it. reason_text's docstring claimed more than it delivers: the RuntimeErrors from validate_source_path carry no strerror and several write the path into their own text, so those lines are unchanged. Restores, in the README, what naming a symlink to a directory actually does, which was lost with the bullet that explained it by history. Suite: 345 passed. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 8 ++++---- rimport | 13 +++++++++---- tests/rimport/test_cmdline.py | 2 +- tests/rimport/test_expand_directories.py | 18 +++++++++++------- tests/rimport/test_main.py | 12 +++++++++--- 5 files changed, 34 insertions(+), 19 deletions(-) diff --git a/README.md b/README.md index f1bbb8f..ec69a23 100644 --- a/README.md +++ b/README.md @@ -12,19 +12,19 @@ Notes: - 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 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 path you named (all `--file`/`--list`/positional entries together). If any of them fail — missing, 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. An invalid file found by expanding a directory you named is not covered by this promise; it is skipped on its own, and the rest of the run continues (see "Directory arguments" below). 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). +- Before staging anything, `rimport` validates every path you named (all `--file`/`--list`/positional entries together). If any of them fail — missing, 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. Anything found by expanding a directory you named — an invalid file, or a subdirectory that cannot be read — is not covered by this promise; it is skipped on its own, and the rest of the run continues (see "Directory arguments" below). 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 path you named 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. ### Directory arguments -Any name you give `rimport` — positional, `--file`, or a `--list` entry — may be a directory inside the inputdata tree. Every file beneath it is enumerated recursively and acted on. A directory outside the tree is rejected without being enumerated, exactly as a file outside it is. The directory itself is never copied to staging or replaced with a symlink. A symlink to a directory is the one carve-out: it is not expanded, and is treated as a single entry. +Any name you give `rimport` — positional, `--file`, or a `--list` entry — may be a directory inside the inputdata tree. Every file beneath it is enumerated recursively and acted on. A directory outside the tree is rejected without being enumerated, exactly as a file outside it is. The directory itself is never copied to staging or replaced with a symlink. A symlink to a directory is the one carve-out: it is not expanded, but treated as a single entry — reported as already published if it points into the staging directory, and an error otherwise. Why symlinks to directories are left alone, rather than enumerated: - A symlink into staging is what `rimport` itself creates, so a symlink here is usually a published file rather than a detour to follow. Treating it as a single entry is what lets you re-run `rimport` over a tree it has already published. - Enumeration never follows a directory symlink either, so it cannot loop on a link that points at its own ancestor, and cannot wander outside the directory you named and publish files you did not ask for. The rule for a name you give matches the rule used while walking, so a path behaves the same whichever way `rimport` reaches it. -A file found by enumeration that cannot be staged does not abort the run. It is reported, skipped, and repeated in a summary at the end so it does not scroll away. A bad name you gave directly is still fatal, and nothing is published. +Anything found by enumeration that cannot be staged does not abort the run — an unstageable file, or a subdirectory that cannot be read. It is reported, skipped, and repeated in a summary at the end so it does not scroll away. A bad name you gave directly is still fatal, and nothing is published. ## Exit codes @@ -37,7 +37,7 @@ A file found by enumeration that cannot be staged does not abort the run. It is When more than one code applies, the precedence is 2 > 1 > 3 > 0. -A name you gave failing is fatal (2). An invalid file found by expanding a directory you named is skipped instead, and the run continues (3). +A name you gave failing is fatal (2). Anything found by expanding a directory you named is skipped instead, and the run continues (3). ## Filenames and metadata: diff --git a/rimport b/rimport index ff39843..29fb9e4 100755 --- a/rimport +++ b/rimport @@ -66,8 +66,9 @@ def build_parser() -> argparse.ArgumentParser: " --list file, or a name you gave failing validation\n" " 3: finished, but one or more items were skipped (listed at the end)\n" "\n" - "a name you gave failing is fatal (2). a file found by expanding a\n" - "directory you named is skipped instead, and the run continues (3).\n" + "a name you gave failing is fatal (2). anything found by expanding a\n" + "directory you named -- a bad file, or a subdirectory that cannot be\n" + "read -- is skipped instead, and the run continues (3).\n" "when several apply, the precedence is 2 > 1 > 3 > 0.\n" ), formatter_class=_EpilogRawFormatter, @@ -124,7 +125,7 @@ def build_parser() -> argparse.ArgumentParser: help=( "Check whether item(s) is/are already published, without staging anything. A bad" " name that you gave aborts before any file is checked, reporting all bad names" - " at once; a file found by enumerating a directory is instead reported and" + " at once; anything found by enumerating a directory is instead reported and" " skipped individually." ), ) @@ -395,7 +396,11 @@ def reason_text(reason: Exception) -> str: `OSError`'s str() appends the filename, so a line built as "'': " prints the path twice. Its `strerror` is the same message without that, and errno is dropped - deliberately: "Permission denied" is what the user can act on. + deliberately: "Permission denied" is what the user can act on. This is safe only because + every OSError reaching here was raised on the same path the message names. + + It changes nothing for the RuntimeErrors `validate_source_path` builds: those carry no + `strerror`, and several write the path into their own text, which this cannot undo. """ if isinstance(reason, OSError) and reason.strerror: return reason.strerror diff --git a/tests/rimport/test_cmdline.py b/tests/rimport/test_cmdline.py index 5fb1d6f..8b3bac9 100644 --- a/tests/rimport/test_cmdline.py +++ b/tests/rimport/test_cmdline.py @@ -1383,5 +1383,5 @@ def test_expansion_count_is_logged_before_staging( assert result.returncode == 0 assert ( result.stdout.index("expanded 1 director(ies) to 2 file(s)") - < result.stdout.index("staged") + < result.stdout.index("[rimport] staged") ) diff --git a/tests/rimport/test_expand_directories.py b/tests/rimport/test_expand_directories.py index 4f4a98c..7bb0029 100644 --- a/tests/rimport/test_expand_directories.py +++ b/tests/rimport/test_expand_directories.py @@ -89,10 +89,10 @@ def test_duplicates_collapse_and_named_wins(tmp_path): assert entries == [rimport.Entry(inner, True)] -def test_named_wins_over_discovered_whichever_comes_first(tmp_path): - """Naming the file before the directory that contains it must reach the same verdict. - The two orders take different routes -- one sets `named` and the walk must not clear it, - the other must upgrade an entry the walk already recorded.""" +def test_named_wins_when_the_file_is_named_before_its_directory(tmp_path): + """Naming the file before the directory that contains it reaches the same verdict as + naming it after (the test above). The two orders take different routes: this one sets + `named` first and the walk must not clear it.""" d = tmp_path / "d" d.mkdir() inner = d / "inner.nc" @@ -196,9 +196,8 @@ def test_unreadable_directory_does_not_warn_that_it_is_empty(tmp_path, caplog): def test_discovered_walk_skip_is_warned_where_it_happened(tmp_path, caplog): - """The spec promises every skip is reported twice. The end-of-run summary is the second - report; this is the first, and without it a skip during a fatal abort is reported zero - times.""" + """Every skip is reported twice: here, as the run reaches it, and again in the end-of-run + summary. Without this first report a skip during a fatal abort is reported zero times.""" d = tmp_path / "d" d.mkdir() (d / "a.nc").write_text("a") @@ -213,6 +212,11 @@ def test_discovered_walk_skip_is_warned_where_it_happened(tmp_path, caplog): os.chmod(locked, 0o700) assert f"skipping '{locked}'" in caplog.text + # The line already names the path, so the reason must not repeat it. OSError's str() + # appends the filename and prefixes the errno; strerror is the part worth reading. + assert "Permission denied" in caplog.text + assert "[Errno" not in caplog.text + assert caplog.text.count(str(locked)) == 1 def test_named_unreadable_directory_is_not_warned_as_skipped(tmp_path, caplog): diff --git a/tests/rimport/test_main.py b/tests/rimport/test_main.py index dd954c0..16178e6 100644 --- a/tests/rimport/test_main.py +++ b/tests/rimport/test_main.py @@ -741,8 +741,9 @@ def test_staging_error_outranks_skip_in_exit_code( def test_check_error_also_outranks_skip_in_exit_code( self, _mock_ensure_running_as, mock_get_staging_root, tmp_path ): - """Precedence 1 > 3 holds under --check: a file that fails while being checked is a - real failure, not a skip, even though nothing was being written.""" + """Precedence 1 > 3 holds under --check: an item that fails while being checked is a + real failure, not a skip, even though nothing was being written. Also pins that the + flag reaches stage_data, which is the only place --check changes what happens.""" inputdata_root = tmp_path / "inputdata" subdir = inputdata_root / "lnd" subdir.mkdir(parents=True) @@ -752,12 +753,17 @@ def test_check_error_also_outranks_skip_in_exit_code( (subdir / "good.nc").write_text("good") (subdir / "broken.nc").symlink_to(inputdata_root / "nonexistent.nc") - with patch.object(rimport, "stage_data", side_effect=RuntimeError("boom")): + with patch.object( + rimport, "stage_data", side_effect=RuntimeError("boom") + ) as mock_stage_data: result = rimport.main( ["-inputdata", str(inputdata_root), str(subdir), "--check"] ) assert result == 1 + mock_stage_data.assert_called_once_with( + subdir / "good.nc", inputdata_root, staging_root, True + ) @patch.object(rimport, "get_staging_root") @patch.object(rimport, "ensure_running_as")