Handle relpaths when rimport called from inputdata subdir - #28
Open
samsrabin wants to merge 16 commits into
Open
Handle relpaths when rimport called from inputdata subdir#28samsrabin wants to merge 16 commits into
samsrabin wants to merge 16 commits into
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 3490 +553
==========================================
+ Hits 2912 3464 +552
- 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>
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.