Handle relpaths when rimport called from inputdata subdir - #28
Merged
Conversation
Fixes the headline bug: rimport always resolved relative --file / positional filenames against the inputdata root, ignoring cwd. Running `rimport test.nc` from an inputdata subdirectory would silently stage a same-named file from the root instead (or fail to find the file that's actually there). get_files_to_process() now takes a required inputdata_root parameter and eagerly anchors non-absolute file/items_to_process names to the resolved cwd whenever cwd is inside the inputdata tree (the root itself counts), with no fallback to the root on a miss. When cwd is outside the tree, names are left unchanged for normalize_paths to resolve against the root, exactly as before. normalize_paths and --list entry handling are untouched (list-relative anchoring is a separate task). The 4 existing unit tests exercising relative CLI-arg names now monkeypatch.chdir() to a directory genuinely outside their tmp_path-based inputdata root, since "cwd outside the tree" is now the condition their existing expectations depend on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Per-entry anchoring for --list (--filelist) entries: a relative entry resolves against the list file's own resolved directory (root itself counts as inside the tree), not the cwd or unconditionally the inputdata root. A relative entry in a list file whose directory is outside the inputdata tree is now a fatal error (rc 2), naming both the offending entry and the list file. This is an intentional breaking change: filelists outside the tree with relative entries previously resolved against the root and now error. Updated the tests that encoded the old contract accordingly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…subdir relink.py already resolves relative positionals (a bare filename and ".") against the caller's cwd rather than the inputdata root, via shared.validate_paths -> os.path.abspath. Nothing pinned that behavior, so add characterization tests that run relink.py from inside a nested inputdata subdirectory and confirm the resulting symlink points at the matching target file.
…README rimport's --file/--list/positional help strings, the normalize_paths and main docstrings, and the README Notes section still described the old "everything is relative to the inputdata root" behavior. Update them to match what get_files_to_process actually does now: CLI-arg names anchor to cwd inside the inputdata tree with no fallback to the root, --list entries anchor to the list file's own directory, and a relative --list entry outside the tree is a fatal (rc 2) error. Flag the list-file case as a breaking change in the README. No logic changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
stage_data checked that the source exists but never that it was a regular file. When run from inside an inputdata subdir on a directory whose staging mirror already exists, control fell into the already-published relink branch and called replace_one_file_with_symlink, which renamed the directory to '<name>.tmp', symlinked over it, then failed to roll back (ENOTDIR), leaving the tree mangled. An empty-string argument (e.g. an unset shell variable) hit the same path against the inputdata root itself. Add a guard right after the existing existence check: raise RuntimeError if the source is a directory (or a symlink to one). Since the guard sits before the check/no-check branches, it also closes the --check path, which previously reported a directory as "already published ... available for download".
Review found that the guard's docstring claimed RuntimeError covers "a directory (or a symlink to one)". False: the is_symlink() branch above the guard returns early (rc 0, "already published and linked") for a live symlink whose target is a directory, without ever reaching the new guard. That state is reachable in practice: it is exactly the tree left behind by the pre-fix bug this task closed, so a user cleaning up after that incident would hit it. Correct the docstring to say only what the code does; no logic change.
Add the two missing semantic tests for --list resolution flagged by the final branch review: every existing list test runs with cwd outside the inputdata tree, so cwd-anchoring and list-dir-anchoring produce the same answer and can't be told apart. Add a unit test (test_get_files_to_process.py) and an e2e counterpart (test_cmdline.py) that put the cwd inside the tree at a location different from the list file, with a decoy file at the cwd-anchored path, to make the discrimination concrete. Also add the list-side twin of test_dotdot_escape_from_subdir_errors: an in-tree list file with a '..'-escaping entry, pinning the already-correct behavior (rc 1, "not under inputdata root") that was previously unpinned. No production-code change; test-only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
get_files_to_process() called Path.cwd() unconditionally to decide whether relative CLI-arg names should anchor to cwd. If the process's cwd has been deleted out from under it, Path.cwd() raises FileNotFoundError (a subclass of OSError), so rimport died with an unhandled traceback even when every argument was absolute and cwd was irrelevant to the operation. Before the branch that introduced this call, that case returned rc 0. Catch OSError around the cwd lookup and treat "cwd can't be determined" the same as "cwd is outside the tree": fall back to the pre-existing root-relative behavior instead of propagating the exception. cwd is only ever dereferenced when cwd_inside is True, and cwd_inside is now always False in the except branch, so cwd=None is never used unguarded. Also warn (once) when this fallback actually changes behavior: if any of the CLI-supplied file/positional names are relative, log that cwd couldn't be determined and those names will resolve against the inputdata root instead. The common absolute-path case stays silent, since cwd genuinely doesn't matter there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
--help for --file and the positional items now states explicitly that
there is no fallback to the inputdata root when a relative name isn't
found under cwd, so the strict resolution rule doesn't have to be
inferred from the README. The out-of-tree list-file error now appends
the remedy (absolute paths, or move the list file into the tree) so a
user hitting it doesn't need to go read the README.
Also corrects two docstring passages in stage_data() that overstated
when a directory-target symlink returns without raising: that only
holds when the target is under staging_root, not when it's outside
(which raises via the existing "outside staging" guardrail). Fixed an
adjacent one-word inaccuracy ("file" -> "target") in the same
Guardrails block, since the underlying check never inspects target
type. And documents get_files_to_process()'s deleted-cwd fallback
(added in 17cf275) and the condition under which its warning fires.
Text-only change; no logic touched.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The single-file sibling of test_command_line_relative_dir_dot_from_inputdata_subdir never got the decoy treatment its neighbor received after review. Add a same-named decoy file at the inputdata root (distinct content, matching decoy target) so the test discriminates cwd-relative resolution from a root-relative regression by file content, not merely by whether a symlink exists. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…p headers
Three small, independent test-hardening edits deferred as Minor findings
from the Task I and J reviews, batched into one commit:
- Assert on a distinctive fragment of the list-error remedy clause
("use absolute paths or move the list file") in
test_list_outside_tree_relative_entry_errors, so deleting the remedy
text from rimport's error message would be caught (previously only the
two dynamic values were asserted).
- Wrap the deleted-cwd tests' calls in caplog.at_level(logging.WARNING),
matching the idiom used in tests/relink/test_verbosity.py and
test_timing.py, so they no longer rely implicitly on
shared.get_log_level never exceeding WARNING.
- Add the missing "# Setup" comment header to those same two tests to
match the Setup/Run/Verify triad used elsewhere in this file.
No production code changed.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #28 +/- ##
==========================================
+ Coverage 99.14% 99.25% +0.10%
==========================================
Files 28 29 +1
Lines 2937 3493 +556
==========================================
+ Hits 2912 3467 +555
- Misses 25 26 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Task L review found the "# Setup" headers added to the two deleted-cwd tests matched the wrong neighbouring convention: the 8 tests directly above them (test_single_or_filelist_or_list_required through test_cli_cwd_inside_tree_via_symlink, lines 497-622) all have real setup code followed straight by "# Run"/"# Verify", with no "# Setup" header. Adding the header made the two tests match a non-adjacent earlier block instead of their true nearest neighbours, creating a new inconsistency. Revert the two "# Setup" additions; items 1 and 2 (the remedy-text assertion and the caplog.at_level(logging.WARNING) wraps) are untouched.
…llback dep Eight e2e tests in test_cmdline.py passed a bare relative filename to rimport without cwd= on subprocess.run, so they only passed via rimport's legacy "resolve a relative name against the inputdata root" fallback rather than any real cwd relationship. Swap each bare name for an absolute path -- the str() of a Path already in scope (test_file, nested_file, src), or inputdata_root / "nonexistent.nc" for the negative test -- so these staging-mechanics tests no longer ride on a fallback that's about to be removed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Replace the two-mode design in get_files_to_process with a single rule: a relative --file/positional name always anchors to cwd, a relative --list entry always anchors to the list file's own directory, and absolute paths are never touched. There is no root-join anywhere any more. The fallback was the bug it was meant to accommodate. When a subdirectory of the inputdata tree is a symlink, os.getcwd() returns the physical path, the "is cwd inside the tree" test fails, and the fallback silently published a same-named file from the root instead -- the wrong file, with exit code 0. Root-relative CLI paths were never an intended workflow, so nothing supported is lost; files under a symlinked-out subdirectory were already unpublishable, because stage_data resolves the source and rejects anything outside the root. Also drop the now-dead inputdata_root parameter (main is the only production caller), and make an undeterminable cwd fatal for relative names -- with no fallback there is nothing to anchor against -- reporting every offending name in one message rather than warning and guessing. Tests: delete three unit tests and one e2e test that existed only to pin the removed mode; flip five unit tests that asserted relative names come back unanchored; replace the deleted outside-the-tree e2e test with one that pins the new behavior, keeping a same-named decoy at the root so it fails if the fallback ever returns. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…hing main() previously looped over the resolved paths calling stage_data, counting failures and continuing, so a batch containing one typo'd filename published some files and then exited 1. Validate every path first instead: if any fail, report all of them and exit 2 without touching the tree. Extract stage_data's read-only guardrails into validate_source_path(), which returns the exception unraised rather than raising it. stage_data raises whatever comes back, so its exception types and message text are unchanged (test_stage_data.py is untouched); main's gate calls the same function, so the two can't drift apart about what is stageable. A live symlink whose target resolves under staging_root validates as OK, not as a failure: that is the normal state of an already-published file, and treating it as bad would break re-running rimport over a published tree. This settles the exit-code split: 2 means we rejected the input before doing anything, 1 means a genuine runtime failure after work began. Ten e2e tests shift from 1 to 2 as a result; only one pinned the exact code. The gate deliberately covers --check too, so one bad entry aborts the batch rather than being reported per-file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tion
The help strings, README notes, and docstrings still described the two-mode
resolution design that this branch deleted, and said nothing about pre-flight
validation. Bring them in line with what the code now does.
The README's advice was wrong rather than merely stale: it told users to "run from
outside the tree, or pass an absolute path, if you want the old root-relative
resolution." Running from outside the tree now anchors to that outside directory,
so following that advice does not do what it promises -- it fails cleanly with rc 2
("source not found", or "source not under inputdata root" if a same-named file
happens to sit at the cwd-anchored path). The README also carried a "Breaking
change" warning about relative entries in an out-of-tree list file erroring; they
no longer error.
State the pre-flight guarantee with its limit intact: if pre-flight passes, no
file fails for a reason pre-flight could have detected, and if it fails nothing
was touched -- but it is not a promise that a clean batch finishes, since a
runtime failure can still partially complete one.
normalize_paths' docstring claimed callers pre-anchor only "inside-tree" names.
Every clause of that is now false: get_files_to_process anchors every relative
name, so nothing relative reaches normalize_paths on main's call path and its
root-join is unreachable there. Say so plainly, and say that the branch is kept
deliberately, so nobody reading the function alone concludes root-relative
resolution survives somewhere.
No logic changes; the suite is unmoved at 287.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
samsrabin
commented
Aug 31, 2026
Finishes the review-requested move of fixture_nested_mock_dirs from tests/relink/test_cmdline.py into tests/conftest.py (moved verbatim by the repo owner) by matching its sibling fixture_temp_dirs in shape: explicit scope="function", yield instead of return, and a docstring matching the sibling's voice. Deliberately does NOT add DEFAULT_INPUTDATA_ROOT/DEFAULT_STAGING_ROOT patching (every user runs relink.py as a subprocess, where in-process patching is inert and could mask a dropped --inputdata-root/--target-root flag falling back to the live production tree), and deliberately omits an explicit shutil.rmtree cleanup since everything is built under tmp_path, which pytest already cleans up automatically. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…iffers Every decoy in the branch-added tests (relink's single-file and "." e2e tests, plus the four rimport --list/positional-argument no-root-fallback tests and their test_get_files_to_process.py in-process twin) previously wrote its basename as a second, independent literal -- sometimes matching the real file's basename by coincidence, sometimes (the relink "." test) deliberately different, per review-comment feedback asking why. The repo owner's ruling was to make them consistent: every decoy is same-named with the file it shadows, and that name must be derived from the real file's basename rather than copy-pasted, so the two can never quietly drift apart and stop discriminating. Where a basename was also duplicated into a CLI command argument, bound it to the same variable used for the path. Also added an explicit assertion at each decoy's creation time that its content differs from the real file's -- documented at the point of setup rather than left for the reader to notice two different string literals. Skipped where there is no real file to differ from (the two no-root-fallback error tests). Verified empirically, for every touched test, that the decoy assertion can still be made to fail: reproduced each scenario against the real production scripts (unmodified) plus scratch-only patched copies that simulate the specific regression each test guards against (root-relative resolution for relink; cwd-anchoring of --list entries; reintroduced root-fallback for CLI/positional names), confirming the decoy gets wrongly touched under the regression and the assertion flips to failing. This included re-confirming the relink "." test's newly same-named decoy still discriminates by location, as the owner's ruling argued it would. No production code changed. 287 tests still pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
It previously discriminated only by absence: a --list entry anchored to the inputdata root instead of the list file's own directory hits a nonexistent path, so the run just fails. That's weaker than it looks -- if a file happened to already exist at the root-anchored path, a root-relative regression would silently succeed by publishing the wrong file, and the test would pass. Add a same-named decoy (basename derived from the real file's, per the convention Task R just established) at the root-anchored path inputdata_root/clm2/file1.nc, with content asserted to differ from the real file's at creation time. Assert it stays untouched (not a symlink, original content intact) and that nothing lands in staging_root at the root-anchored path, alongside the existing checks that the real file (under lnd/clm2/) is what actually got staged and relinked. No cwd decoy is added: this test passes no cwd= to subprocess.run, so the subprocess inherits pytest's own outside-the-tree cwd, and a cwd-anchoring regression would resolve outside the inputdata root and fail loudly -- nowhere useful to plant one. The sibling test (test_list_inside_tree_relative_entries_anchor_to_list_dir_not_cwd) already covers that case. Verified empirically that the decoy assertion is not dead coverage: against a scratch-only patched copy of rimport (list entries anchored to the inputdata root instead of the list file's directory -- the tracked rimport was never touched), the run succeeds, the decoy gets wrongly staged and relinked, and all three added decoy assertions flip to failing; a pytest run with the decoy checks temporarily reordered ahead of the pre-existing ones (scratch copy only) fails directly on the new `assert not decoy_file.is_symlink()` line. Scratch dir removed afterward. No production code changed. 287 tests still pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Extract a shared helper for each pair (relink's filename-vs-"." relative resolution test, rimport's mixed-validity list vs --check test) while keeping each test's own docstring and payoff assertions intact, per the PR review comments asking to reduce duplication without losing what each test discriminates. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Comments and docstrings across the branch referenced the development process rather than the code: "the deleted root-fallback", "a later task", "see the brief", "as it did before the guard", "previously had no end-to-end coverage". None of that means anything to someone reading these files without knowing this PR existed. Replace each with the rationale it was standing in for, or cut it where the surrounding sentence already carried the useful fact. Kept the passages that explain why the code is shaped the way it is rather than how it got there -- the "file must exist, or the source-not-found check fires before the guardrail" comments, and normalize_paths' note that its root-join is unreachable and deliberately-kept dead code, without which a reader would delete the branch or conclude root-relative resolution is still supported. Also corrects two inaccuracies rather than only trimming: a vague reference to "the existing e2e list test" now names it, and _run_mixed_validity_list's docstring no longer claims its hoisted rc-2 assertions are something "neither test discriminates on" -- they do guard against the pre-flight gate not firing at all, they just aren't what tells the two tests apart. Fixes the script name in that same docstring: it is rimport, not rimport.py. Text only; no executable line changed and the suite is unmoved at 287. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #27.