From 76f4fee0549eda7ae2ad750ae80c3a9b448c6425 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 29 Sep 2026 09:11:36 +0000 Subject: [PATCH 1/3] P0-8: resolve sh:// command paths only for files present in launch dir and absent from case dir Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01RhG7KcNHxMhoJrsZJFtC4Y --- NEWS.md | 15 +++++ fz/runners/sh.py | 43 +++++++++++--- tests/test_p0_8_sh_path_resolution.py | 86 +++++++++++++++++++++++++++ 3 files changed, 137 insertions(+), 7 deletions(-) create mode 100644 tests/test_p0_8_sh_path_resolution.py diff --git a/NEWS.md b/NEWS.md index f470fa0..cfdc51c 100644 --- a/NEWS.md +++ b/NEWS.md @@ -2,6 +2,21 @@ ## Unreleased +### Fix: potentially wrong results with `sh://` commands (P0-8) + +- Path resolution in `sh://` commands converted every word that looked like a + file name to an absolute path in the launch directory without checking that + it exists. With `sh://cat in.txt > out.txt`, the calculation read the + **un-substituted** template from the launch directory instead of the case's + compiled file, and wrote `out.txt` outside the case directory (shared across + parallel cases), with no error. A word is now resolved only if it exists in + the launch directory and does not exist in the case directory; targets of + `>`/`>>` redirections are never resolved (stricter than "unless existing": + writing into the launch directory is the defect). Each resolved word is + logged at info level. Scripts located only in the launch directory + (`sh://bash script.sh`) still resolve. Results obtained earlier with + commands referencing input or output files by bare name should be re-checked. + ### Breaking changes - **Dropped Python 3.8 support** (P0-5). `requires-python` is now `>=3.9` and diff --git a/fz/runners/sh.py b/fz/runners/sh.py index 7fada79..01dea57 100644 --- a/fz/runners/sh.py +++ b/fz/runners/sh.py @@ -15,7 +15,9 @@ from .errors import classify_error -def resolve_all_paths_in_command(command: str, original_cwd: str) -> tuple[str, bool]: +def resolve_all_paths_in_command( + command: str, original_cwd: str, working_dir: str = None +) -> tuple[str, bool]: """ Resolve ALL file paths in a shell command to absolute paths @@ -30,6 +32,10 @@ def resolve_all_paths_in_command(command: str, original_cwd: str) -> tuple[str, Args: command: Original shell command original_cwd: Original working directory for resolving relative paths + working_dir: Case working directory. A word is resolved only if it exists + in ``original_cwd`` and does not exist in ``working_dir`` (compiled + input files take precedence). Output redirection targets are never + resolved. If None, only the existence check in ``original_cwd`` applies. Returns: Tuple of (resolved_command, was_changed) where: @@ -62,7 +68,7 @@ def resolve_all_paths_in_command(command: str, original_cwd: str) -> tuple[str, # Process this command segment resolved_segment, segment_changed = _resolve_paths_in_segment( - segment, original_cwd + segment, original_cwd, working_dir ) resolved_segments.append(resolved_segment) @@ -83,10 +89,15 @@ def resolve_all_paths_in_command(command: str, original_cwd: str) -> tuple[str, return command, False -def _resolve_paths_in_segment(segment: str, original_cwd: str) -> tuple[str, bool]: +def _resolve_paths_in_segment( + segment: str, original_cwd: str, working_dir: str = None +) -> tuple[str, bool]: """ - Resolve ALL relative paths in a command segment to absolute paths. - Simple approach: convert any token that looks like a file path to absolute. + Resolve relative paths in a command segment to absolute paths. + + A token that looks like a file path is converted only if it exists in + ``original_cwd`` and not in ``working_dir``; the target of ``>``/``>>`` + redirections is never converted. """ import shlex import re @@ -103,8 +114,15 @@ def _resolve_paths_in_segment(segment: str, original_cwd: str) -> tuple[str, boo resolved_parts = [] was_changed = False + prev_part = "" for part in command_parts: + is_output_target = prev_part.lstrip("0123456789&") in (">", ">>", ">|") + prev_part = part + if is_output_target: + resolved_parts.append(part) + continue + # Skip if already absolute path if os.path.isabs(part): resolved_parts.append(part) @@ -210,9 +228,20 @@ def _resolve_paths_in_segment(segment: str, original_cwd: str) -> tuple[str, boo should_resolve = True if should_resolve: - # Convert to absolute path abs_path = os.path.abspath(os.path.join(original_cwd, part)) + # Resolve only files really present in the launch directory and + # absent from the case directory (compiled inputs take precedence) + if not os.path.exists(abs_path): + resolved_parts.append(part) + continue + if working_dir is not None and os.path.exists( + os.path.join(str(working_dir), part) + ): + resolved_parts.append(part) + continue + log_info(f"Info: sh:// word resolved: {part} -> {abs_path}") + # On Windows, convert path to forward slashes for bash compatibility # MSYS2/Git Bash/WSL all expect Unix-style paths if os.name == 'nt': @@ -294,7 +323,7 @@ def run_local_calculation( # Construct command - resolve ALL file paths to absolute for reliable parallel execution if command: resolved_command, was_changed = resolve_all_paths_in_command( - command.replace("\\","/"), original_cwd + command.replace("\\","/"), original_cwd, "." # cwd is the case dir after chdir ) # Apply shell path resolution to command if FZ_SHELL_PATH is set diff --git a/tests/test_p0_8_sh_path_resolution.py b/tests/test_p0_8_sh_path_resolution.py new file mode 100644 index 0000000..8661e33 --- /dev/null +++ b/tests/test_p0_8_sh_path_resolution.py @@ -0,0 +1,86 @@ +"""P0-8 regression tests: path resolution in sh:// commands. + +Only words that exist in the launch directory and not in the case directory +are resolved; output redirection targets are never resolved. +""" + +import os +from pathlib import Path + +import fz +from fz.runners.sh import resolve_all_paths_in_command + + +def test_resolver_skips_nonexistent_and_case_files(tmp_path): + launch = tmp_path / "launch" + case = tmp_path / "case" + launch.mkdir() + case.mkdir() + (launch / "in.txt").write_text("template") + (launch / "tool.sh").write_text("echo tool") + (case / "in.txt").write_text("compiled") + + cmd, _ = resolve_all_paths_in_command( + "bash tool.sh in.txt > out.txt", str(launch), str(case) + ) + assert f"{launch}/tool.sh" in cmd # only in launch dir: resolved + assert f"{launch}/in.txt" not in cmd # in case dir: compiled file wins + assert f"{launch}/out.txt" not in cmd # redirection target untouched + assert cmd.endswith("> out.txt") + + +def test_resolver_never_resolves_output_redirect_even_if_exists(tmp_path): + launch = tmp_path / "launch" + case = tmp_path / "case" + launch.mkdir() + case.mkdir() + (launch / "out.txt").write_text("x") + for op in (">", ">>", "2>"): + cmd, _ = resolve_all_paths_in_command( + f"echo hi {op} out.txt", str(launch), str(case) + ) + assert str(launch) not in cmd + + +def test_resolver_nonexistent_word_kept(tmp_path): + cmd, _ = resolve_all_paths_in_command("bash ghost.sh ./x", str(tmp_path), None) + assert str(tmp_path) not in cmd + + +def test_fzr_sh_cat_redirect_two_cases(tmp_path): + model_dir = tmp_path + (model_dir / "in.txt").write_text("x=${x}\n") + inp = model_dir / "in.txt" + launch = Path.cwd() + + res = fz.fzr( + str(inp), + {"x": [1, 2]}, + {"varprefix": "$", "delim": "{}", "output": {"y": "cat res.txt"}}, + calculators="sh://cat in.txt > res.txt", + results_dir=str(tmp_path / "res"), + ) + # fz appends the input file name to the command, so cat prints it twice + assert [v.splitlines()[0] for v in res["y"].astype(str)] == ["x=1", "x=2"] + case_dirs = sorted(p for p in (tmp_path / "res").iterdir() if p.is_dir()) + assert len(case_dirs) == 2 + for d in case_dirs: + assert (d / "res.txt").read_text().startswith("x=") + assert not (launch / "res.txt").exists() + + +def test_fzr_sh_script_only_in_launch_dir(tmp_path): + (tmp_path / "in.txt").write_text("v=${x}\n") + script = Path.cwd() / "myscript.sh" + script.write_text("#!/bin/bash\ncat in.txt > out.dat\n") + try: + res = fz.fzr( + str(tmp_path / "in.txt"), + {"x": [3]}, + {"varprefix": "$", "delim": "{}", "output": {"y": "cat out.dat"}}, + calculators="sh://bash myscript.sh", + results_dir=str(tmp_path / "res"), + ) + assert str(res["y"].iloc[0]).strip() == "v=3" + finally: + script.unlink() From 3240090aadb6c0509ae9cfe29241b3fc5f4b316c Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 29 Sep 2026 09:22:11 +0000 Subject: [PATCH 2/3] P0-8: drop Windows 'failed' expectation for tar case (was a symptom of the old resolver) Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01RhG7KcNHxMhoJrsZJFtC4Y --- tests/test_comprehensive_paths.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/test_comprehensive_paths.py b/tests/test_comprehensive_paths.py index 27b6f16..7a9a385 100644 --- a/tests/test_comprehensive_paths.py +++ b/tests/test_comprehensive_paths.py @@ -115,7 +115,9 @@ def test_comprehensive_path_resolution(): { "name": "Archive operations", "calculator": "sh://tar -czf archive.tar.gz subdir/ && echo 'result = 600'", - "expected_status": "done" if platform.system() != "Windows" else "failed" + # P0-8: "failed" on Windows came from the old resolver rewriting the + # (non-existent) archive name to the launch directory + "expected_status": "done" }, #{ NO: awk with file argument and redirection inside command line is not supported # "name": "Multiple file arguments", From a1df0930f0603574fe0f4c1e7db83875e91eed09 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 29 Sep 2026 17:52:42 +0000 Subject: [PATCH 3/3] P0-8: make resolver unit tests path-separator agnostic (Windows) Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01RhG7KcNHxMhoJrsZJFtC4Y --- tests/test_p0_8_sh_path_resolution.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/test_p0_8_sh_path_resolution.py b/tests/test_p0_8_sh_path_resolution.py index 8661e33..292c775 100644 --- a/tests/test_p0_8_sh_path_resolution.py +++ b/tests/test_p0_8_sh_path_resolution.py @@ -23,9 +23,9 @@ def test_resolver_skips_nonexistent_and_case_files(tmp_path): cmd, _ = resolve_all_paths_in_command( "bash tool.sh in.txt > out.txt", str(launch), str(case) ) - assert f"{launch}/tool.sh" in cmd # only in launch dir: resolved - assert f"{launch}/in.txt" not in cmd # in case dir: compiled file wins - assert f"{launch}/out.txt" not in cmd # redirection target untouched + assert f"{launch.as_posix()}/tool.sh" in cmd # only in launch dir: resolved + assert f"{launch.as_posix()}/in.txt" not in cmd # in case dir: compiled file wins + assert f"{launch.as_posix()}/out.txt" not in cmd # redirection target untouched assert cmd.endswith("> out.txt") @@ -39,12 +39,12 @@ def test_resolver_never_resolves_output_redirect_even_if_exists(tmp_path): cmd, _ = resolve_all_paths_in_command( f"echo hi {op} out.txt", str(launch), str(case) ) - assert str(launch) not in cmd + assert launch.as_posix() not in cmd def test_resolver_nonexistent_word_kept(tmp_path): cmd, _ = resolve_all_paths_in_command("bash ghost.sh ./x", str(tmp_path), None) - assert str(tmp_path) not in cmd + assert tmp_path.as_posix() not in cmd def test_fzr_sh_cat_redirect_two_cases(tmp_path):