relink.py raises an unhandled traceback when --inputdata-root contains a symlinked component
- Dominant language
- Python
- Stars
- 0
- Forks
- 2
- Avg merge
- 3d 2h
- Merged PRs (30d)
- 2
Description
(Authored by Claude)
### Summary
`relink.py` mixes **lexical** and **physical** path handling. When
`--inputdata-root` is given as a path with a symlinked component, the
"item is under inputdata root" check fails for items that genuinely
*are* under the root, and the resulting `ArgumentTypeError` escapes as a
traceback instead of a clean error message.
### Reproduction
```bash
# real tree + a symlink to it
mkdir -p /tmp/demo/real_root/sub /tmp/demo/target/sub
echo data > /tmp/demo/real_root/sub/f.txt
echo data > /tmp/demo/target/sub/f.txt
ln -s /tmp/demo/real_root /tmp/demo/link_root
cd /tmp/demo/link_root/sub
relink.py f.txt --target-root /tmp/demo/target --inputdata-root /tmp/demo/link_root
```
Result:
```
Traceback (most recent call last):
...
argparse.ArgumentTypeError: Item '/tmp/demo/real_root/sub/f.txt' not under
inputdata root '/tmp/demo/link_root'
```
Expected: the item is under the inputdata root (via the symlink), so the
relink should succeed — and in any case a user error should never be a
traceback.
### Cause
Two path conventions meet and disagree:
- `shared.validate_paths` (`shared.py:159`) returns `os.path.abspath(path)`.
That is lexical, but it is built on `os.getcwd()`, which is **physical** —
the kernel does not retain the logical route you took through a symlink.
So a relative positional becomes a *physical* absolute path.
- `process_args` (`relink.py:380`) then does
`Path(item).is_relative_to(args.inputdata_root)` — a purely **lexical**
comparison against whatever string the user typed for `--inputdata-root`.
Physical item vs. logical root ⇒ the comparison fails.
The same lexical comparison appears at `relink.py:387` for the
"`target_root` must NOT be under `inputdata_root`" check, which means that
guard can also be bypassed by passing a symlinked path.
Because the `ArgumentTypeError` is raised from `process_args` rather than
from an argparse `type=` callable, argparse never catches it, so it
surfaces as a traceback rather than a usage error.
### Why the test suite doesn't catch this
`tests/relink/` passes today only because `TMPDIR` on the machine where it
runs contains no symlink components. The tests are green for an
environmental reason, not because the code is correct. On GLADE, symlinked
paths are entirely plausible.
Note the asymmetry: `rimport` handles this case correctly — it resolves
both the inputdata root and the cwd before comparing them — so the two
tools currently disagree about what "under the inputdata root" means.
### Suggested fix
Compare **resolved** paths on both sides at `relink.py:380` and
`relink.py:387`.
**Trap to avoid:** do *not* simply call `Path(item).resolve()`. `relink.py`
exists to replace files with symlinks into the target root, so an item may
already *be* a symlink pointing outside the inputdata tree. Fully resolving
it would follow that leaf symlink and make a legitimately in-tree item look
out-of-tree — turning the fix into a new bug.
Resolve the parent, keep the leaf name:
```python
item_path = Path(item)
item_resolved = item_path.parent.resolve() / item_path.name
```
That canonicalizes every directory component (the actual problem) while
preserving the leaf symlink (which relink must not follow). For
`target_root` and `inputdata_root` — directories, not files — a plain
`.resolve()` is correct.
### Test coverage the fix needs
1. Symlinked inputdata root with an in-tree item — currently raises, should
be accepted.
2. An item genuinely outside the root — must **still** be rejected, so the
fix doesn't silently disable the guard.
3. An in-tree item that is already a symlink pointing *out* of the tree —
must still be accepted (the trap above).
4. A `target_root` under the inputdata root via a symlinked path — must
still be rejected.
5. End-to-end via subprocess from inside a symlinked root, asserting rc 0,
a correct symlink, and **no traceback in stderr**.
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.