Add replay-patches command#1290
Open
spoorcc wants to merge 22 commits into
Open
Conversation
Contributor
WalkthroughAdds ChangesReplay-patches command feature
Sequence Diagram(s)Single-project replay flow sequenceDiagram
participant User
participant ReplayPatches
participant SubProject
participant GitSuperProject
User->>ReplayPatches: dfetch replay-patches project
ReplayPatches->>ReplayPatches: parse project spec and validate prerequisites
ReplayPatches->>SubProject: create subproject
alt Git superproject
ReplayPatches->>GitSuperProject: add_path(path)
end
ReplayPatches->>SubProject: apply_patches(count)
alt Git superproject
ReplayPatches->>GitSuperProject: restore_staged(path) or restore_from_head(path)
else non-Git superproject
ReplayPatches->>SubProject: reinitialize and reapply patches
end
Combined multi-project replay flow sequenceDiagram
participant User
participant ReplayPatches
participant SubProject
participant GitSuperProject
participant _step_tui_multi
User->>ReplayPatches: dfetch replay-patches proj1 proj2
ReplayPatches->>ReplayPatches: collect eligible projects
loop each selected project
ReplayPatches->>GitSuperProject: add_path(path)
ReplayPatches->>SubProject: apply_patches(count)
end
alt interactive
ReplayPatches->>_step_tui_multi: launch multi-project TUI
User->>_step_tui_multi: up/down/left/right keys
else non-interactive
ReplayPatches-->>User: pause for manual review
end
loop each selected project
ReplayPatches->>GitSuperProject: restore_staged(path) or restore_from_head(path)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dfetch/commands/review_patch.py`:
- Around line 113-200: The `_review_project` method has too many branches and
conditional paths, exceeding the cyclomatic complexity limit of 8. Extract the
guard validations (checking for patch existence, on_disk_version, and local
changes) into a separate helper method that returns early if validation fails,
move the interactive review logic and non-interactive logic into their own
helper methods, and simplify the main method to orchestrate validation,
application, and restoration in a clearer sequence. This will distribute the
branching logic across focused helper methods while keeping the main method as a
clear orchestrator.
- Around line 70-78: The --count argument lacks validation, allowing negative
integers that produce unexpected Python slice behavior rather than a meaningful
CLI contract. Add a custom type validator to the add_argument call for --count
to ensure only positive integers are accepted. Additionally, update the logic
around line 173 where the raw count is forwarded to patch_count to validate
against negative values and clamp to valid ranges. Finally, modify the reporting
logic around line 181 to track and report the actual number of patches that were
successfully applied, not the requested count, since the effective count may
differ from what was requested.
- Around line 183-187: The logger.print_info_line call in the review_patch
function currently hardcodes the instruction to use git diff, but this is
inconsistent with non-Git superprojects like SVN that should use their own diff
commands. Make the diff command suggestion in the message VCS-aware by checking
the project's VCS type and conditionally including the appropriate diff command
(git diff for Git projects, svn diff for SVN projects, etc.) in the status
message printed to the user.
In `@dfetch/vcs/git.py`:
- Around line 785-793: Add the `--` separator before the path argument in both
the add_path and restore_staged methods to prevent Git from interpreting
option-style paths as flags. In add_path, insert `"--"` between `"add"` and
`path` in the command list passed to run_on_cmdline. In restore_staged, insert
`"--"` between `"--staged"` and `path` in the command list passed to
run_on_cmdline. This ensures Git treats the path as a positional argument rather
than a potential option flag.
In `@tests/test_review_patch.py`:
- Around line 79-97: Add a new test function to validate that negative count
values are rejected by the ReviewPatch command. Create a test that instantiates
ReviewPatch, mocks the required dependencies (create_super_project,
create_sub_project, in_directory, is_tty) similar to
test_review_count_1_uses_patch_count_1, and then calls cmd(_make_args(count=-1))
while asserting that this raises an appropriate validation error or exception.
This ensures the CLI contract maintains count-based validation and prevents
regression to slice-driven behavior for negative values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1290abec-833f-46ef-9d9d-5ff586b6111b
📒 Files selected for processing (11)
CHANGELOG.rstdfetch/__main__.pydfetch/commands/command.pydfetch/commands/review_patch.pydfetch/commands/update_patch.pydfetch/project/gitsuperproject.pydfetch/vcs/git.pydoc/howto/patching.rstfeatures/review-patch-in-git.featurefeatures/review-patch-in-svn.featuretests/test_review_patch.py
spoorcc
force-pushed
the
claude/review-patch-command-xyosoo
branch
from
cfc9798 to
6312b9a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dfetch/commands/review_patch.py`:
- Line 260: The Ctrl-C handler is currently catching KeyboardInterrupt, clearing
the screen, and returning normally, but the UI message advertises "Ctrl-C abort"
which implies the command should abort. Locate the exception handlers that catch
KeyboardInterrupt (near lines 260 and 319-321 in the review_patch.py file) and
modify them to re-raise the KeyboardInterrupt exception after clearing the
screen, rather than returning normally. This will allow the outer finally block
to restore state before the command properly aborts.
- Line 47: The import statement in review_patch.py violates the command-layer
dependency boundary by directly importing from dfetch.terminal, which is not an
allowed dependency. The imports of Screen, is_tty, read_key, BOLD, DIM, and
RESET must be sourced from one of the allowed layers (dfetch.reporting,
dfetch.project, dfetch.manifest, dfetch.vcs, dfetch.util, or dfetch.log). Either
move these terminal primitives to one of the allowed modules or create a
wrapper/facade in an allowed module that exposes these utilities, then update
the import in review_patch.py to import from the allowed layer instead of
directly from dfetch.terminal.
- Around line 137-147: The mutations to the subproject via subproject.update()
and to git_super via git_super.add_path() are occurring before the try/finally
restore guard begins, which means if either call fails, the restore mechanism in
the finally block will not execute and the worktree/index could be left in an
inconsistent state. Move the subproject.update() call (with patch_count=0) and
the conditional git_super.add_path() call to occur after the try block starts,
so they are protected by the restore guard in the finally block. The same issue
also applies to the code in the range around lines 159-174, so ensure all
mutations that need protection occur within the try block.
- Around line 183-202: The ReviewPatch functionality needs to validate patch
files exist and are accessible before treating them as applicable or performing
worktree operations. Add comprehensive patch file validation in the ReviewPatch
method that performs checks similar to those shown in the diff (verifying patch
existence via subproject.patch, confirming the subproject version exists via
on_disk_version(), and checking for local changes via
has_local_changes_in_dir()) to ensure that chosen_count == -1 or any decision to
apply patches is only made when the patch file is actually valid and accessible.
Ensure this validation logic is applied consistently across all locations where
patches are processed (including the locations at lines 217-221, 233-246, and
290-297) before any worktree replacement operations occur, and validate the
actual patch file object when calling Patch.from_file to catch missing or
out-of-root patch files early.
In `@dfetch/project/subproject.py`:
- Around line 90-97: The apply_patches method accepts a count parameter but does
not validate the input, allowing invalid values like -2 to be passed and cause
unexpected behavior in the slicing logic. Add a validation guard at the
beginning of the apply_patches method that raises a ValueError if count is less
than -1, ensuring only -1 (meaning all patches) or positive values are accepted
as documented in the docstring.
In `@features/steps/git_steps.py`:
- Around line 264-266: In the subprocess.check_output call for the git status
command, add the `--` separator argument between the `--porcelain` option and
the path argument to prevent paths starting with a hyphen from being interpreted
as git options. Modify the command list from `["git", "status", "--porcelain",
path]` to `["git", "status", "--porcelain", "--", path]`.
- Line 10: Move the no-name-in-module pylint suppression from the inline comment
on the import statement (from behave import given, then, when) to the
module-level header. Locate the existing pylint disable comment on line 3 in the
module header and add no-name-in-module to that existing disable list, then
remove the inline # pylint: disable=no-name-in-module comment from the import
statement on line 10.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8c8ecd44-472d-43c4-89d7-b69bfa272e16
⛔ Files ignored due to path filters (1)
doc/asciicasts/review-patch.gifis excluded by!**/*.gif
📒 Files selected for processing (18)
.github/workflows/run.ymlCHANGELOG.rstdfetch/__main__.pydfetch/commands/command.pydfetch/commands/review_patch.pydfetch/commands/update_patch.pydfetch/project/gitsuperproject.pydfetch/project/subproject.pydfetch/vcs/git.pydoc/asciicasts/review-patch.castdoc/generate-casts/generate-casts.shdoc/generate-casts/review-patch-demo.shdoc/howto/patching.rstfeatures/review-patch-in-git.featurefeatures/review-patch-in-svn.featurefeatures/steps/git_steps.pysecurity/tm_usage.pytests/test_review_patch.py
| from dfetch.project.gitsuperproject import GitSuperProject | ||
| from dfetch.project.subproject import SubProject | ||
| from dfetch.project.superproject import NoVcsSuperProject, SuperProject | ||
| from dfetch.terminal import BOLD, DIM, RESET, Screen, is_tty, read_key |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Route terminal primitives through an allowed layer.
dfetch.commands.review_patch imports dfetch.terminal directly, which violates the command-layer dependency boundary. Move or expose Screen, is_tty, read_key, and the style constants through an allowed lower layer before merging.
As per coding guidelines, dfetch/commands/*.py commands may only import from dfetch.reporting, dfetch.project, dfetch.manifest, dfetch.vcs, dfetch.util, and dfetch.log.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dfetch/commands/review_patch.py` at line 47, The import statement in
review_patch.py violates the command-layer dependency boundary by directly
importing from dfetch.terminal, which is not an allowed dependency. The imports
of Screen, is_tty, read_key, BOLD, DIM, and RESET must be sourced from one of
the allowed layers (dfetch.reporting, dfetch.project, dfetch.manifest,
dfetch.vcs, dfetch.util, or dfetch.log). Either move these terminal primitives
to one of the allowed modules or create a wrapper/facade in an allowed module
that exposes these utilities, then update the import in review_patch.py to
import from the allowed layer instead of directly from dfetch.terminal.
Source: Coding guidelines
Comment on lines
+137
to
+147
| saved_metadata = Path(subproject.metadata_path).read_bytes() | ||
| total_patches = len(list(subproject.patch)) | ||
| subproject.update( | ||
| force=True, | ||
| ignored_files_callback=_ignored, | ||
| patch_count=0, | ||
| eol_preferences_callback=superproject.eol_preferences, | ||
| ) | ||
|
|
||
| if git_super is not None: | ||
| git_super.add_path(subproject.local_path) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Start the restore guard before mutating the project.
subproject.update(... patch_count=0) and git_super.add_path(...) both mutate the worktree/index before the try/finally begins. If either raises, the command can leave the project at clean upstream or with a staged index, bypassing the restore path.
🛡️ Proposed restore-scope fix
saved_metadata = Path(subproject.metadata_path).read_bytes()
total_patches = len(list(subproject.patch))
- subproject.update(
- force=True,
- ignored_files_callback=_ignored,
- patch_count=0,
- eol_preferences_callback=superproject.eol_preferences,
- )
-
- if git_super is not None:
- git_super.add_path(subproject.local_path)
-
chosen_count = count if count is not None else -1
effective = (
total_patches if chosen_count == -1 else min(chosen_count, total_patches)
)
@@
)
worktree_fully_patched = False
try:
+ subproject.update(
+ force=True,
+ ignored_files_callback=_ignored,
+ patch_count=0,
+ eol_preferences_callback=superproject.eol_preferences,
+ )
+
+ if git_super is not None:
+ git_super.add_path(subproject.local_path)
+
worktree_fully_patched = _apply_review(
subproject, project.name, chosen_count, interactive, info_msg
)Also applies to: 159-174
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dfetch/commands/review_patch.py` around lines 137 - 147, The mutations to the
subproject via subproject.update() and to git_super via git_super.add_path() are
occurring before the try/finally restore guard begins, which means if either
call fails, the restore mechanism in the finally block will not execute and the
worktree/index could be left in an inconsistent state. Move the
subproject.update() call (with patch_count=0) and the conditional
git_super.add_path() call to occur after the try block starts, so they are
protected by the restore guard in the finally block. The same issue also applies
to the code in the range around lines 159-174, so ensure all mutations that need
protection occur within the try block.
Comment on lines
+183
to
+202
| if not subproject.patch: | ||
| logger.print_warning_line( | ||
| project_name, | ||
| 'skipped - there is no patch file, use "dfetch diff"' | ||
| f" {project_name} to create one", | ||
| ) | ||
| return False | ||
| if not subproject.on_disk_version(): | ||
| logger.print_warning_line( | ||
| project_name, | ||
| f'skipped - the project was never fetched, use "dfetch update {project_name}"', | ||
| ) | ||
| return False | ||
| if superproject.has_local_changes_in_dir(subproject.local_path): | ||
| logger.print_warning_line( | ||
| project_name, | ||
| f"skipped - uncommitted changes in {subproject.local_path}", | ||
| ) | ||
| return False | ||
| return True |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate patch files before replacing the worktree.
SubProject._apply_patches skips missing or out-of-root patch files, but ReviewPatch still treats chosen_count == -1 as fully restored/applicable and may only unstage Git afterwards. A missing patch can leave the working tree at clean upstream instead of the original patched state; interactive mode also feeds raw manifest paths directly to Patch.from_file.
🛡️ Proposed pre-validation shape
-from collections.abc import Callable, Generator
+from collections.abc import Callable, Generator, Sequence
@@
def _can_review_project(
@@
return True
+
+
+def _patch_paths_for_review(
+ patches: Sequence[str],
+ project_name: str,
+) -> list[str] | None:
+ """Return normalized patch paths, or None when review should be skipped."""
+ cwd = Path(".").resolve()
+ normalized: list[str] = []
+ for patch in patches:
+ patch_path = (cwd / patch).resolve()
+ try:
+ relative_patch_path = patch_path.relative_to(cwd)
+ except ValueError:
+ logger.print_warning_line(
+ project_name,
+ f'skipped - patch "{patch}" is outside {cwd}',
+ )
+ return None
+ if not patch_path.exists():
+ logger.print_warning_line(
+ project_name,
+ f'skipped - patch "{patch}" does not exist',
+ )
+ return None
+ normalized.append(relative_patch_path.as_posix())
+ return normalized if not _can_review_project(superproject, subproject, project.name):
return
+ patches = _patch_paths_for_review(subproject.patch, project.name)
+ if patches is None:
+ return
+
saved_metadata = Path(subproject.metadata_path).read_bytes()
- total_patches = len(list(subproject.patch))
+ total_patches = len(patches)
@@
worktree_fully_patched = _apply_review(
- subproject, project.name, chosen_count, interactive, info_msg
+ subproject, project.name, chosen_count, interactive, info_msg, patches
) def _apply_review(
subproject: SubProject,
project_name: str,
chosen_count: int,
interactive: bool,
info_msg: str,
+ patches: list[str],
) -> bool:
@@
if interactive:
- _step_tui(list(subproject.patch), subproject.local_path, project_name)
+ _step_tui(patches, subproject.local_path, project_name)
return FalseAlso applies to: 217-221, 233-246, 290-297
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dfetch/commands/review_patch.py` around lines 183 - 202, The ReviewPatch
functionality needs to validate patch files exist and are accessible before
treating them as applicable or performing worktree operations. Add comprehensive
patch file validation in the ReviewPatch method that performs checks similar to
those shown in the diff (verifying patch existence via subproject.patch,
confirming the subproject version exists via on_disk_version(), and checking for
local changes via has_local_changes_in_dir()) to ensure that chosen_count == -1
or any decision to apply patches is only made when the patch file is actually
valid and accessible. Ensure this validation logic is applied consistently
across all locations where patches are processed (including the locations at
lines 217-221, 233-246, and 290-297) before any worktree replacement operations
occur, and validate the actual patch file object when calling Patch.from_file to
catch missing or out-of-root patch files early.
| """Render the current patch-stack state to the screen.""" | ||
| count_label = str(current) if current < total else "all" | ||
| lines: list[str] = [ | ||
| f" {DIM}← → step Enter restore and exit Ctrl-C abort{RESET}", |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not swallow Ctrl-C when the UI says abort.
The frame advertises Ctrl-C abort, but the handler clears the screen and returns normally. Re-raise after clearing so the command aborts after the outer finally restores state.
🛠️ Proposed signal-handling fix
try:
key = read_key()
except KeyboardInterrupt:
screen.clear()
- return
+ raiseAlso applies to: 319-321
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dfetch/commands/review_patch.py` at line 260, The Ctrl-C handler is currently
catching KeyboardInterrupt, clearing the screen, and returning normally, but the
UI message advertises "Ctrl-C abort" which implies the command should abort.
Locate the exception handlers that catch KeyboardInterrupt (near lines 260 and
319-321 in the review_patch.py file) and modify them to re-raise the
KeyboardInterrupt exception after clearing the screen, rather than returning
normally. This will allow the outer finally block to restore state before the
command properly aborts.
| import argparse | ||
| import tempfile | ||
| from pathlib import Path | ||
| from unittest.mock import ANY, Mock, call, patch |
ben-edna
changed the title
Contributor
|
@coderabbitai resume |
Contributor
|
✅ Action performedReviews resumed. Review finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/run.yml:
- Line 59: The CI workflow is invoking the wrong dfetch subcommand, so replace
the existing review-patch step with the registered replay-patches command in the
run job. Update the workflow entries that use dfetch review-patch to use dfetch
replay-patches instead, keeping the rest of the step unchanged so the CI
coverage runs the intended patch replay flow.
In `@dfetch/commands/replay_patches.py`:
- Around line 70-80: The project spec parser in _parse_project_spec currently
accepts negative N values from name:N, which lets project:-1 be treated as all
patches and other negatives fail later outside normal handling. Update
_parse_project_spec in replay_patches.py to validate the parsed integer and
reject any count below zero with the same RuntimeError-style validation used for
malformed specs, so SubProject.apply_patches() only receives None or
non-negative counts.
In `@doc/asciicasts/replay-patches.cast`:
- Line 8: The recorded command in the replay-patches cast uses the old
subcommand name, so update the command entry in replay-patches.cast to use
dfetch replay-patches instead of dfetch review-patch. Regenerate the cast
artifact for that command so the documented terminal recording matches the
published name and users copy the correct invocation.
In `@doc/generate-casts/generate-casts.sh`:
- Line 31: The cast filename used by the generate-casts script is inconsistent
with the documented artifact name, so update the asciinema command in
generate-casts.sh to emit the same replay filename referenced by the docs. Keep
the cleanup behavior intact, and ensure the output path and cast basename used
in the script match the documentation expectation so the generated artifact can
be found reliably.
In `@doc/generate-casts/review-patch-demo.sh`:
- Line 47: The demo script is calling the wrong dfetch subcommand, so align the
command used in the review-patch demo with the documented replay flow. Update
the invocation in the shell script around the `dfetch review-patch` call to use
the correct `dfetch replay-patches` command so the cast generation example
matches the intended feature name.
In `@features/steps/git_steps.py`:
- Around line 262-267: The new Behave step function step_impl in git_steps.py is
missing the required Google-style docstring. Add a concise docstring directly
under step_impl that follows the repository’s convention and describes the
step’s purpose and arguments (including superproject and path) so it is
consistent with the other step definitions in this module.
In `@plan.md`:
- Around line 135-145: Add a language tag to the fenced code block in the
markdown snippet so it satisfies markdownlint MD040; update the fence in the
affected block to use a language such as text, keeping the content in the
plan/patch list unchanged.
In `@security/tm_usage.py`:
- Around line 47-49: The import in tm_usage should not rely on inline lint
suppressions; remove the noqa/pylint disables from the USAGE_CONTROLS import and
fix the underlying import-order issue instead. Update the module so the import
can pass lint cleanly without suppressions, using the tm_usage and
tm_controls_data import structure as the place to resolve the root cause.
In `@tests/test_replay_patches.py`:
- Around line 56-390: The new pytest test functions in replay_patches tests are
missing the required Google-style docstrings. Add a short Google-style docstring
to each new test function in this module, using the existing test names like
test_review_all_patches_calls_update_add_path_update,
test_svn_superproject_warns_and_skips_staging, and
test_combined_interactive_launches_tui to locate them, and keep the wording
consistent with the test’s behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4b3dfb1d-4c18-482e-9af4-13e63746a612
⛔ Files ignored due to path filters (1)
doc/asciicasts/replay-patches.gifis excluded by!**/*.gif
📒 Files selected for processing (19)
.github/workflows/run.ymlCHANGELOG.rstdfetch/__main__.pydfetch/commands/command.pydfetch/commands/replay_patches.pydfetch/commands/update_patch.pydfetch/project/gitsuperproject.pydfetch/project/subproject.pydfetch/vcs/git.pydoc/asciicasts/replay-patches.castdoc/generate-casts/generate-casts.shdoc/generate-casts/review-patch-demo.shdoc/howto/patching.rstfeatures/replay-patches-in-git.featurefeatures/replay-patches-in-svn.featurefeatures/steps/git_steps.pyplan.mdsecurity/tm_usage.pytests/test_replay_patches.py
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/run.yml:
- Line 59: The CI workflow is invoking the wrong dfetch subcommand, so replace
the existing review-patch step with the registered replay-patches command in the
run job. Update the workflow entries that use dfetch review-patch to use dfetch
replay-patches instead, keeping the rest of the step unchanged so the CI
coverage runs the intended patch replay flow.
In `@dfetch/commands/replay_patches.py`:
- Around line 70-80: The project spec parser in _parse_project_spec currently
accepts negative N values from name:N, which lets project:-1 be treated as all
patches and other negatives fail later outside normal handling. Update
_parse_project_spec in replay_patches.py to validate the parsed integer and
reject any count below zero with the same RuntimeError-style validation used for
malformed specs, so SubProject.apply_patches() only receives None or
non-negative counts.
In `@doc/asciicasts/replay-patches.cast`:
- Line 8: The recorded command in the replay-patches cast uses the old
subcommand name, so update the command entry in replay-patches.cast to use
dfetch replay-patches instead of dfetch review-patch. Regenerate the cast
artifact for that command so the documented terminal recording matches the
published name and users copy the correct invocation.
In `@doc/generate-casts/generate-casts.sh`:
- Line 31: The cast filename used by the generate-casts script is inconsistent
with the documented artifact name, so update the asciinema command in
generate-casts.sh to emit the same replay filename referenced by the docs. Keep
the cleanup behavior intact, and ensure the output path and cast basename used
in the script match the documentation expectation so the generated artifact can
be found reliably.
In `@doc/generate-casts/review-patch-demo.sh`:
- Line 47: The demo script is calling the wrong dfetch subcommand, so align the
command used in the review-patch demo with the documented replay flow. Update
the invocation in the shell script around the `dfetch review-patch` call to use
the correct `dfetch replay-patches` command so the cast generation example
matches the intended feature name.
In `@features/steps/git_steps.py`:
- Around line 262-267: The new Behave step function step_impl in git_steps.py is
missing the required Google-style docstring. Add a concise docstring directly
under step_impl that follows the repository’s convention and describes the
step’s purpose and arguments (including superproject and path) so it is
consistent with the other step definitions in this module.
In `@plan.md`:
- Around line 135-145: Add a language tag to the fenced code block in the
markdown snippet so it satisfies markdownlint MD040; update the fence in the
affected block to use a language such as text, keeping the content in the
plan/patch list unchanged.
In `@security/tm_usage.py`:
- Around line 47-49: The import in tm_usage should not rely on inline lint
suppressions; remove the noqa/pylint disables from the USAGE_CONTROLS import and
fix the underlying import-order issue instead. Update the module so the import
can pass lint cleanly without suppressions, using the tm_usage and
tm_controls_data import structure as the place to resolve the root cause.
In `@tests/test_replay_patches.py`:
- Around line 56-390: The new pytest test functions in replay_patches tests are
missing the required Google-style docstrings. Add a short Google-style docstring
to each new test function in this module, using the existing test names like
test_review_all_patches_calls_update_add_path_update,
test_svn_superproject_warns_and_skips_staging, and
test_combined_interactive_launches_tui to locate them, and keep the wording
consistent with the test’s behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4b3dfb1d-4c18-482e-9af4-13e63746a612
⛔ Files ignored due to path filters (1)
doc/asciicasts/replay-patches.gifis excluded by!**/*.gif
📒 Files selected for processing (19)
.github/workflows/run.ymlCHANGELOG.rstdfetch/__main__.pydfetch/commands/command.pydfetch/commands/replay_patches.pydfetch/commands/update_patch.pydfetch/project/gitsuperproject.pydfetch/project/subproject.pydfetch/vcs/git.pydoc/asciicasts/replay-patches.castdoc/generate-casts/generate-casts.shdoc/generate-casts/review-patch-demo.shdoc/howto/patching.rstfeatures/replay-patches-in-git.featurefeatures/replay-patches-in-svn.featurefeatures/steps/git_steps.pyplan.mdsecurity/tm_usage.pytests/test_replay_patches.py
🛑 Comments failed to post (9)
.github/workflows/run.yml (1)
59-59: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the registered
replay-patchescommand in CI.These steps run
dfetch review-patch, but the command introduced and documented in this PR context isreplay-patches. This will fail with an unknown subcommand and miss the intended CI coverage.Suggested fix
- - run: dfetch review-patch + - run: dfetch replay-patches ... - - run: dfetch review-patch + - run: dfetch replay-patchesAlso applies to: 202-202
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/run.yml at line 59, The CI workflow is invoking the wrong dfetch subcommand, so replace the existing review-patch step with the registered replay-patches command in the run job. Update the workflow entries that use dfetch review-patch to use dfetch replay-patches instead, keeping the rest of the step unchanged so the CI coverage runs the intended patch replay flow.dfetch/commands/replay_patches.py (1)
70-80: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject negative counts in
name:Nspecs.
--countis validated, butproject:-1andproject:-2still get through here.-1is silently treated as “all patches”, while< -1later tripsSubProject.apply_patches()with an uncaughtValueError, bypassing the normal per-project error handling.🔧 Suggested fix
def _parse_project_spec(spec: str) -> tuple[str, int | None]: """Split 'name:N' into (name, N); a bare name returns (name, None).""" if ":" not in spec: return spec, None name, _, tail = spec.rpartition(":") try: - return name, int(tail) + count = int(tail) except ValueError as exc: raise RuntimeError( f"invalid project spec {spec!r}; expected name or name:N" ) from exc + if count < 0: + raise RuntimeError("project patch count must be >= 0") + return name, count📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.def _parse_project_spec(spec: str) -> tuple[str, int | None]: """Split 'name:N' into (name, N); a bare name returns (name, None).""" if ":" not in spec: return spec, None name, _, tail = spec.rpartition(":") try: count = int(tail) except ValueError as exc: raise RuntimeError( f"invalid project spec {spec!r}; expected name or name:N" ) from exc if count < 0: raise RuntimeError("project patch count must be >= 0") return name, count🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dfetch/commands/replay_patches.py` around lines 70 - 80, The project spec parser in _parse_project_spec currently accepts negative N values from name:N, which lets project:-1 be treated as all patches and other negatives fail later outside normal handling. Update _parse_project_spec in replay_patches.py to validate the parsed integer and reject any count below zero with the same RuntimeError-style validation used for malformed specs, so SubProject.apply_patches() only receives None or non-negative counts.doc/asciicasts/replay-patches.cast (1)
8-8: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Recorded command uses outdated name in replay-patches cast.
Line 8 shows
dfetch review-patch ..., but this artifact is published asreplay-patches. Please regenerate this cast withdfetch replay-patches ...so users don’t copy a wrong command.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@doc/asciicasts/replay-patches.cast` at line 8, The recorded command in the replay-patches cast uses the old subcommand name, so update the command entry in replay-patches.cast to use dfetch replay-patches instead of dfetch review-patch. Regenerate the cast artifact for that command so the documented terminal recording matches the published name and users copy the correct invocation.Source: Learnings
doc/generate-casts/generate-casts.sh (1)
31-31: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Cast output filename does not match documentation reference.
Line 31 emits
review-patch.cast, while docs referencereplay-patches.cast. After cleanup at Line 12, this can leave docs pointing to a missing artifact.Suggested fix
-asciinema rec --overwrite -c "./review-patch-demo.sh" ../asciicasts/review-patch.cast +asciinema rec --overwrite -c "./review-patch-demo.sh" ../asciicasts/replay-patches.cast📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.asciinema rec --overwrite -c "./review-patch-demo.sh" ../asciicasts/replay-patches.cast🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@doc/generate-casts/generate-casts.sh` at line 31, The cast filename used by the generate-casts script is inconsistent with the documented artifact name, so update the asciinema command in generate-casts.sh to emit the same replay filename referenced by the docs. Keep the cleanup behavior intact, and ensure the output path and cast basename used in the script match the documentation expectation so the generated artifact can be found reliably.doc/generate-casts/review-patch-demo.sh (1)
47-47: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Demo script invokes mismatched command name.
Line 47 uses
dfetch review-patch, but the feature is documented asdfetch replay-patches. This should be aligned before regenerating casts.Suggested fix
-pe "echo '' | dfetch review-patch cpputest" +pe "echo '' | dfetch replay-patches cpputest"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.pe "echo '' | dfetch replay-patches cpputest"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@doc/generate-casts/review-patch-demo.sh` at line 47, The demo script is calling the wrong dfetch subcommand, so align the command used in the review-patch demo with the documented replay flow. Update the invocation in the shell script around the `dfetch review-patch` call to use the correct `dfetch replay-patches` command so the cast generation example matches the intended feature name.features/steps/git_steps.py (1)
262-267: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a Google-style docstring to this new Behave step function.
This new
step_implis missing a docstring required by the repository’s docstring convention.🧰 Tools
🪛 ast-grep (0.44.0)
[error] 263-265: Command coming from incoming request
Context: subprocess.check_output(
["git", "status", "--porcelain", "--", path], text=True
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').(subprocess-from-request)
[error] 263-265: Avoid command injection
Context: subprocess.check_output(
["git", "status", "--porcelain", "--", path], text=True
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').(command-injection-python)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@features/steps/git_steps.py` around lines 262 - 267, The new Behave step function step_impl in git_steps.py is missing the required Google-style docstring. Add a concise docstring directly under step_impl that follows the repository’s convention and describes the step’s purpose and arguments (including superproject and path) so it is consistent with the other step definitions in this module.Source: Coding guidelines
plan.md (1)
135-145: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language to the fenced code block at Line 135.
markdownlint MD040 is triggered here; use a language tag (for example,
text) on the fence.🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 135-135: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plan.md` around lines 135 - 145, Add a language tag to the fenced code block in the markdown snippet so it satisfies markdownlint MD040; update the fence in the affected block to use a language such as text, keeping the content in the plan/patch list unchanged.Source: Linters/SAST tools
security/tm_usage.py (1)
47-49: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove inline lint suppressions from this non-test module.
Please fix the import-order/root-cause instead of suppressing lint on this line.
As per coding guidelines, "
**/*.py: Do not use inline lint suppressions like# noqa,# type: ignore,# pylint: disable, or# pyright: ignore; fix the root cause instead. Exception: module-level tool headers (lines 1–5 of test files) are permitted."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@security/tm_usage.py` around lines 47 - 49, The import in tm_usage should not rely on inline lint suppressions; remove the noqa/pylint disables from the USAGE_CONTROLS import and fix the underlying import-order issue instead. Update the module so the import can pass lint cleanly without suppressions, using the tm_usage and tm_controls_data import structure as the place to resolve the root cause.Source: Coding guidelines
tests/test_replay_patches.py (1)
56-390: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add Google-style docstrings to the new pytest test functions.
The
test_*functions in this new module are missing docstrings, which violates the test-file docstring convention.🧰 Tools
🪛 ast-grep (0.44.0)
[info] 289-289: Do not hardcode temporary file or directory names
Context: "/tmp"
Note: [CWE-377] Insecure Temporary File.(hardcoded-tmp-file)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_replay_patches.py` around lines 56 - 390, The new pytest test functions in replay_patches tests are missing the required Google-style docstrings. Add a short Google-style docstring to each new test function in this module, using the existing test names like test_review_all_patches_calls_update_add_path_update, test_svn_superproject_warns_and_skips_staging, and test_combined_interactive_launches_tui to locate them, and keep the wording consistent with the test’s behavior.Source: Coding guidelines
spoorcc
force-pushed
the
claude/review-patch-command-xyosoo
branch
from
51d9752 to
67fbbc9
Compare
Introduces `dfetch review-patch` which stages the clean upstream source in the git index and applies the selected patches to the working tree, so any diff-aware editor sees `git diff` (working tree vs index) showing exactly what the patches contribute. The command always restores original state on exit — no permanent changes to working tree or index. - New command: dfetch/commands/review_patch.py with --count/-n, --interactive/-i - GitSuperProject.add_path() and restore_staged() methods for index control - GitLocalRepo.add_path() and restore_staged() as git-level primitives - SVN superprojects supported (with a warning; no staging step) - Interactive TUI uses read_key()/Screen for ← → step-through - 8 unit tests, 4 git BDD scenarios - Documentation and changelog updated Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zY8BoH65KBX6cz7Pm8aeF
- Extract shared project-iteration loop into Command._iter_projects to eliminate duplicate code between review_patch and update_patch (R0801) - Fix import order: review_patch before update in __main__.py (isort) - Remove redundant `import dfetch.project` in review_patch; import create_sub_project directly instead (code review finding) - Remove unused MagicMock import from test_review_patch (code review finding) - Update test patch targets to match new import locations Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zY8BoH65KBX6cz7Pm8aeF
- Add `dfetch review-patch` step after `dfetch update-patch` in both the `run` (all platforms) and `test-cygwin` jobs in run.yml - Add `review-patch-demo.sh` cast generation script (mirrors update-patch-demo.sh; pipes stdin to avoid blocking on input()) - Register the demo in generate-casts.sh after the update-patch line - Add synthetic review-patch.cast (asciicast v2) showing the full flow: manifest inspection, patch preview, staging, and restore - Wire `.. asciinema:: ../asciicasts/review-patch.cast` into the Reviewing a patch section of doc/howto/patching.rst Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zY8BoH65KBX6cz7Pm8aeF
In the default (all-patches) case review-patch was fetching three times: once for clean upstream, once to apply all patches, and once more in the finally block to "restore" — even though the working tree was already fully patched after the second fetch. Track worktree_fully_patched after the non-interactive update call and skip the finally fetch when the working tree is already at patch_count=-1. The --count N and --interactive paths still re-fetch because the final working-tree state differs from fully patched. Update tests and the synthetic demo cast accordingly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zY8BoH65KBX6cz7Pm8aeF
After the initial update(patch_count=0) puts clean upstream in the working tree there is no reason to re-fetch for any subsequent step. - Add SubProject.apply_patches(count) — applies patch files to the already-fetched working tree without going back to the remote. - Add GitSuperProject.restore_worktree(path) / GitLocalRepo.restore_worktree — runs `git restore <path>` to reset the working tree from the staged index (which holds clean upstream throughout the review session). - Non-interactive: replace the second update() call with apply_patches(). Git all-patches case: 1 fetch total (was 2). Git --count N case: 1 fetch + restore_worktree + apply_patches (was 3). - Interactive TUI: replace per-step update() calls with direct patch apply/reverse (Patch.from_file().apply() / .reverse().apply()). No fetch at all during stepping, for both Git and SVN. - Finally block (Git): restore_worktree + apply_patches instead of a re-fetch. SVN still re-fetches when the worktree is not fully patched (no staged index to restore from). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zY8BoH65KBX6cz7Pm8aeF
Extract _can_review_project, _apply_review, _restore_project, _draw_tui_frame, and _apply_step as module-level helpers so every function stays below the CC=8 limit enforced by CI. Black-format tests to match. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zY8BoH65KBX6cz7Pm8aeF
After the refactor, review-patch does only one fetch (clean upstream) and applies/reverses patches directly without re-fetching. The expected output in the BDD scenarios previously included 3 Fetched lines; update them to reflect the new single-fetch flow. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zY8BoH65KBX6cz7Pm8aeF
…ff cmd - Reject --count < 0 with a RuntimeError early in __call__ - Add test_negative_count_raises to prevent regression - Add -- separator before path in git add, git restore --staged, and git restore to prevent option-style paths being parsed as flags - Show `svn diff` in the status message for SVN superprojects instead of always saying `git diff` - Clamp reported patch count to min(requested, total) so the message reflects patches actually applied rather than the raw CLI argument - Add review-patch to the dfetch_cli threat-model description, noting the transient git index mutation and interruption risk - Reduce _apply_review to 5 args by pre-computing chosen_count and info_msg in _review_project (fixes pylint too-many-arguments) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zY8BoH65KBX6cz7Pm8aeF
Remove the second (now-eliminated) fetch from the cast, regenerate with agg to produce an animated GIF showing the single-fetch flow. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zY8BoH65KBX6cz7Pm8aeF
- Replace sp.__class__ = GitSuperProject/NoVcsSuperProject with Mock(spec=...) so isinstance() checks work without unsafe __class__ mutation that pyright rejects - Add str | None type annotation to on_disk_version parameter so passing None in test_never_fetched_logs_warning_and_skips is valid Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zY8BoH65KBX6cz7Pm8aeF
Replace is_git: bool + assert isinstance(superproject, GitSuperProject) pattern with git_super: GitSuperProject | None so the type checker tracks the narrowed type directly — no assert needed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zY8BoH65KBX6cz7Pm8aeF
update(patch_count=0) writes patch: '' to the metadata file. The restore path fixes the working tree but apply_patches() has no metadata-writing logic, leaving the metadata file missing the patch list. Fix: snapshot the metadata file bytes before the clean-upstream fetch and write them back in the finally block after restore completes. Add a BDD step "the metadata of ... lists patch ..." and assert it in both Git and SVN review-patch scenarios to pin the invariant. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zY8BoH65KBX6cz7Pm8aeF
Instead of asserting the patch field in dfetch_data.yaml directly, verify from the user's VCS perspective: git status --porcelain SomeProject must report nothing after review-patch exits, covering both the working tree files and the metadata file in one shot. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zY8BoH65KBX6cz7Pm8aeF
patch_ng logs "successfully patched N/M: file" via Python logging when Patch.apply() is called. In the TUI loop this output appears between frames, so Screen.draw()'s cursor-up count is wrong and the help line gets duplicated on each arrow keypress. Suppress the patch_ng logger to CRITICAL for the duration of each LEFT/RIGHT apply/reverse operation inside the TUI loop. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zY8BoH65KBX6cz7Pm8aeF
If Patch.apply() or Patch.reverse().apply() raises RuntimeError during the interactive stepper, the TUI frame was left on screen while the error and 'restored' messages printed below it. Catch RuntimeError in _step_tui, clear the screen, then re-raise so the existing try/finally in _review_project still runs _restore_project (working tree and metadata are always restored), and _iter_projects logs the error under the project name as usual. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zY8BoH65KBX6cz7Pm8aeF
The old restore path did restore_worktree (WT = staged = clean upstream) followed by apply_patches(), which would hit the same failing patch and raise again, leaving the index staged, the metadata corrupted, and the working tree in the wrong state. Replace with restore_from_head (git restore --source=HEAD --staged --worktree -- <path>) which atomically resets both the working tree and the index to the committed HEAD state without re-applying patches. This makes restore unconditionally safe regardless of patch health. Also nest the metadata write_bytes in its own try/finally so the dfetch_data.yaml snapshot is always restored even if _restore_project encounters an error (e.g. SVN path where apply_patches can still fail). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zY8BoH65KBX6cz7Pm8aeF
spoorcc
force-pushed
the
claude/review-patch-command-xyosoo
branch
from
67fbbc9 to
ea49e29
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/run.yml:
- Line 59: The smoke workflow currently only checks that dfetch replay-patches
exits successfully, but it does not verify that the repository is restored to a
clean state afterward. Update the run step in the workflow job to add a
follow-up cleanliness check such as git status --porcelain or git diff
--exit-code immediately after dfetch replay-patches so CI fails if any staged or
working-tree changes remain.
In `@doc/howto/patching.rst`:
- Around line 291-301: Clarify the Dfetch replay-patches introduction by
distinguishing Git and SVN behavior: the current statement about putting clean
upstream source in the git index only applies to Git superprojects, while SVN
superprojects use warning-based `svn diff` guidance without a staging step.
Update the prose in the patching how-to section to either qualify the index
wording for Git or explicitly mention the SVN difference so readers do not
assume index-based diff behavior for all backends.
In `@features/replay-patches-in-svn.feature`:
- Around line 33-50: The replay-patches SVN scenarios currently verify file
contents but not that the working copy is clean after restore. Update the
affected scenarios around the replay-patches flow to assert the SVN status is
clean after the command finishes, using an `svn status`-based step or an
equivalent existing assertion step, so the restore guarantee is covered in the
scenarios for `replay-patches` and `MySvnProject`.
In `@security/__init__.py`:
- Around line 6-9: The threat-model modules are relying on security/__init__.py
to mutate sys.path, which only works when the package is imported and breaks if
security/tm_usage.py or security/tm_supply_chain.py are run directly. Update the
entrypoints in those modules to use package-style execution/imports only, and
make the documentation build import the package through Sphinx configuration
rather than depending on runtime path injection. If needed, move the bootstrap
responsibility into doc/conf.py or package installation setup so internal
imports like security.tm_render always resolve without executing modules as
standalone scripts.
In `@tests/test_replay_patches.py`:
- Around line 56-112: Add a failure-path cleanup test for ReplayPatches: the
current tests cover only successful restoration, but not the case where
apply_patches() or the interactive flow raises after fake_super.add_path() has
staged changes. Add a test in
test_review_all_patches_calls_update_add_path_update that makes the subproject
or tty-driven step raise, then assert the appropriate restore helper on
fake_super still runs via the ReplayPatches cleanup path (finally-style), using
the existing ReplayPatches, _make_superproject, _make_subproject, and restore_*
symbols.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: aa839f61-fc5d-4216-bc5e-b664bc9c41a0
⛔ Files ignored due to path filters (1)
doc/asciicasts/replay-patches.gifis excluded by!**/*.gif
📒 Files selected for processing (20)
.github/workflows/run.ymlCHANGELOG.rstdfetch/__main__.pydfetch/commands/command.pydfetch/commands/replay_patches.pydfetch/commands/update_patch.pydfetch/project/gitsuperproject.pydfetch/project/subproject.pydfetch/vcs/git.pydoc/asciicasts/replay-patches.castdoc/generate-casts/generate-casts.shdoc/generate-casts/replay-patches-demo.shdoc/howto/patching.rstfeatures/replay-patches-in-git.featurefeatures/replay-patches-in-svn.featurefeatures/steps/git_steps.pysecurity/__init__.pysecurity/tm_supply_chain.pysecurity/tm_usage.pytests/test_replay_patches.py
| - run: dfetch update | ||
| - run: dfetch update | ||
| - run: dfetch update-patch | ||
| - run: dfetch replay-patches |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Verify the repo is clean after the smoke run.
dfetch replay-patches exiting 0 is not enough here; the feature's main guarantee is that it restores the original state. Add a follow-up git status --porcelain or git diff --exit-code check after this step so CI fails if the command leaves staged or working-tree changes behind.
Also applies to: 202-202
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/run.yml at line 59, The smoke workflow currently only
checks that dfetch replay-patches exits successfully, but it does not verify
that the repository is restored to a clean state afterward. Update the run step
in the workflow job to add a follow-up cleanliness check such as git status
--porcelain or git diff --exit-code immediately after dfetch replay-patches so
CI fails if any staged or working-tree changes remain.
Comment on lines
+291
to
+301
| *Dfetch* puts the clean upstream source in the git index and applies the | ||
| patches to the working tree. You can now see exactly what the patches change | ||
| using any diff tool you prefer — for example: | ||
|
|
||
| .. code-block:: console | ||
|
|
||
| $ git diff some-project/ | ||
|
|
||
| Or open the project in VS Code and browse the **Changes** view in the Source | ||
| Control panel. (The **Staged Changes** view shows something different and | ||
| unrelated to your patches — use **Changes**.) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clarify SVN behavior in the replay-patches introduction.
The description states that Dfetch "puts the clean upstream source in the git index," which is only true for Git superprojects. SVN superprojects are supported with a warning and use svn diff hints without a staging step. This will mislead SVN users who expect index-based diff behavior.
Add a qualifier for Git or note the SVN difference explicitly in this section.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@doc/howto/patching.rst` around lines 291 - 301, Clarify the Dfetch
replay-patches introduction by distinguishing Git and SVN behavior: the current
statement about putting clean upstream source in the git index only applies to
Git superprojects, while SVN superprojects use warning-based `svn diff` guidance
without a staging step. Update the prose in the patching how-to section to
either qualify the index wording for Git or explicitly mention the SVN
difference so readers do not assume index-based diff behavior for all backends.
Comment on lines
+33
to
+50
| Scenario: All patches are set up for review and state is restored afterwards | ||
| When I run "dfetch replay-patches SomeProject" in MySvnProject | ||
| Then the output shows | ||
| """ | ||
| Dfetch (0.14.0) | ||
| replay-patches has limited support in SVN superprojects (no staging area — use `svn diff` to inspect changes) | ||
| SomeProject: | ||
| > Fetched trunk - 1 | ||
| > Applying patch "patches/SomeProject.patch" | ||
| successfully patched 1/1: b'README.md' | ||
| > stage = upstream, working tree = 1 patch(es) applied — open your editor and run `svn diff` to inspect | ||
| > restored | ||
| """ | ||
| And the patched 'MySvnProject/SomeProject/README.md' is | ||
| """ | ||
| Patched file for SomeProject | ||
| """ | ||
|
|
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Assert the SVN working copy is clean after restore.
These scenarios only re-check file contents. That can still miss the bug this command is supposed to prevent: leaving local SVN modifications behind after exit. Please add an svn status assertion (or equivalent step) after each scenario so the restore guarantee is actually covered.
Also applies to: 51-69
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@features/replay-patches-in-svn.feature` around lines 33 - 50, The
replay-patches SVN scenarios currently verify file contents but not that the
working copy is clean after restore. Update the affected scenarios around the
replay-patches flow to assert the SVN status is clean after the command
finishes, using an `svn status`-based step or an equivalent existing assertion
step, so the restore guarantee is covered in the scenarios for `replay-patches`
and `MySvnProject`.
Comment on lines
+6
to
+9
| # Ensure the security package is importable when loaded by Sphinx directive | ||
| _repo_root = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) | ||
| if _repo_root not in sys.path: | ||
| sys.path.insert(0, _repo_root) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# How are the security tm_* modules invoked? Look for direct script execution,
# python -m, Sphinx directive config, or generation scripts.
rg -nP 'tm_(usage|supply_chain|common)' --glob '!security/tm_*.py' -C2
# Sphinx/pytm directive registration and conf.py
fd -t f 'conf.py' doc | xargs -r rg -nP 'pytm|security|tm_' -C2
# Any direct execution patterns
rg -nP 'python\s+security/|python\s+-m\s+security' -C2
# Confirm whether sibling tm_common.py still self-injects sys.path (inconsistency)
fd -t f 'tm_common.py' security | xargs -r rg -nP 'sys\.path|os\.path' -C2Repository: dfetch-org/dfetch
Length of output: 377
Ensure threat-model modules are imported as a package, never executed as standalone scripts.
The security/tm_usage.py and security/tm_supply_chain.py modules depend on security/__init__.py (lines 6-9) to inject the repository root into sys.path. This injection only occurs when the security package is imported (e.g., import security.tm_usage). If these modules are executed directly via python security/tm_usage.py, the __init__.py logic is skipped, leaving sys.path incomplete and causing ModuleNotFoundError for internal imports like from security.tm_render import ....
Documentation references to these modules are currently static links rather than Sphinx autodoc imports, meaning the build process does not yet trigger this bootstrap logic. However, the dependency remains fragile for local testing or future integration. The root cause is the modification of sys.path at runtime rather than proper package installation or configuration in doc/conf.py.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@security/__init__.py` around lines 6 - 9, The threat-model modules are
relying on security/__init__.py to mutate sys.path, which only works when the
package is imported and breaks if security/tm_usage.py or
security/tm_supply_chain.py are run directly. Update the entrypoints in those
modules to use package-style execution/imports only, and make the documentation
build import the package through Sphinx configuration rather than depending on
runtime path injection. If needed, move the bootstrap responsibility into
doc/conf.py or package installation setup so internal imports like
security.tm_render always resolve without executing modules as standalone
scripts.
Comment on lines
+56
to
+112
| def test_review_all_patches_calls_update_add_path_update(): | ||
| """All patches are applied and the staging area is restored for a git superproject.""" | ||
| cmd = ReplayPatches() | ||
| fake_super = _make_superproject(is_git=True) | ||
| fake_sub = _make_subproject() | ||
|
|
||
| with patch( | ||
| "dfetch.commands.replay_patches.create_super_project", return_value=fake_super | ||
| ): | ||
| with patch("dfetch.commands.command.in_directory"): | ||
| with patch( | ||
| "dfetch.commands.replay_patches.create_sub_project", | ||
| return_value=fake_sub, | ||
| ): | ||
| with patch("dfetch.commands.replay_patches.is_tty", return_value=False): | ||
| cmd(_make_args()) | ||
|
|
||
| fake_sub.update.assert_called_once_with( | ||
| force=True, | ||
| ignored_files_callback=ANY, | ||
| patch_count=0, | ||
| eol_preferences_callback=ANY, | ||
| ) | ||
| fake_super.add_path.assert_called_once_with("my_project") | ||
| fake_sub.apply_patches.assert_called_once_with(-1) | ||
| fake_super.restore_worktree.assert_not_called() | ||
| fake_super.restore_staged.assert_called_once_with("my_project") | ||
|
|
||
|
|
||
| def test_review_count_1_uses_patch_count_1(): | ||
| """--count 1 limits apply_patches to exactly one patch.""" | ||
| cmd = ReplayPatches() | ||
| fake_super = _make_superproject(is_git=True) | ||
| fake_sub = _make_subproject() | ||
|
|
||
| with patch( | ||
| "dfetch.commands.replay_patches.create_super_project", return_value=fake_super | ||
| ): | ||
| with patch("dfetch.commands.command.in_directory"): | ||
| with patch( | ||
| "dfetch.commands.replay_patches.create_sub_project", | ||
| return_value=fake_sub, | ||
| ): | ||
| with patch("dfetch.commands.replay_patches.is_tty", return_value=False): | ||
| cmd(_make_args(count=1)) | ||
|
|
||
| fake_sub.update.assert_called_once_with( | ||
| force=True, | ||
| ignored_files_callback=ANY, | ||
| patch_count=0, | ||
| eol_preferences_callback=ANY, | ||
| ) | ||
| fake_sub.apply_patches.assert_called_once_with(1) | ||
| fake_super.restore_from_head.assert_called_once_with("my_project") | ||
| fake_super.restore_worktree.assert_not_called() | ||
| fake_super.restore_staged.assert_not_called() | ||
|
|
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Add one failure-path cleanup test.
These tests only prove restoration on successful runs. The risky part of replay-patches is leaving the index/worktree dirty when apply_patches() or the interactive step raises after staging starts, and there is no regression coverage for that contract here. Add a case that forces an exception after add_path and asserts the matching restore helper still runs from a finally path.
Also applies to: 331-378
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_replay_patches.py` around lines 56 - 112, Add a failure-path
cleanup test for ReplayPatches: the current tests cover only successful
restoration, but not the case where apply_patches() or the interactive flow
raises after fake_super.add_path() has staged changes. Add a test in
test_review_all_patches_calls_update_add_path_update that makes the subproject
or tty-driven step raise, then assert the appropriate restore helper on
fake_super still runs via the ReplayPatches cleanup path (finally-style), using
the existing ReplayPatches, _make_superproject, _make_subproject, and restore_*
symbols.
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
3 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.
Introduces
dfetch replay-patchwhich stages the clean upstream source inthe git index and applies the selected patches to the working tree, so any
diff-aware editor sees
git diff(working tree vs index) showing exactlywhat the patches contribute. The command always restores original state on
exit — no permanent changes to working tree or index.
Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_017zY8BoH65KBX6cz7Pm8aeF
Summary by CodeRabbit
New Features
Bug Fixes
Documentation