Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a top-level Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as "dfetch remove"
participant SP as "SuperProject / Manifest"
participant FS as "Filesystem"
User->>CLI: run `dfetch remove <projects...>`
CLI->>SP: create_super_project()
CLI->>FS: in_directory(superproject.root_directory)
alt superproject is NoVcsSuperProject
CLI->>FS: copy manifest -> manifest.backup
end
loop for each project
CLI->>SP: manifest.selected_projects([name]) -> entry or missing
alt entry found
CLI->>SP: manifest.remove(name)
else missing
CLI->>User: print "project '<name>' not found in manifest"
end
end
CLI->>SP: superproject.manifest.dump()
SP->>FS: write updated manifest file
loop for each removed project
CLI->>FS: safe_rm(entry.destination)
CLI->>User: log "<project>: removed"
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![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: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dfetch/commands/remove.py`:
- Line 41: The log message in remove.py currently uses "Project '<project>'
removed from manifest." which doesn't match the expected output "<project>:
removed"; change the logger.info call (in the remove command function where
logger.info(f"Project '{project}' removed from manifest.") is used) to log
exactly f"{project}: removed" so the acceptance output comparison matches the
spec.
- Around line 45-46: The current code moves DEFAULT_MANIFEST_NAME to
DEFAULT_MANIFEST_NAME + ".backup" before calling manifest.dump(path), which can
leave no manifest if dump fails; instead, create a backup by copying the file
(not moving) and perform an atomic replace when writing: write the new manifest
to a temporary file alongside path and then use an atomic rename/replace
(os.replace or pathlib.Path.replace) to swap the temp into place only after
manifest.dump succeeds; keep references to DEFAULT_MANIFEST_NAME, shutil.move
(replace its usage with a copy operation), and the manifest.dump(path) call so
the sequence is: copy backup, dump to temp, atomic replace temp -> path, and
only then remove/rotate backups as needed.
- Around line 38-44: The code currently calls safe_rm(project) which deletes
using the project identifier instead of the resolved checkout path; before
calling safe_rm, resolve the project's destination from the manifest (e.g., use
the manifest's resolve/get_destination method for the given project identifier)
and pass that resolved path to safe_rm, keep manifest.remove(project) to update
the manifest, and update the logger.info message to include the actual
destination path so the deleted path is clear; ensure you use the resolved path
variable in safe_rm rather than the raw project identifier.
In `@dfetch/manifest/manifest.py`:
- Around line 250-252: RequestedProjectNotFoundError is being raised with
project_name (a str) which will be interpreted as an iterable of characters;
instead pass a sequence of missing names. Change the raise in the project lookup
(the block using self._projects, project_name and possibles) to pass a
list/sequence containing the unfound project(s) (e.g., use possibles or
[project_name]) as the unfound argument to RequestedProjectNotFoundError so it
receives a list rather than a raw string.
In `@features/steps/generic_steps.py`:
- Line 281: The Behave step decorator string is malformed: change the
placeholder from "'{path'" to the correct "'{path}'" in the `@then`(...) decorator
so the step binds (i.e., update the decorator that currently reads "the
directory '{path' should be removed from disk" to use "the directory '{path}'
should be removed from disk"); locate the decorator above the corresponding step
function in features/steps/generic_steps.py and correct the placeholder so the
function (the one decorated by this `@then`) receives the path argument.
🪄 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: c994caf4-97a1-40b3-aae0-1f16fe3b03e9
📒 Files selected for processing (5)
dfetch/__main__.pydfetch/commands/remove.pydfetch/manifest/manifest.pyfeatures/remove-project.featurefeatures/steps/generic_steps.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
dfetch/manifest/manifest.py (1)
325-331:⚠️ Potential issue | 🟡 MinorPass
project_nameas a list toRequestedProjectNotFoundError
RequestedProjectNotFoundError.__init__expectsunfound: Sequence[str]. Passing a bare string causes iteration over characters instead of names.🔧 Proposed fix
def remove(self, project_name: str) -> None: """Remove a project from the manifest.""" if project_name not in self._projects: possibles = [project.name for project in self._projects.values()] - raise RequestedProjectNotFoundError(project_name, possibles) + raise RequestedProjectNotFoundError([project_name], possibles) del self._projects[project_name]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dfetch/manifest/manifest.py` around lines 325 - 331, In remove, RequestedProjectNotFoundError expects a sequence of unfound names but you pass a bare string; change the call in the remove method (function name: remove) to pass project_name as a one-element sequence (e.g., [project_name] or (project_name,)) instead of a plain string, preserving the existing possibles list generation and raising RequestedProjectNotFoundError(project_name_as_sequence, possibles).features/steps/generic_steps.py (1)
462-465:⚠️ Potential issue | 🔴 CriticalFix malformed Behave step placeholder
The step decorator has a broken placeholder
'{path'instead of'{path}'. This prevents the step from binding to feature scenarios.🔧 Proposed fix
-@then("the directory '{path' should be removed from disk") +@then("the directory '{path}' should be removed from disk") def step_impl(_, path): """Check if a directory is removed.""" assert not os.path.exists(path), f"Directory {path} still exists!"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/steps/generic_steps.py` around lines 462 - 465, The Behave step decorator string in the `@then` on function step_impl uses a malformed placeholder "'{path" which prevents binding; update the decorator pattern to use the correct placeholder format "{path}" in the `@then`("the directory '{path}' should be removed from disk") so the step_impl function binds to feature scenarios (function name: step_impl, decorator: `@then`(...)).dfetch/commands/remove.py (2)
41-41:⚠️ Potential issue | 🟡 MinorLog message format doesn't match feature expectation
The feature file expects output
ext/test-repo-tag: removedbut this logsProject 'ext/test-repo-tag' removed from manifest.🔧 Proposed fix
- logger.info(f"Project '{project}' removed from manifest.") + logger.info(f"{project}: removed")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dfetch/commands/remove.py` at line 41, Change the logger message in the removal flow so it matches the feature expectation format; replace the current logger.info call that uses logger.info(f"Project '{project}' removed from manifest.") with a message that prints "{project}: removed" (use the existing variable project and the same logger.info invocation) so output becomes e.g. ext/test-repo-tag: removed.
38-46:⚠️ Potential issue | 🟠 MajorMultiple issues with removal flow need addressing
Several issues flagged in previous reviews remain unresolved:
Inconsistent state on partial failure: If
manifest.remove()raisesRequestedProjectNotFoundErrorfor a later project, earlier directories are already deleted but the manifest hasn't been updated.Wrong path for deletion:
safe_rm(project)uses the CLI argument (project name), not the resolveddstpath from the manifest. Projects with custom destinations will leave their actual directories behind.Unsafe backup strategy:
shutil.move()removes the manifest beforedump()succeeds. A crash during dump leaves no valid manifest.🔧 Proposed fix addressing all issues
def __call__(self, args: argparse.Namespace) -> None: """Perform the remove action.""" manifest, path = get_manifest() with in_directory(os.path.dirname(path)): + # Phase 1: Validate all projects exist and collect paths + targets = [] for project in args.projects: + selected = manifest.selected_projects([project])[0] + remove_path = selected.destination or selected.name + targets.append((project, remove_path)) - manifest.remove(project) - logger.info(f"Project '{project}' removed from manifest.") + # Phase 2: Remove directories + for _, remove_path in targets: + safe_rm(remove_path) - safe_rm(project) + # Phase 3: Update manifest + for project, _ in targets: + manifest.remove(project) + logger.info(f"{project}: removed") - shutil.move(DEFAULT_MANIFEST_NAME, DEFAULT_MANIFEST_NAME + ".backup") - manifest.dump(path) + # Phase 4: Safe write with backup + backup_path = DEFAULT_MANIFEST_NAME + ".backup" + shutil.copy2(DEFAULT_MANIFEST_NAME, backup_path) + tmp_path = path + ".tmp" + manifest.dump(tmp_path) + os.replace(tmp_path, path)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dfetch/commands/remove.py` around lines 38 - 46, The removal flow must: 1) resolve and validate all target projects from the manifest first (use the manifest API to lookup each project's resolved dst before touching disk) and fail fast on RequestedProjectNotFoundError so no partial deletions occur; 2) avoid deleting directories using the CLI name—use the resolved dst paths you looked up; and 3) make manifest updates atomic by writing/dumping the new manifest to a temp file and only replacing DEFAULT_MANIFEST_NAME after dump succeeds (do not shutil.move the original away first; use a safe backup/copy or atomic replace), then delete the resolved dst directories (call safe_rm on the resolved dsts) after the manifest has been successfully persisted; update code around manifest.remove, safe_rm, DEFAULT_MANIFEST_NAME and manifest.dump accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dfetch/commands/remove.py`:
- Around line 24-30: The argument parser for the "projects" positional in
dfetch.commands.remove currently uses nargs="*" which allows zero projects;
change it to nargs="+" to require at least one project so dfetch remove fails
when no targets are given. Locate the parser.add_argument call that defines
"projects" in remove.py and replace nargs="*" with nargs="+" (you can keep the
metavar, type and help as-is) to ensure at least one project is provided.
In `@features/remove-project.feature`:
- Around line 10-12: The test currently assumes project name equals destination
so it doesn't catch the bug in remove.py where the code calls safe_rm(project)
instead of deleting the resolved destination; update remove.py to call
safe_rm(resolved_dst) (or whatever variable holds the resolved path, e.g.,
project.dst or project.get_destination()) inside the remove logic (e.g., the
function handling removal), and add a new scenario in
features/remove-project.feature that defines a project with an explicit dst:
different from name (and asserts that the dst directory is removed) to verify
correct behavior.
---
Duplicate comments:
In `@dfetch/commands/remove.py`:
- Line 41: Change the logger message in the removal flow so it matches the
feature expectation format; replace the current logger.info call that uses
logger.info(f"Project '{project}' removed from manifest.") with a message that
prints "{project}: removed" (use the existing variable project and the same
logger.info invocation) so output becomes e.g. ext/test-repo-tag: removed.
- Around line 38-46: The removal flow must: 1) resolve and validate all target
projects from the manifest first (use the manifest API to lookup each project's
resolved dst before touching disk) and fail fast on
RequestedProjectNotFoundError so no partial deletions occur; 2) avoid deleting
directories using the CLI name—use the resolved dst paths you looked up; and 3)
make manifest updates atomic by writing/dumping the new manifest to a temp file
and only replacing DEFAULT_MANIFEST_NAME after dump succeeds (do not shutil.move
the original away first; use a safe backup/copy or atomic replace), then delete
the resolved dst directories (call safe_rm on the resolved dsts) after the
manifest has been successfully persisted; update code around manifest.remove,
safe_rm, DEFAULT_MANIFEST_NAME and manifest.dump accordingly.
In `@dfetch/manifest/manifest.py`:
- Around line 325-331: In remove, RequestedProjectNotFoundError expects a
sequence of unfound names but you pass a bare string; change the call in the
remove method (function name: remove) to pass project_name as a one-element
sequence (e.g., [project_name] or (project_name,)) instead of a plain string,
preserving the existing possibles list generation and raising
RequestedProjectNotFoundError(project_name_as_sequence, possibles).
In `@features/steps/generic_steps.py`:
- Around line 462-465: The Behave step decorator string in the `@then` on function
step_impl uses a malformed placeholder "'{path" which prevents binding; update
the decorator pattern to use the correct placeholder format "{path}" in the
`@then`("the directory '{path}' should be removed from disk") so the step_impl
function binds to feature scenarios (function name: step_impl, decorator:
`@then`(...)).
🪄 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: bbc1428a-051a-4e31-8189-25538334592f
📒 Files selected for processing (5)
dfetch/__main__.pydfetch/commands/remove.pydfetch/manifest/manifest.pyfeatures/remove-project.featurefeatures/steps/generic_steps.py
Comment on lines
+10
to
+12
| - name: ext/test-repo-tag | ||
| url: https://github.com/dfetch-org/test-repo | ||
| tag: v1 |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Test assumes project name equals destination path
The test uses ext/test-repo-tag as both the project name and the expected directory path. However, the remove.py implementation currently uses the project name argument directly with safe_rm(project) rather than the resolved destination. While this test will pass, it doesn't verify correct behavior when dst differs from name.
Consider adding a scenario where the project has an explicit dst: that differs from name to ensure the correct path is deleted.
Also applies to: 16-16, 24-24
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@features/remove-project.feature` around lines 10 - 12, The test currently
assumes project name equals destination so it doesn't catch the bug in remove.py
where the code calls safe_rm(project) instead of deleting the resolved
destination; update remove.py to call safe_rm(resolved_dst) (or whatever
variable holds the resolved path, e.g., project.dst or
project.get_destination()) inside the remove logic (e.g., the function handling
removal), and add a new scenario in features/remove-project.feature that defines
a project with an explicit dst: different from name (and asserts that the dst
directory is removed) to verify correct behavior.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
dfetch/manifest/manifest.py (1)
325-329:⚠️ Potential issue | 🟡 MinorWrap
project_namein a list when raisingRequestedProjectNotFoundError.
project_nameis astr, so it is treated as a sequence of characters in the exception logic.Proposed fix
- raise RequestedProjectNotFoundError(project_name, possibles) + raise RequestedProjectNotFoundError([project_name], possibles)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dfetch/manifest/manifest.py` around lines 325 - 329, The remove method treats project_name (a str) as an iterable when constructing the RequestedProjectNotFoundError; modify the raise to pass project_name as a single-element list so the exception receives a list (e.g. change raise RequestedProjectNotFoundError(project_name, possibles) to raise RequestedProjectNotFoundError([project_name], possibles)); update the raise in def remove(self, project_name: str) to wrap project_name in a list so the error handling and any downstream logic expecting a sequence of candidates behaves correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dfetch/commands/remove.py`:
- Around line 52-53: The backup is created unconditionally when make_backup is
true even if no projects are passed; update the logic in remove.py (where
make_backup, manifest_path and shutil.copyfile are used) so the backup is only
created when there are projects to remove—either change the CLI to require
projects (nargs="+" in the argument definition) or wrap/move the
shutil.copyfile(manifest_path, manifest_path + ".backup") behind a check for
args.projects (or into the loop that processes each project) so no backup is
made when the project list is empty.
- Line 67: The inline comment saying "manifest_updated is set but not used" is
stale; update or remove it to reflect that manifest_updated is actually checked
to call update_dump() (see manifest_updated and update_dump()). Edit the comment
near the manifest_updated variable so it either explains its conditional use
(used to decide whether to call update_dump()) or delete the incorrect remark.
- Around line 57-59: The check for "if not project_entries" is dead because
superproject.manifest.selected_projects(...) raises
RequestedProjectNotFoundError via _check_all_names_found; remove the unreachable
ValueError branch or explicitly catch RequestedProjectNotFoundError and re-raise
a custom error. Locate the call to selected_projects in remove.py (variable
project_entries) and either delete the if-not block so the upstream
RequestedProjectNotFoundError propagates, or wrap the selected_projects call in
a try/except catching RequestedProjectNotFoundError and raise your desired
ValueError/message instead.
In `@doc/generate-casts/remove-demo.sh`:
- Around line 1-10: The script remove-demo.sh doesn't fail fast or guarantee
cleanup on directory-change failures; update it to enable strict bash options
(e.g., set -euo pipefail) and add a cleanup trap that always runs on EXIT to
remove the temporary "remove" directory and safely popd; also guard mkdir and
pushd (or check their success) so failures trigger exit and the trap will run.
Specifically, modify the top of remove-demo.sh to enable strict mode, add a trap
handler that does "popd" safely (suppress errors) and removes the created
"remove" directory, and ensure the mkdir/remove/pushd/popd usage in the script
(the mkdir remove, pushd remove, and final popd) rely on the trap rather than
unconditional assumptions so cleanup always occurs even if mkdir or pushd fail.
In `@tests/test_manifest.py`:
- Around line 390-405: The test
test_remove_last_project_updates_manifest_with_empty_list currently asserts
exact YAML string equality which is fragile; instead, after creating the
Manifest via Manifest.from_yaml(...) and calling manifest.remove("myproject"),
assert the object model: check manifest.version (or manifest.manifest.version if
applicable) equals "0.0" and that manifest.projects is an empty list (e.g.,
len(manifest.projects) == 0 or manifest.projects == []), rather than comparing
manifest._doc.as_yaml() to a hard-coded YAML string; update the assertions to
verify these attributes and remove the fragile string equality.
In `@tests/test_remove.py`:
- Around line 12-67: Add unit tests to cover the two edge cases: (1) when a
requested project name is not present in the manifest, call Remove() with args =
argparse.Namespace(projects=["nonexistent"]) using the same patched helpers
(create_super_project, in_directory, safe_rm, shutil.copyfile) and assert that
calling remove(args) raises ValueError and that manifest.remove/update_dump are
not called; (2) when the projects list is empty, call Remove() with args =
argparse.Namespace(projects=[]) and assert the expected behavior (e.g., no-op):
patch create_super_project to return a fake_superproject/NoVcsSuperProject, run
remove(args), and assert that manifest.remove and safe_rm/shutil.copyfile were
not called (or assert specific behavior your CLI should enforce). Use the
existing helpers mock_manifest, NoVcsSuperProject, Remove, and the same patched
symbols ("dfetch.commands.remove.create_super_project",
"dfetch.commands.remove.in_directory", "dfetch.commands.remove.safe_rm",
"dfetch.commands.remove.shutil.copyfile") to locate the code under test.
---
Duplicate comments:
In `@dfetch/manifest/manifest.py`:
- Around line 325-329: The remove method treats project_name (a str) as an
iterable when constructing the RequestedProjectNotFoundError; modify the raise
to pass project_name as a single-element list so the exception receives a list
(e.g. change raise RequestedProjectNotFoundError(project_name, possibles) to
raise RequestedProjectNotFoundError([project_name], possibles)); update the
raise in def remove(self, project_name: str) to wrap project_name in a list so
the error handling and any downstream logic expecting a sequence of candidates
behaves correctly.
🪄 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: 0e17c50f-3b41-4d75-af83-6ebcb10b7f06
📒 Files selected for processing (14)
CHANGELOG.rstdfetch/__main__.pydfetch/commands/remove.pydfetch/manifest/manifest.pydoc/asciicasts/remove.castdoc/generate-casts/generate-casts.shdoc/generate-casts/remove-demo.shdoc/howto/remove-a-project.rstdoc/index.rstdoc/reference/commands.rstfeatures/remove-project.featurefeatures/steps/generic_steps.pytests/test_manifest.pytests/test_remove.py
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
doc/generate-casts/remove-demo.sh (1)
1-10:⚠️ Potential issue | 🟠 MajorHarden script failure paths and guaranteed cleanup.
pushd/popdare unchecked and cleanup is not guaranteed on early failure, so cast generation can leave stale state.Proposed fix
#!/usr/bin/env bash +set -euo pipefail source ./demo-magic/demo-magic.sh PROMPT_TIMEOUT=1 # Copy example manifest and fetch projects -mkdir remove -pushd remove +trap 'popd >/dev/null 2>&1 || true; rm -rf -- remove' EXIT +mkdir -p remove +pushd remove >/dev/null || exit 1 @@ -popd -rm -rf remove +# cleanup handled by EXIT trapUse this read-only check to verify hardening is in place:
#!/usr/bin/env bash rg -n 'set -euo pipefail|trap .*EXIT|mkdir -p remove|pushd remove >/dev/null \|\| exit 1' doc/generate-casts/remove-demo.shAlso applies to: 27-28
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/generate-casts/remove-demo.sh` around lines 1 - 10, Add robust failure handling and guaranteed cleanup to the remove-demo.sh script: enable strict mode (set -euo pipefail) at the top, create the directory with mkdir -p remove, guard pushd with a failure exit (pushd remove >/dev/null || exit 1), and install a trap on EXIT to always popd (e.g., trap 'popd >/dev/null || true' EXIT) so the working directory is restored even on errors; update any matching pushd/popd invocations (including lines ~27-28) to follow this pattern.dfetch/commands/remove.py (1)
52-66:⚠️ Potential issue | 🟠 MajorAvoid partial multi-project state corruption on mid-loop failure.
Current order can delete earlier directories but never persist manifest changes if a later project lookup/delete fails, and it can log
removedbefore deletion succeeds.Proposed fix (pre-validate targets, persist manifest once, then delete+log)
with in_directory(superproject.root_directory): manifest_path = superproject.manifest.path - manifest_updated = False + targets: list[tuple[str, str]] = [] for project in args.projects: - # Get the project entry before removing it to get the destination - project_entries = superproject.manifest.selected_projects([project]) - destination = project_entries[0].destination - - if make_backup and not manifest_updated: - shutil.copyfile(manifest_path, manifest_path + ".backup") - - superproject.manifest.remove(project) - logger.print_info_line(project, "removed") - safe_rm(destination) - manifest_updated = True - - if manifest_updated: - superproject.manifest.update_dump() + entry = superproject.manifest.selected_projects([project])[0] + targets.append((project, entry.destination)) + + if make_backup and targets: + shutil.copyfile(manifest_path, manifest_path + ".backup") + + for project, _ in targets: + superproject.manifest.remove(project) + + if targets: + superproject.manifest.update_dump() + + for project, destination in targets: + safe_rm(destination) + logger.print_info_line(project, "removed")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dfetch/commands/remove.py` around lines 52 - 66, The loop in remove.py can leave the repo in a partially-deleted state because it removes and logs projects one-by-one before persisting the manifest; instead pre-validate all requested projects by calling superproject.manifest.selected_projects for each entry and collect their destinations, perform the backup (shutil.copyfile(manifest_path, manifest_path + ".backup")) once if make_backup and any project will be removed, then remove all projects from the manifest (superproject.manifest.remove) in-memory, call superproject.manifest.update_dump() once to persist the manifest change, and only after successful persistence perform safe_rm(destination) and logger.print_info_line(project, "removed") for each destination; ensure manifest_updated is set appropriately and errors during validation or update abort before any filesystem deletions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@dfetch/commands/remove.py`:
- Around line 52-66: The loop in remove.py can leave the repo in a
partially-deleted state because it removes and logs projects one-by-one before
persisting the manifest; instead pre-validate all requested projects by calling
superproject.manifest.selected_projects for each entry and collect their
destinations, perform the backup (shutil.copyfile(manifest_path, manifest_path +
".backup")) once if make_backup and any project will be removed, then remove all
projects from the manifest (superproject.manifest.remove) in-memory, call
superproject.manifest.update_dump() once to persist the manifest change, and
only after successful persistence perform safe_rm(destination) and
logger.print_info_line(project, "removed") for each destination; ensure
manifest_updated is set appropriately and errors during validation or update
abort before any filesystem deletions.
In `@doc/generate-casts/remove-demo.sh`:
- Around line 1-10: Add robust failure handling and guaranteed cleanup to the
remove-demo.sh script: enable strict mode (set -euo pipefail) at the top, create
the directory with mkdir -p remove, guard pushd with a failure exit (pushd
remove >/dev/null || exit 1), and install a trap on EXIT to always popd (e.g.,
trap 'popd >/dev/null || true' EXIT) so the working directory is restored even
on errors; update any matching pushd/popd invocations (including lines ~27-28)
to follow this pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e5b924ca-8732-41cb-aea5-fb548fd79e17
📒 Files selected for processing (15)
CHANGELOG.rstdfetch/__main__.pydfetch/commands/remove.pydfetch/manifest/manifest.pydoc/asciicasts/remove.castdoc/generate-casts/generate-casts.shdoc/generate-casts/remove-demo.shdoc/howto/remove-a-project.rstdoc/index.rstdoc/reference/commands.rstfeatures/remove-project.featurefeatures/steps/generic_steps.pytests/manifest_mock.pytests/test_manifest.pytests/test_remove.py
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Adapt to the manifest refactor on main: _projects is no longer an instance attribute (use self.projects instead) and update_dump was renamed to dump. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
doc/generate-casts/remove-demo.sh (1)
1-10:⚠️ Potential issue | 🟠 MajorHarden script execution and cleanup path (
pushd/popd+ temp dir lifecycle).The script still assumes directory stack operations always succeed and cleanup always reaches the end. Please enforce strict mode and cleanup via trap to avoid leaking
remove/or running in wrong cwd on failure.Proposed fix
#!/usr/bin/env bash +set -euo pipefail source ./demo-magic/demo-magic.sh +WORKDIR="remove" +cleanup() { + popd >/dev/null 2>&1 || true + rm -rf "$WORKDIR" +} +trap cleanup EXIT + PROMPT_TIMEOUT=1 # Copy example manifest and fetch projects -mkdir remove -pushd remove +mkdir -p "$WORKDIR" +pushd "$WORKDIR" >/dev/null || exit 1 @@ PROMPT_TIMEOUT=3 wait pei "" - -popd -rm -rf removeRun this read-only verification after applying the fix:
#!/usr/bin/env bash rg -n 'set -euo pipefail|trap cleanup EXIT|mkdir -p|pushd .*\\|\\| exit|cleanup\\(|popd >/dev/null' doc/generate-casts/remove-demo.shAlso applies to: 22-28
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/generate-casts/remove-demo.sh` around lines 1 - 10, Enable strict mode and robust cleanup in remove-demo.sh: add "set -euo pipefail" at the top, create a cleanup() function that removes the temporary "remove" directory and restores the cwd via "popd >/dev/null" if pushd succeeded, register it with "trap cleanup EXIT", and change "mkdir remove" to "mkdir -p remove" and "pushd remove" to "pushd remove || exit" so failures don't leave you in the wrong directory; ensure all references to the temp dir and directory stack use the same cleanup logic.dfetch/commands/remove.py (1)
54-73:⚠️ Potential issue | 🟡 MinorDeduplicate requested project names before removal.
At Line 54, iterating raw
args.projectsallows duplicates (dfetch remove a a). Both entries are prevalidated, but the second removal at Line 73 raisesRequestedProjectNotFoundErrorbecause the first removal already deleted it.🔧 Proposed fix
- # Pre-validate all projects and collect their destinations - projects_to_remove = [] - for project in args.projects: + # Pre-validate all unique projects (preserve CLI order) and collect destinations + projects_to_remove = [] + requested_projects = list(dict.fromkeys(args.projects)) + for project in requested_projects: try: project_entries = superproject.manifest.selected_projects([project]) destination = project_entries[0].destination projects_to_remove.append((project, destination)) except RequestedProjectNotFoundError:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dfetch/commands/remove.py` around lines 54 - 73, The code fails when duplicate project names are passed because args.projects is iterated raw and the second removal raises RequestedProjectNotFoundError after the first removal; to fix, deduplicate the requested names before validating and building projects_to_remove (e.g., by converting args.projects to an order-preserving unique list or set) so that duplicate entries are only processed once, then proceed with the existing logic that creates a backup (manifest_path, make_backup) and calls superproject.manifest.remove for each unique project.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/manifest_mock.py`:
- Around line 23-35: The mock_selected_projects function currently validates
against the raw names list and can falsely raise RequestedProjectNotFoundError
for duplicate inputs; change it to deduplicate the incoming names first
(preserving order) into a variable like deduped_names (e.g., via
list(dict.fromkeys(names))) then use deduped_names for filtering against
project_mocks, for the length comparison, and when computing unfound and
possibles before raising RequestedProjectNotFoundError; keep project_mocks and
the RequestedProjectNotFoundError usage the same but operate on deduped_names
everywhere.
---
Duplicate comments:
In `@dfetch/commands/remove.py`:
- Around line 54-73: The code fails when duplicate project names are passed
because args.projects is iterated raw and the second removal raises
RequestedProjectNotFoundError after the first removal; to fix, deduplicate the
requested names before validating and building projects_to_remove (e.g., by
converting args.projects to an order-preserving unique list or set) so that
duplicate entries are only processed once, then proceed with the existing logic
that creates a backup (manifest_path, make_backup) and calls
superproject.manifest.remove for each unique project.
In `@doc/generate-casts/remove-demo.sh`:
- Around line 1-10: Enable strict mode and robust cleanup in remove-demo.sh: add
"set -euo pipefail" at the top, create a cleanup() function that removes the
temporary "remove" directory and restores the cwd via "popd >/dev/null" if pushd
succeeded, register it with "trap cleanup EXIT", and change "mkdir remove" to
"mkdir -p remove" and "pushd remove" to "pushd remove || exit" so failures don't
leave you in the wrong directory; ensure all references to the temp dir and
directory stack use the same cleanup logic.
🪄 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: 25913754-5ee0-49af-9986-815c6d3cb27f
📒 Files selected for processing (16)
CHANGELOG.rstdfetch/__main__.pydfetch/commands/remove.pydfetch/manifest/manifest.pydoc/asciicasts/remove.castdoc/generate-casts/generate-casts.shdoc/generate-casts/remove-demo.shdoc/howto/remove-a-project.rstdoc/index.rstdoc/reference/commands.rstfeatures/remove-project.featurefeatures/steps/generic_steps.pytests/manifest_mock.pytests/test_add.pytests/test_manifest.pytests/test_remove.py
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
doc/generate-casts/remove-demo.sh (1)
1-28:⚠️ Potential issue | 🟡 MinorHarden script flow around directory changes and cleanup.
The script should fail fast and safely handle directory navigation failures. The static analysis warnings (SC2164) for
pushd/popdwithout error handling remain unaddressed from the previous review.Additionally, the
waitcommand on line 23 has no effect since no background processes are spawned.Proposed fix
#!/usr/bin/env bash +set -euo pipefail source ./demo-magic/demo-magic.sh PROMPT_TIMEOUT=1 # Copy example manifest and fetch projects -mkdir remove -pushd remove +mkdir -p remove +pushd remove >/dev/null || exit 1 dfetch init dfetch update clear # Run the command pe "cat dfetch.yaml" pe "ls -la cpputest/src" pe "dfetch remove cpputest" pe "cat dfetch.yaml" pe "ls -la cpputest" PROMPT_TIMEOUT=3 -wait pei "" -popd +popd >/dev/null || exit 1 rm -rf remove,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/generate-casts/remove-demo.sh` around lines 1 - 28, Add strict error handling and a safe cleanup trap: enable "set -euo pipefail" at the top, replace bare pushd/popd with guarded calls (e.g., use pushd remove || exit 1 and ensure popd is run only if pushd succeeded), remove the no-op "wait" since there are no background jobs, and install a trap that always runs cleanup (popd if inside the dir and rm -rf remove) on EXIT to guarantee directory restoration and removal even on failure; update references in this script to these guarded operations (pushd/popd, rm -rf remove, PROMPT_TIMEOUT usage) so flow fails fast and safely.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dfetch/manifest/manifest.py`:
- Around line 484-507: Silent return from update_project_version when
_find_doc_project(project.name) is None hides errors; make it fail like remove()
and find_name_in_manifest(). Modify update_project_version to detect p is None
and raise a clear exception (e.g., RequestedProjectNotFoundError or
RuntimeError) with a message including the project.name; keep the existing
branch behavior (mu = p etc.) unchanged, only replace the early return with the
exception to match the explicit failure pattern used by remove() and
find_name_in_manifest().
In `@tests/test_remove.py`:
- Around line 75-103: The test test_remove_nonexistent_project_logs_error
currently checks that no manifest or filesystem changes occur but fails to
assert the expected log; patch or mock
dfetch.commands.remove.logger.print_info_line around the Remove() call and add
an assertion that logger.print_info_line was called once with a message
indicating the missing project (i.e., containing "nonexistent" or an explicit
"project not found" text) to verify the missing-project log output from Remove.
- Around line 135-182: The test test_remove_multiple_projects_atomically only
asserts counts/arguments but not sequencing; update it to assert the exact call
order (validate selected_projects calls, then manifest.remove calls, then
manifest.dump, then file deletions and backups) by inspecting the
fake_superproject/fake_manifest and patched function call histories (e.g.,
fake_manifest.method_calls or mocked objects' method_calls and
mocked_safe_rm/mocked_copyfile.mock_calls) and use assert_has_calls or direct
comparison with an ordered list of expected calls (with any_order=False) to
ensure the sequence: selected_projects("project1"),
selected_projects("project2"), selected_projects("project3"),
remove("project1"), remove("project3"), dump(), then safe_rm(...) calls, and
finally copyfile if applicable; reference the test function name, Remove(),
fake_manifest.selected_projects, fake_manifest.remove, fake_manifest.dump,
mocked_safe_rm, and mocked_copyfile when making these assertions.
---
Duplicate comments:
In `@doc/generate-casts/remove-demo.sh`:
- Around line 1-28: Add strict error handling and a safe cleanup trap: enable
"set -euo pipefail" at the top, replace bare pushd/popd with guarded calls
(e.g., use pushd remove || exit 1 and ensure popd is run only if pushd
succeeded), remove the no-op "wait" since there are no background jobs, and
install a trap that always runs cleanup (popd if inside the dir and rm -rf
remove) on EXIT to guarantee directory restoration and removal even on failure;
update references in this script to these guarded operations (pushd/popd, rm -rf
remove, PROMPT_TIMEOUT usage) so flow fails fast and safely.
🪄 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: e84d7143-797e-4cb0-a888-da00c3b06f27
📒 Files selected for processing (16)
CHANGELOG.rstdfetch/__main__.pydfetch/commands/remove.pydfetch/manifest/manifest.pydoc/asciicasts/remove.castdoc/generate-casts/generate-casts.shdoc/generate-casts/remove-demo.shdoc/howto/remove-a-project.rstdoc/index.rstdoc/reference/commands.rstfeatures/remove-project.featurefeatures/steps/generic_steps.pytests/manifest_mock.pytests/test_add.pytests/test_manifest.pytests/test_remove.py
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary by CodeRabbit
New Features
Bug Fixes / Validation
Documentation
Tests