Skip to content

Commit 64690ce

Browse files
committed
Address remaining review comments: patch-path validation, CI clean-tree check
- Validate each patch path in the interactive TUI stepper before handing it to Patch.from_file, mirroring SubProject._apply_patches' own skip checks (missing file, or outside the current directory). A rejected patch logs a warning and the step still advances, rather than crashing or silently applying an unvalidated path. Filtering the patch list up front was tried first but rejected: it shrinks the reported total, which threw off the fully-patched restore-path decision in combined mode. - CI: verify dfetch replay-patches actually leaves the working tree unchanged (snapshot git status before/after) instead of just checking it exits 0, in both the cygwin and the OS/Python matrix jobs. Runs under shell: bash explicitly so the check is identical on Windows (Git Bash) and Linux/macOS; verified against a real replay-patches run. Left unfixed with reasoning: - security/tm_usage.py's "preserve pre-existing staged changes" comment: verified against current code -- _can_review_project already calls has_local_changes_in_dir before staging, which skips any project with staged OR unstaged changes, so there's never a pre-existing staged state to lose. False positive; the reviewer's own analysis was scoped to replay_patches.py and git.py only, missing this upstream guard. - The dfetch.terminal import "layer violation": already verified in an earlier round -- pyproject.toml's actual enforced import-linter contract places dfetch.terminal in the same bottom layer as util/log, and lint-imports passes. The reviewer's guideline source (AGENTS.md's prose diagram) just omits it from the list. - Generalizing asciinema's exit-status propagation across all ~19 generate-casts.sh demo scripts: out of scope for this PR, which only fixed it for the two replay-patches demos it adds. Verified: 734 unit tests, ruff/pylint/mypy/pydocstyle/bandit/lint-imports, git+SVN replay-patches BDD scenarios, and the new CI check against a real replay-patches run all pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A7miizBGMoLEXXZxuq6XUJ
1 parent 4ceb0c1 commit 64690ce

3 files changed

Lines changed: 77 additions & 6 deletions

File tree

.github/workflows/run.yml

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,17 @@ jobs:
6666
- run: dfetch update
6767
- run: dfetch update
6868
- run: dfetch update-patch
69-
- run: dfetch replay-patches
69+
- name: Verify replay-patches restores a clean working tree
70+
shell: bash
71+
run: |
72+
before="$(git status --porcelain)"
73+
dfetch replay-patches
74+
after="$(git status --porcelain)"
75+
if [ "$before" != "$after" ]; then
76+
echo "::error::dfetch replay-patches left the working tree changed"
77+
git status
78+
exit 1
79+
fi
7080
- run: dfetch format-patch
7181
- run: dfetch report -t sbom
7282
- run: dfetch remove test-repo
@@ -212,7 +222,17 @@ jobs:
212222
- run: dfetch update
213223
- run: dfetch update
214224
- run: dfetch update-patch
215-
- run: dfetch replay-patches
225+
- name: Verify replay-patches restores a clean working tree
226+
shell: bash
227+
run: |
228+
before="$(git status --porcelain)"
229+
dfetch replay-patches
230+
after="$(git status --porcelain)"
231+
if [ "$before" != "$after" ]; then
232+
echo "::error::dfetch replay-patches left the working tree changed"
233+
git status
234+
exit 1
235+
fi
216236
- run: dfetch format-patch
217237
- run: dfetch report -t sbom
218238
- run: dfetch remove test-repo

dfetch/commands/replay_patches.py

Lines changed: 28 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -284,6 +284,22 @@ def _can_review_project(
284284
return True
285285

286286

287+
def _is_safe_patch_path(patch: str) -> bool:
288+
"""Return False for a patch that ``SubProject._apply_patches`` would have skipped.
289+
290+
Mirrors its checks (stays within the current directory, and exists) so
291+
the interactive stepper never hands ``Patch.from_file`` a path that the
292+
non-interactive apply would have silently skipped instead.
293+
"""
294+
cwd = Path.cwd()
295+
patch_path = (cwd / patch).resolve()
296+
try:
297+
patch_path.relative_to(cwd)
298+
except ValueError:
299+
return False
300+
return patch_path.exists()
301+
302+
287303
def _apply_review(
288304
subproject: SubProject,
289305
project_name: str,
@@ -509,12 +525,20 @@ def _apply_step(
509525
) -> tuple[int, bool]:
510526
"""Handle one keypress; return (new_current, done)."""
511527
if key == "LEFT" and current > 0:
512-
with _silent_patch_ng():
513-
Patch.from_file(patches[current - 1]).reverse().apply(root=local_path)
528+
patch = patches[current - 1]
529+
if _is_safe_patch_path(patch):
530+
with _silent_patch_ng():
531+
Patch.from_file(patch).reverse().apply(root=local_path)
532+
else:
533+
logger.warning(f'Skipping patch "{patch}": missing or outside {Path.cwd()}')
514534
return current - 1, False
515535
if key == "RIGHT" and current < total:
516-
with _silent_patch_ng():
517-
Patch.from_file(patches[current]).apply(root=local_path)
536+
patch = patches[current]
537+
if _is_safe_patch_path(patch):
538+
with _silent_patch_ng():
539+
Patch.from_file(patch).apply(root=local_path)
540+
else:
541+
logger.warning(f'Skipping patch "{patch}": missing or outside {Path.cwd()}')
518542
return current + 1, False
519543
if key in ("ENTER", "ESC"):
520544
return current, True

tests/test_replay_patches.py

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -431,3 +431,30 @@ def test_stage_one_restores_worktree_on_add_path_failure():
431431

432432
sub_a.update.assert_called_once()
433433
fake_super.restore_from_head.assert_called_once_with("proj_a")
434+
435+
436+
def test_is_safe_patch_path_rejects_missing_and_outside_root():
437+
"""A missing patch, or one outside cwd, is rejected rather than handed to Patch.from_file."""
438+
from dfetch.commands.replay_patches import _is_safe_patch_path
439+
440+
with tempfile.TemporaryDirectory() as tmpdir:
441+
with patch("pathlib.Path.cwd", return_value=Path(tmpdir)):
442+
(Path(tmpdir) / "patches").mkdir()
443+
(Path(tmpdir) / "patches" / "real.patch").write_text("diff")
444+
445+
assert _is_safe_patch_path("patches/real.patch") is True
446+
assert _is_safe_patch_path("patches/missing.patch") is False
447+
assert _is_safe_patch_path("../outside.patch") is False
448+
449+
450+
def test_apply_step_skips_unsafe_patch_without_crashing():
451+
"""RIGHT on a missing patch logs a warning, still advances, and does not raise."""
452+
from dfetch.commands.replay_patches import _apply_step
453+
454+
with tempfile.TemporaryDirectory() as tmpdir:
455+
with patch("pathlib.Path.cwd", return_value=Path(tmpdir)):
456+
current, done = _apply_step(
457+
"RIGHT", 0, 1, ["patches/missing.patch"], "some/local/path"
458+
)
459+
460+
assert (current, done) == (1, False)

0 commit comments

Comments
 (0)