Implement selective and in-place freeze (#1063)#1102
Open
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 optional project-name args to Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant CLI as "dfetch CLI"
participant SP as "SuperProject"
participant Manifest as "Manifest / YamlDocument"
participant FS as "Filesystem"
User->>CLI: run `dfetch freeze [projects...]`
CLI->>SP: open superproject & load manifest
CLI->>SP: call manifest.selected_projects(args.projects)
loop per selected project
CLI->>SP: freeze_project(project) -> new_version?
alt new_version produced
CLI->>Manifest: update_project_version(project)
Manifest->>Manifest: modify YAML AST (revision/tag/branch/integrity)
alt SP is NoVcsSuperProject
CLI->>FS: copy manifest -> manifest.backup
end
Manifest->>FS: update_dump() -> write updated manifest text
end
end
CLI-->>User: report updated manifest path / exit
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 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: 4
🤖 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 681-748: The current logic only adds/updates version fields but
never removes stale ones; update _apply_block_updates (used by
_update_project_version_in_text) to remove any existing "revision", "tag", and
"branch" lines from the block when they are not present in fields_to_set, and to
remove the integrity.hash entry when integrity_hash is empty (adjust or extend
_set_integrity_hash_in_block or add a _remove_integrity_hash_in_block helper to
perform the deletion). Locate and change behavior around _find_field,
_update_value, and the integrity handling so that missing keys in the project
YAML result in the corresponding lines being deleted from the block before
inserting/updating remaining fields.
- Around line 517-569: The code currently matches the first bare "- name:
<project>" anywhere in the file and doesn't accept quoted YAML scalars; update
_find_project_block and _locate_project_name_line so they only search inside the
"projects:" list and accept name values quoted with single or double quotes.
Concretely: first scan for the "projects:" key and compute its block
range/indentation, then limit the search in _locate_project_name_line to lines
inside that block; change the regex in _locate_project_name_line to allow
optional surrounding single or double quotes around the name value (e.g.
\s*(?P<q>['"]?)(?P<name>...)(?P=q)\s*) while still using re.escape(project_name)
when building the pattern; ensure _find_project_block calls the updated locator
and still returns (start, end, item_indent).
- Around line 594-620: Both helpers currently rebuild lines as "field: value\n"
losing any inline comment/trailing whitespace and forcing LF; update
_update_value and _append_field (and the similar code around the other
occurrence at the same file) to preserve the original line suffix and line
ending: in _update_value keep the original line's trailing content after the
value (including inline comments/whitespace) and reuse its exact EOL when
replacing the line, and in _append_field choose the EOL style by inspecting
neighboring lines (or a provided line's EOL) so inserted lines use the same
CR/LF style and do not drop comments/whitespace semantics when yaml_value is
empty or present.
In `@features/freeze-inplace.feature`:
- Around line 35-68: The selective in-place freeze scenario needs the same "no
.backup" invariant as the all-projects path: after running the command in the
"Only selected project is frozen in-place" scenario, add an assertion that no
".backup" file exists (e.g., no dfetch.yaml.backup or any ".backup" artifact) in
the repo; this ensures the selected_projects(args.projects) code path performs
in-place edits without falling back to backup+rewrite.
🪄 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: c048f3f0-d3b6-4dc0-aa30-5db8c49ac01c
📒 Files selected for processing (7)
CHANGELOG.rstdfetch/commands/freeze.pydfetch/manifest/manifest.pyfeatures/freeze-inplace.featurefeatures/freeze-specific-projects.featurefeatures/steps/manifest_steps.pytests/test_manifest.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
dfetch/manifest/manifest.py (2)
655-676:⚠️ Potential issue | 🟠 MajorStale version fields are not removed during in-place freeze.
_apply_block_updatesonly adds/updates fields present infields_to_setbut never removes existingrevision/tag/branchlines that are no longer in the frozen project. If a user switches pinning mode (e.g., fromtagtorevision), the old field remains, causing contradictory version state that diverges from the backup+dump path.🔧 Suggested approach
Before applying updates, remove any existing version fields (
revision,tag,branch) that are not infields_to_set:def _apply_block_updates(...) -> list[str]: # Remove stale version fields first fields_to_keep = {f for f, _ in fields_to_set} for field in ("revision", "tag", "branch"): if field not in fields_to_keep: idx = _find_field(block, field, field_indent) if idx is not None: del block[idx] # Then apply updates as before...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dfetch/manifest/manifest.py` around lines 655 - 676, _in _apply_block_updates remove stale version fields before inserting/updating: compute fields_to_keep = {f for f,_ in fields_to_set} and for each of ("revision","tag","branch") if the field is not in fields_to_keep call _find_field(block, field, field_indent) and del block[idx] when found, then proceed with the existing insert/update logic and finally call _set_integrity_hash_in_block as before so stale version lines are not left behind after a pinning-mode change.
518-541:⚠️ Potential issue | 🟠 MajorProject name search scope and quoted scalars are not handled.
_locate_project_name_linesearches the entire file for- name: <value>and doesn't:
- Limit the search to the
projects:section — aremotes:entry with the same name would match first- Handle YAML-quoted names like
name: 'myproject'orname: "176"This can cause in-place freeze to target the wrong block or fail on valid manifests with quoted project names.
🔧 Suggested approach
Update the regex to optionally match single/double quotes around the name value:
pattern = re.compile( - r"^(?P<indent>\s*)-\s*name:\s*(?P<name>" + r"^(?P<indent>\s*)-\s*name:\s*(?P<quote>['\"]?)(?P<name>" + re.escape(project_name) - + r")\s*(?:#.*)?$" + + r")(?P=quote)\s*(?:#.*)?$" )For scope limitation, consider first locating the
projects:key and its indentation, then only searching within that range.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dfetch/manifest/manifest.py` around lines 518 - 541, _locate_project_name_line currently scans the whole file and uses a regex that only matches unquoted scalars; update it to first find the projects: mapping block (locate the "projects:" line and note its indentation, then limit subsequent search to lines indented under that block until a same-or-less-indented non-blank non-comment line ends the block) and change the regex (the pattern variable) to accept optional single or double quotes around the name value (allowing name: myproj, name: 'myproj', and name: "myproj") while preserving capture groups for indent and name so the function still returns (line_idx, item_indent, name_col_start, name_col_end).
🤖 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 670-671: The code under the "if inserted:" branch constructs lines
with hardcoded "\n" and inserts them directly into the block, bypassing the
existing formatting helper; replace that manual insertion by iterating over the
inserted list and calling the existing _append_field helper for each (f, v) so
formatting/line endings and duplication logic are centralized; ensure you call
_append_field with the current block and the same field_indent/context used
elsewhere (preserve insertion order or insert in reverse if _append_field
mutates the block at the insertion index) and remove the hardcoded string
construction.
In `@dfetch/util/yaml.py`:
- Around line 45-61: The code in update_value only preserves '\n' eol, so CRLF
lines become partially converted; change update_value to detect and preserve
"\r\n" or "\n" (e.g., set eol = "\r\n" if line.endswith("\r\n") elif
line.endswith("\n") else ""), and use that eol when writing back; also update
append_field to choose the EOL for appended lines consistently (for example,
inspect the last line of the block to detect its eol and use that instead of
always appending "\n"), referencing the functions update_value and append_field
so the fix is applied where line endings are constructed.
---
Duplicate comments:
In `@dfetch/manifest/manifest.py`:
- Around line 655-676: _in _apply_block_updates remove stale version fields
before inserting/updating: compute fields_to_keep = {f for f,_ in fields_to_set}
and for each of ("revision","tag","branch") if the field is not in
fields_to_keep call _find_field(block, field, field_indent) and del block[idx]
when found, then proceed with the existing insert/update logic and finally call
_set_integrity_hash_in_block as before so stale version lines are not left
behind after a pinning-mode change.
- Around line 518-541: _locate_project_name_line currently scans the whole file
and uses a regex that only matches unquoted scalars; update it to first find the
projects: mapping block (locate the "projects:" line and note its indentation,
then limit subsequent search to lines indented under that block until a
same-or-less-indented non-blank non-comment line ends the block) and change the
regex (the pattern variable) to accept optional single or double quotes around
the name value (allowing name: myproj, name: 'myproj', and name: "myproj") while
preserving capture groups for indent and name so the function still returns
(line_idx, item_indent, name_col_start, name_col_end).
🪄 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: d0fcc303-cd67-461e-bb8d-0f0a67627fa1
📒 Files selected for processing (4)
dfetch/manifest/manifest.pydfetch/util/yaml.pyfeatures/freeze-inplace.featuretests/test_manifest.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
dfetch/manifest/manifest.py (2)
518-556:⚠️ Potential issue | 🔴 CriticalRestrict project lookup to
projects:and accept quoted names.
_locate_project_name_line()still scans the whole document for the first bare- name:match. If a remote shares the same name,_find_project_block()can rewrite the remote block; if the manifest stores the project name as'176'or"176", lookup fails even though the YAML is valid.
657-691:⚠️ Potential issue | 🟠 MajorRemove version keys that disappeared from
project.as_yaml().This path only updates/adds keys. If a project switches from
branch/tagtorevision, or losesintegrity, the old entries stay in the manifest; when no version keys remain, Lines 690-691 return the original text unchanged. That makes the in-place path diverge from the backup+dump flow and can leave contradictory pins behind.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dfetch/manifest/manifest.py` around lines 657 - 691, The current in-place path only adds/updates version keys and never removes keys that disappeared from project.as_yaml(), so stale entries remain; update the logic so when _collect_version_fields returns no fields and no integrity_hash you remove any existing version-related lines from the project's block before returning. Concretely, modify _apply_block_updates (or call it from _update_project_version_in_text) to detect and delete lines whose field name is one of the version keys (e.g. "revision", "tag", "branch", "integrity.hash") at the given field_indent when fields_to_set is empty, and ensure _set_integrity_hash_in_block is not re-adding; then return the cleaned block so the in-place flow matches the backup+dump behavior.dfetch/util/yaml.py (1)
57-64:⚠️ Potential issue | 🟡 MinorPreserve the original EOL convention here.
update_value()still collapses\r\nto\n, andappend_field()always emits\n. A CRLF-formatted manifest will not round-trip verbatim through these helpers.Also applies to: 81-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dfetch/util/yaml.py` around lines 57 - 64, The code in update_value() (and similarly in append_field()) always uses "\n" for eol which collapses CRLF; detect and preserve the original EOL sequence by checking the source line(s) for "\r\n" vs "\n" (e.g., use line.endswith("\r\n") ? "\r\n" : line.endswith("\n") ? "\n" : "") or infer the file/ block EOL from existing lines, then use that eol variable when constructing the replacement string (replace the hard-coded "\n" in block[line_idx] assignment and in append_field() with the detected eol).
🤖 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 589-600: The loop that computes sub_end for the nested integrity
mapping incorrectly treats comment-only lines as ending the block, causing
_find_field to miss existing "hash:" entries; update the loop in the manifest
handling (the loop using variables sub_end, block, integrity_line, field_indent)
to ignore comment-only lines (e.g., skip any line where
block[i].strip().startswith("#")) when deciding the end boundary, so _find_field
and subsequent calls to _update_value/_append_field operate over the full nested
mapping.
---
Duplicate comments:
In `@dfetch/manifest/manifest.py`:
- Around line 657-691: The current in-place path only adds/updates version keys
and never removes keys that disappeared from project.as_yaml(), so stale entries
remain; update the logic so when _collect_version_fields returns no fields and
no integrity_hash you remove any existing version-related lines from the
project's block before returning. Concretely, modify _apply_block_updates (or
call it from _update_project_version_in_text) to detect and delete lines whose
field name is one of the version keys (e.g. "revision", "tag", "branch",
"integrity.hash") at the given field_indent when fields_to_set is empty, and
ensure _set_integrity_hash_in_block is not re-adding; then return the cleaned
block so the in-place flow matches the backup+dump behavior.
In `@dfetch/util/yaml.py`:
- Around line 57-64: The code in update_value() (and similarly in
append_field()) always uses "\n" for eol which collapses CRLF; detect and
preserve the original EOL sequence by checking the source line(s) for "\r\n" vs
"\n" (e.g., use line.endswith("\r\n") ? "\r\n" : line.endswith("\n") ? "\n" :
"") or infer the file/ block EOL from existing lines, then use that eol variable
when constructing the replacement string (replace the hard-coded "\n" in
block[line_idx] assignment and in append_field() with the detected eol).
🪄 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: c4d4cb95-596e-470b-a040-a219d0c2b6aa
📒 Files selected for processing (3)
dfetch/manifest/manifest.pydfetch/util/yaml.pytests/test_manifest.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/util/yaml.py`:
- Around line 68-85: The trailing-comment detection in update_value uses
re.search(r"(\s+#.*)$") which can treat hashes inside quoted YAML values as
comments; update update_value to detect an unquoted, whitespace-prefixed '#'
instead of using that regex—scan the stripped line character-by-character
tracking single/double-quote state and locate the first '#' that is not inside
quotes and is preceded by whitespace, then split comment = line[comment_start:]
(or empty if none); keep existing behavior for indent, eol and replacement.
Reference function: update_value.
🪄 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: 197224af-fb0a-4f55-becb-5d5d3ee870a6
📒 Files selected for processing (2)
dfetch/manifest/manifest.pydfetch/util/yaml.py
dfetch/util/yaml.py
Outdated
Comment on lines
+68
to
+85
| def update_value( | ||
| block: Sequence[str], line_idx: int, field_name: str, yaml_value: str | ||
| ) -> list[str]: | ||
| """Return a copy of *block* with the value on *line_idx* replaced by *yaml_value*. | ||
|
|
||
| The indentation, line ending (LF or CRLF), and any trailing comment of | ||
| the original line are all preserved so that in-place edits do not destroy | ||
| annotations or alter the file's line-ending convention. | ||
| """ | ||
| result = list(block) | ||
| line = result[line_idx] | ||
| indent = len(line) - len(line.lstrip()) | ||
| eol = _line_eol(line) | ||
| stripped = line.rstrip("\n\r") | ||
| comment_match = re.search(r"(\s+#.*)$", stripped) | ||
| comment = comment_match.group(1) if comment_match else "" | ||
| result[line_idx] = " " * indent + field_name + ": " + yaml_value + comment + eol | ||
| return result |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Trailing comment regex may incorrectly match # inside quoted YAML values.
The regex r"(\s+#.*)$" will match # characters that appear within the value portion of a line, not just actual trailing comments. For example, a line like field: "value#with#hash" would have part of the value incorrectly treated as a comment.
However, since this module is used for version fields (revision, tag, branch, hash) which typically don't contain # characters, this is a minor edge case. Consider documenting this limitation or using a more robust YAML-aware approach if needed in the future.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dfetch/util/yaml.py` around lines 68 - 85, The trailing-comment detection in
update_value uses re.search(r"(\s+#.*)$") which can treat hashes inside quoted
YAML values as comments; update update_value to detect an unquoted,
whitespace-prefixed '#' instead of using that regex—scan the stripped line
character-by-character tracking single/double-quote state and locate the first
'#' that is not inside quotes and is preceded by whitespace, then split comment
= line[comment_start:] (or empty if none); keep existing behavior for indent,
eol and replacement. Reference function: update_value.
- Add optional project name arguments to `dfetch freeze` so users can freeze individual projects rather than all at once. - When the manifest lives inside a git or SVN superproject, freeze now edits the manifest file in-place (preserving comments, blank lines and indentation) instead of creating a `.backup` copy and regenerating from scratch. - Add `update_project_in_manifest_file` and helper functions in `dfetch/manifest/manifest.py` for in-place YAML text editing. - Add BDD feature files: `freeze-specific-projects.feature` and `freeze-inplace.feature`, plus the required step definitions. - Add unit tests for all new in-place editing helpers. - Add CHANGELOG entry for v0.14.0 (unreleased). https://claude.ai/code/session_01Xd8EcAUkSoJo9YZimGzEuA
Split _set_integrity_hash_in_block (CC 11) and _update_project_version_in_text (CC 13) into smaller helpers so every new function stays below CC 9: - _update_hash_in_existing_integrity (CC 6) - _append_integrity_block (CC 3) - _set_integrity_hash_in_block (CC 4) - _collect_version_fields (CC 4) - _apply_block_updates (CC 7) - _update_project_version_in_text (CC 2) https://claude.ai/code/session_01Xd8EcAUkSoJo9YZimGzEuA
Both find_name_in_manifest (used by SARIF/Jenkins/CodeClimate reporters) and _find_project_block (used by in-place freeze) independently scanned for the '- name: <project>' line with slightly different regexes. Extract _locate_project_name_line(lines, project_name) -> tuple | None that performs the scan once and returns all four pieces of information both callers need: - line_idx (0-based) - item_indent (column of the '-') - name_col_start / name_col_end (for ManifestEntryLocation) find_name_in_manifest now calls _locate_project_name_line, preserving its public interface (same error message, same ManifestEntryLocation values including the existing "no spaces" edge case). _find_project_block delegates the name scan to the helper and only retains the block-boundary search loop. Side-effect: CC of find_name_in_manifest drops 5→3, _find_project_block drops 7→4. https://claude.ai/code/session_01Xd8EcAUkSoJo9YZimGzEuA
Replace specific-purpose helpers with three building-block functions: - `_find_field`: locate a named field at exact indent within a block - `_update_value`: replace the value on a known line index - `_append_field`: insert a new key-value line at a given position All higher-level helpers (_set_simple_field_in_block, _update_hash_in_existing_integrity, _append_integrity_block, _set_integrity_hash_in_block, _apply_block_updates) are refactored to compose these primitives instead of duplicating regex/index logic. https://claude.ai/code/session_01Xd8EcAUkSoJo9YZimGzEuA
- Extract yaml_scalar, find_field, update_value, append_field into the new dfetch.util.yaml module so they can be reused outside the manifest - Fix yaml_scalar: use splitlines()[0] instead of strip() to avoid the trailing '\n...' document-end marker that yaml.dump emits for plain strings - Fix update_value: preserve trailing inline comments (e.g. '# note') when replacing a field value, so freeze does not destroy annotations - manifest.py: replace the four duplicate implementations with aliases that delegate to dfetch.util.yaml - Add BDD scenario: inline comments on fields survive an in-place freeze - Add unit tests for comment preservation in update_value and _update_project_version_in_text https://claude.ai/code/session_01Xd8EcAUkSoJo9YZimGzEuA
find_field: add an explicit guard to skip lines where the first non-whitespace character is '#'. Previously the startswith-prefix check happened to reject these lines, but the intent was implicit. _find_project_block: skip commented lines when scanning for the end of a project block. A comment at item-indent level (e.g. ' # old pin') was incorrectly treated as a block boundary, causing fields that followed it to be excluded from the block and duplicated on freeze instead of updated. Add unit tests covering: - find_field skips '# field:' and '#field:' (no space) - find_field continues past a commented-out duplicate to find the live field - _find_project_block includes fields that follow a comment at item-indent - _update_project_version_in_text: commented-out field treated as absent - _update_project_version_in_text: comment at item-indent does not break block https://claude.ai/code/session_01Xd8EcAUkSoJo9YZimGzEuA
integrity sub_end scan skipped comment lines (manifest.py) The loop in _update_hash_in_existing_integrity that computes sub_end treated comment-only lines as block boundaries, so a '# note' line inside an integrity: mapping caused the subsequent hash: line to be missed. The loop now skips blank and comment-only lines before comparing indentation. Stale version keys not removed on in-place freeze (manifest.py) _apply_block_updates only added/updated fields; keys absent from project.as_yaml() (e.g. revision after unfreeze, or branch after switching to tag) were left in the manifest as stale entries. Added _remove_stale_version_fields and _remove_integrity_block helpers that delete version keys not present in the new project data before applying updates. Removed the early-return in _update_project_version_in_text so cleanup always runs. CRLF files collapsed to LF (yaml.py) update_value always wrote "\n"; append_field also hard-coded "\n". update_value now detects "\r\n" vs "\n" from the source line. append_field uses a new _detect_eol() helper that inspects the block's existing lines to infer the file's EOL convention. https://claude.ai/code/session_01Xd8EcAUkSoJo9YZimGzEuA
Sequence[str] for read-only inputs (manifest.py + yaml.py)
All functions that accept a block/lines parameter but never mutate it
in-place now declare Sequence[str] instead of list[str]. This resolves
8 pyright errors in tests where str.splitlines() on a LiteralString
produced list[LiteralString], which is invariant and could not be
passed as list[str].
Eliminate duplicated EOL detection (yaml.py)
_detect_eol and update_value both duplicated the endswith("\r\n") /
endswith("\n") pattern. Extracted into _line_eol(line) -> str and
wired both callers through it.
Cleaner copy pattern in _remove_* helpers (manifest.py)
_remove_integrity_block and _remove_stale_version_fields used
conditional list() copies. Both now copy unconditionally at the top
(result = list(block)) for clarity and consistent return type.
https://claude.ai/code/session_01Xd8EcAUkSoJo9YZimGzEuA
spoorcc
force-pushed
the
claude/implement-issue-1063-5NU0T
branch
from
e1a96e1 to
0b30352
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
features/freeze-inplace.feature (1)
63-96:⚠️ Potential issue | 🟡 MinorAdd the no-backup invariant to the selective in-place scenario.
Line 63 onward covers a separate execution path (
dfetch freeze <project>in a VCS superproject), but it currently does not assert thatdfetch.yaml.backupis absent.Suggested assertion
Scenario: Only selected project is frozen in-place @@ Then the manifest 'dfetch.yaml' in superproject2 is replaced with """ @@ """ + And no file 'dfetch.yaml.backup' exists in superproject2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/freeze-inplace.feature` around lines 63 - 96, The selective in-place freeze scenario ("Only selected project is frozen in-place") doesn't assert the no-backup invariant; after running "dfetch freeze ext/test-repo-tag" and verifying the manifest replacement (manifest 'dfetch.yaml'), add an assertion that no backup file named "dfetch.yaml.backup" exists in the superproject2 working tree to ensure no backup was created by the selective freeze path.dfetch/manifest/manifest.py (1)
518-558:⚠️ Potential issue | 🔴 CriticalScope the name lookup to
projects:and allow quoted scalars.This still matches the first bare
- name:anywhere in the file. If a remote shares the project's name,_find_project_block()will rewrite the remote entry; if a valid manifest spells the name as'foo'or"foo", the updater raises instead. Please bound the search to theprojects:block and accept optional matching quotes around the scalar.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dfetch/manifest/manifest.py` around lines 518 - 558, The current search in _find_project_block/_locate_project_name_line matches any "- name:" anywhere and fails on quoted scalars; update the logic to first locate the top-level "projects:" block (find the line matching a top-level ^\s*projects:\s*$ or with inline comment) and restrict subsequent name lookups to lines that are indented under that block (stop when you hit a line whose indent is <= the projects: indent or another top-level key). Then change the regex in _locate_project_name_line to accept optional single or double quotes around the scalar (e.g. allow name values like foo, 'foo', or "foo") when comparing to project_name, and return the same (line_idx, item_indent, name_col_start, name_col_end) coordinates for the unquoted characters; keep raising RuntimeError in _find_project_block when not found.
🤖 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/freeze.py`:
- Around line 108-111: The backup is using DEFAULT_MANIFEST_NAME instead of the
resolved manifest path, so change the no-VCS branch to use the manifest path
referenced by superproject.manifest.path (or the same resolved variable used
later) when copying the file and when writing updates; ensure the
shutil.copyfile call (when not use_inplace) and the subsequent edits that set
superproject.manifest.path both operate on the resolved manifest path rather
than DEFAULT_MANIFEST_NAME so non-default manifest filenames are backed up and
updated correctly.
In `@dfetch/manifest/manifest.py`:
- Around line 502-505: The current use of Path.read_text() and Path.write_text()
in manifest.py normalizes line endings and breaks the EOL-preservation logic in
_update_project_version_in_text; change the file I/O around the path variable so
you open the file with Path.open(newline='') (preserving exact EOLs) and read
the raw text (pass encoding='utf-8'), call _update_project_version_in_text(text,
project) as before, then write the updated content back using the same opened
file or a new open with newline='' and encoding='utf-8' to avoid any newline
normalization; update the code that currently uses path.read_text() and
path.write_text(updated, ...) to use open(newline='') instead so
_update_project_version_in_text receives and writes the original line endings
unchanged.
In `@features/steps/manifest_steps.py`:
- Around line 55-61: The step_impl currently calls check_file which uses a
lenient comparator that strips per-line whitespace; change it to perform a
strict byte/character comparison: build the manifest path with
os.path.join(directory, name), read the file contents (open(...).read()),
compute expected = apply_manifest_substitutions(context, context.text), and
assert file_contents == expected (or raise an assertion with both values) so
formatting/indentation regressions fail; update step_impl to use this direct
strict comparison instead of check_file.
---
Duplicate comments:
In `@dfetch/manifest/manifest.py`:
- Around line 518-558: The current search in
_find_project_block/_locate_project_name_line matches any "- name:" anywhere and
fails on quoted scalars; update the logic to first locate the top-level
"projects:" block (find the line matching a top-level ^\s*projects:\s*$ or with
inline comment) and restrict subsequent name lookups to lines that are indented
under that block (stop when you hit a line whose indent is <= the projects:
indent or another top-level key). Then change the regex in
_locate_project_name_line to accept optional single or double quotes around the
scalar (e.g. allow name values like foo, 'foo', or "foo") when comparing to
project_name, and return the same (line_idx, item_indent, name_col_start,
name_col_end) coordinates for the unquoted characters; keep raising RuntimeError
in _find_project_block when not found.
In `@features/freeze-inplace.feature`:
- Around line 63-96: The selective in-place freeze scenario ("Only selected
project is frozen in-place") doesn't assert the no-backup invariant; after
running "dfetch freeze ext/test-repo-tag" and verifying the manifest replacement
(manifest 'dfetch.yaml'), add an assertion that no backup file named
"dfetch.yaml.backup" exists in the superproject2 working tree to ensure no
backup was created by the selective freeze path.
🪄 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: 65dc41e6-702a-4489-b9e1-e551673e824b
📒 Files selected for processing (12)
CHANGELOG.rstdfetch.yamldfetch/commands/freeze.pydfetch/manifest/manifest.pydfetch/reporting/sbom_reporter.pydfetch/util/yaml.pyfeatures/freeze-archive.featurefeatures/freeze-inplace.featurefeatures/freeze-projects.featurefeatures/freeze-specific-projects.featurefeatures/steps/manifest_steps.pytests/test_manifest.py
…ifest tests
- dfetch/util/yaml.py: Fix update_filtered to split target_field on "."
so "integrity.hash" creates a nested integrity/hash structure instead
of a literal flat key "integrity.hash:"
- dfetch/commands/freeze.py: Rename use_inplace -> make_backup (both VCS
and NoVCS now use in-place editing; only NoVCS creates a .backup copy);
update class and module docstrings accordingly
- features/steps/manifest_steps.py: Add "the file '{name}' exists" step
for asserting backup file creation in NoVCS freeze scenarios
- tests/test_manifest.py: Rewrite commented-out _update_project_version_in_text
tests to use Manifest.update_project_version() + _doc.dump() pattern
https://claude.ai/code/session_01Xd8EcAUkSoJo9YZimGzEuA
yaml.dump's return type is Any without stubs; annotating the local variable as str satisfies mypy's no-any-return check without changing runtime behaviour. https://claude.ai/code/session_01Xd8EcAUkSoJo9YZimGzEuA
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
dfetch/manifest/manifest.py (1)
114-116: 🛠️ Refactor suggestion | 🟠 MajorRemove duplicate
_UNSAFE_NAME_REclass attribute.The regex pattern
_UNSAFE_NAME_REis defined identically at line 114 and line 421. This was flagged in a previous review and should be resolved.♻️ Remove duplicate definition
Keep the definition at line 114 and remove lines 420-421:
- # Characters not allowed in a project name (YAML special chars). - _UNSAFE_NAME_RE = re.compile(r"[\x00-\x1F\x7F-\x9F:#\[\]{}&*!|>'\"%@`]") - def check_name_uniqueness(self, project_name: str) -> None:Also applies to: 420-421
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dfetch/manifest/manifest.py` around lines 114 - 116, Duplicate class attribute _UNSAFE_NAME_RE is defined twice; remove the later redundant definition (the second occurrence around line ~420) and keep the original at the earlier location so only a single _UNSAFE_NAME_RE exists. Locate the duplicate _UNSAFE_NAME_RE assignment in the Manifest class (or same class where _VERSION_KEYS is defined) and delete that second declaration, then run linters/tests to ensure no references rely on the removed duplicate.dfetch/util/yaml.py (1)
94-122: 🧹 Nitpick | 🔵 TrivialMinor edge case:
get()may incorrectly parse values containing#.Line 101 splits on
#to strip trailing comments, but this can truncate values likefield: "value#with#hash". Since this module is used for version fields (revision,tag,branch) which typically don't contain#, the impact is minimal, but worth documenting.📝 Add inline documentation
def get(self, path: str | FieldPath) -> str | None: - """Get the value of a field by path.""" + """Get the value of a field by path. + + Note: Values containing unquoted ``#`` characters may be truncated + as the trailing comment detection is naive. + """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dfetch/util/yaml.py` around lines 94 - 122, The get() method currently strips trailing comments by splitting on "#" which can truncate values containing "#" inside quotes (e.g., "value#with#hash"); update the get() docstring in util/yaml.py to document this edge case and clarify that get() removes inline comments (text after an unquoted '#') and may mis-handle '#' characters inside quoted values, and suggest using set()/dump() or a proper YAML parser for such cases; reference the get() method and FieldPath conversion so readers can locate the behavior quickly.
🤖 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/freeze.py`:
- Around line 133-137: The manifest is always rewritten because
superproject.manifest.update_dump() is called unconditionally; change the logic
to track whether any project was actually updated (e.g., set a local flag when
superproject.manifest.update_project_version(project) runs/returns a change) and
only call superproject.manifest.update_dump() and the logger.info(f"Updated
manifest ({manifest_path}) in {os.getcwd()}") when that flag is true; refer to
update_project_version, update_dump, superproject.manifest and manifest_path to
locate and gate the write.
In `@dfetch/manifest/manifest.py`:
- Around line 223-224: The check in manifest.py that does `if not
isinstance(text, str): text = str(text.read())` is wrapping the read() result in
str unnecessarily; because the declared type is IO[str] (text-mode), read()
already returns str. Replace the conversion by assigning the raw read result
(i.e., use text = text.read()) in the same conditional, removing the redundant
str() wrapper so TextIO inputs are handled directly; update the branch handling
where this occurs to rely on the type hint and avoid double-wrapping.
In `@dfetch/util/yaml.py`:
- Around line 273-313: The _find_field logic relies on fixed 2-space indentation
increments (it increases indent by 2 after each path segment and expects
_find_field_at_indent to match that spacing), which should be documented; add a
clear comment or docstring near the _find_field and/or module-level doc
explaining the assumption that dfetch manifests use 2-space indentation (and
that behavior may not work with other indentation widths), so future readers and
maintainers understand why the indent arithmetic is fixed at 2.
In `@tests/test_manifest.py`:
- Around line 292-303: The helper _make_project currently uses a plain dict and
silences type checking with "# type: ignore[arg-type]"; instead annotate the
local variable as the concrete ProjectEntryDict (or cast kwargs/data to
ProjectEntryDict) so the ProjectEntry constructor receives the correct type and
you can remove the ignore. Update the signature/body of _make_project to produce
a ProjectEntryDict (or use typing.cast[ProjectEntryDict](data)) before calling
ProjectEntry, referencing ProjectEntry and ProjectEntryDict so the type-checker
accepts the call.
---
Duplicate comments:
In `@dfetch/manifest/manifest.py`:
- Around line 114-116: Duplicate class attribute _UNSAFE_NAME_RE is defined
twice; remove the later redundant definition (the second occurrence around line
~420) and keep the original at the earlier location so only a single
_UNSAFE_NAME_RE exists. Locate the duplicate _UNSAFE_NAME_RE assignment in the
Manifest class (or same class where _VERSION_KEYS is defined) and delete that
second declaration, then run linters/tests to ensure no references rely on the
removed duplicate.
In `@dfetch/util/yaml.py`:
- Around line 94-122: The get() method currently strips trailing comments by
splitting on "#" which can truncate values containing "#" inside quotes (e.g.,
"value#with#hash"); update the get() docstring in util/yaml.py to document this
edge case and clarify that get() removes inline comments (text after an unquoted
'#') and may mis-handle '#' characters inside quoted values, and suggest using
set()/dump() or a proper YAML parser for such cases; reference the get() method
and FieldPath conversion so readers can locate the behavior quickly.
🪄 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: 6904bace-7650-45a9-8d78-d7634c2853c6
📒 Files selected for processing (7)
dfetch/commands/freeze.pydfetch/manifest/manifest.pydfetch/util/versions.pydfetch/util/yaml.pydfetch/vcs/git.pyfeatures/steps/manifest_steps.pytests/test_manifest.py
💤 Files with no reviewable changes (2)
- dfetch/vcs/git.py
- dfetch/util/versions.py
…tr, improve docs/types - freeze.py: Only call update_dump()/log when at least one project was actually frozen (track via manifest_updated flag) - manifest.py: Remove redundant str() wrapper around text.read() (IO[str] already returns str); remove duplicate _UNSAFE_NAME_RE class attribute (the first definition at the top of the class is kept) - yaml.py: Document the 2-space indentation assumption in _find_field; document the '#'-in-quoted-values edge case in get() docstring - tests/test_manifest.py: Replace # type: ignore[arg-type] in _make_project with typing.cast(ProjectEntryDict, ...) and import ProjectEntryDict https://claude.ai/code/session_01Xd8EcAUkSoJo9YZimGzEuA
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 515-518: The TODO requests turning the standalone function
append_entry_manifest_file into a Manifest class method that operates on the
manifest instance's internal YamlDocument (self.__doc) so updates preserve
formatting and comments and so the class can control when to write to disk;
refactor append_entry_manifest_file into a method on the Manifest class, replace
direct file reads/writes inside that function with in-memory edits to self.__doc
via the existing YamlDocument APIs, update all callers to call the new
Manifest.append_entry_manifest_file method, and ensure the class exposes a
single write/save method that decides when to flush changes to disk.
🪄 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: 381e26ae-d250-44e0-8640-11d9ce0c57ce
📒 Files selected for processing (4)
dfetch/commands/freeze.pydfetch/manifest/manifest.pydfetch/util/yaml.pytests/test_manifest.py
The standalone append_entry_manifest_file function (with its TODO) is
replaced by Manifest.append_project_entry, which appends the new project
entry to self._doc.lines in-memory. Callers then call update_dump() to
flush changes to disk — consistent with how update_project_version works.
- manifest.py: add append_project_entry(); remove standalone function
and its TODO comment
- add.py: call manifest.append_project_entry() + manifest.update_dump()
instead of the standalone function; drop the now-unused import
- test_add.py: remove patch("...append_entry_manifest_file") wrappers
(manifest is already a Mock so the method is auto-mocked); update all
assertions to use fake_superproject.manifest.append_project_entry
https://claude.ai/code/session_01Xd8EcAUkSoJo9YZimGzEuA
…ument
The old get(FieldPath)/set(FieldPath)/find_by_filter()/update_filtered()
methods are replaced by two orthogonal operations:
doc.get(jsonpath) → list[NodeMatch]
doc.set(jsonpath, field, val) → None
Supported JSONPath subset: member access (.key), array index ([n]),
and equality filter ([?(@.key == "value")]). This unifies the query and
mutation paths and makes the freeze call read naturally:
doc.set('$.manifest.projects[?(@.name == "my-project")]', "tag", "v1.2.3")
manifest.py callers updated:
- update_project_version: uses doc.set() with a single filter path
- find_name_in_manifest: uses doc.get(...name) and NodeMatch.location
29 new unit tests added in tests/test_yaml.py covering parsing,
get(), set(), and edge cases (nested fields, integer-quoted scalars,
comment preservation, no-op on no match).
https://claude.ai/code/session_01Xd8EcAUkSoJo9YZimGzEuA
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
dfetch/manifest/manifest.py (2)
411-423:⚠️ Potential issue | 🟠 MajorRemove version keys that disappeared from the frozen project.
This only writes non-empty values from
project.versionandproject.integrity. If a project moves frombranchtorevision, or dropsintegrity.hash, the old keys stay in the manifest and you end up with contradictory pins. The in-place path needs to delete absentrevision/tag/branchfields and dropintegritywhen no hash remains.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dfetch/manifest/manifest.py` around lines 411 - 423, The update_project_version method only sets non-empty version/integrity fields but does not remove keys that are now absent, leaving stale pins; modify update_project_version (in manifest.update_project_version working with ProjectEntry and project.version/project.integrity) to explicitly remove any version keys that are empty/missing (e.g., delete "revision", "tag", "branch" when not present) and to remove the entire "integrity" object when project.integrity is falsy or project.integrity.hash is empty; use the document helper (self._doc.set currently used to write fields) plus the document delete/remove API (the inverse of self._doc.set) to remove those paths (e.g., self._doc.delete(path, "revision") etc.) so the manifest reflects only the current pins.
223-224:⚠️ Potential issue | 🟠 MajorPreserve raw newlines before building
YamlDocument.
text.read()only preserves\r\nwhen the wrapper was opened withnewline="".from_file()still uses default text mode, so file-backed manifests reach the new editor already normalized to\n, andupdate_dump()rewrites CRLF manifests as LF.Expected: the first read normalizes
\r\nto\n, the second preserves\r\n, and the grep showsfrom_file()still opens withoutnewline="".#!/bin/bash python - <<'PY' import io raw = b"manifest:\r\n version: '0.0'\r\n" print("newline=None ->", repr(io.TextIOWrapper(io.BytesIO(raw), encoding="utf-8").read())) print("newline='' ->", repr(io.TextIOWrapper(io.BytesIO(raw), encoding="utf-8", newline="").read())) PY rg -n -A2 -B2 'def from_file|open\(path, encoding="utf-8"' dfetch/manifest/manifest.py🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dfetch/manifest/manifest.py` around lines 223 - 224, The code currently calls text.read() (in the manifest logic around from_file / the block that assigns to variable text) while files are opened in default text mode so CRLF gets normalized to LF; change the file open in from_file to use open(path, encoding="utf-8", newline="") so reads preserve raw CRLF when intended, and keep the existing text.read() usage (or ensure the wrapper passed into the block is opened with newline="") so YamlDocument/update_dump sees the original newlines; look for the from_file function and the variable text / YamlDocument construction to update the open(...) call 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/manifest/manifest.py`:
- Around line 286-302: The missing-project check in _check_all_names_found
treats duplicate requested names as separate entries, causing spurious errors;
deduplicate the incoming names before doing length comparisons and computing
unfound: inside _check_all_names_found, create a deduped list or set (e.g.,
unique_names = list(dict.fromkeys(names)) or unique_names = set(names)), use
unique_names for the early-return check (if not unique_names or len(projects) ==
len(unique_names): return) and when building unfound (unfound = [n for n in
unique_names if n not in found]) so RequestedProjectNotFoundError is raised only
for truly missing unique names. Ensure you still use self._projects (and its
project.name values) to compute possibles.
In `@dfetch/util/yaml.py`:
- Around line 334-379: _find_field() currently continues scanning to EOF after
matching a node, so a later sibling's field can be wrongly matched; change the
logic to bound lookups to the matched node's block: after you find an index idx
for a part (both for sequence items found via "-" and for named keys), compute
the block end by scanning forward from idx+1 until you hit a line whose
indentation is <= the current indent (or EOF) and use that end as the upper
bound for subsequent _find_field_at_indent searches (best done by extending
_find_field_at_indent with an optional end parameter or by passing start/end
slice indices); ensure indent is the indent of the matched node (before
incrementing for the child) so children are searched only inside that node,
update _find_field to use this bound when handling digit-keys and named keys,
and add a two-project regression test covering manifest.projects[0] missing
revision/tag/branch while projects[1] has one.
In `@tests/test_add.py`:
- Around line 177-179: Add assertions that the persistence step runs: after
verifying fake_superproject.manifest.append_project_entry was called and
entry.name == "myrepo" in tests/test_add.py, also assert that
fake_superproject.manifest.update_dump.assert_called_once() to ensure the change
was written; and in the abort/failure test case assert that
fake_superproject.manifest.update_dump.assert_not_called() (and keep
append_project_entry assert_not_called() where appropriate) so the test verifies
both append and the subsequent update_dump persistence behavior.
---
Duplicate comments:
In `@dfetch/manifest/manifest.py`:
- Around line 411-423: The update_project_version method only sets non-empty
version/integrity fields but does not remove keys that are now absent, leaving
stale pins; modify update_project_version (in manifest.update_project_version
working with ProjectEntry and project.version/project.integrity) to explicitly
remove any version keys that are empty/missing (e.g., delete "revision", "tag",
"branch" when not present) and to remove the entire "integrity" object when
project.integrity is falsy or project.integrity.hash is empty; use the document
helper (self._doc.set currently used to write fields) plus the document
delete/remove API (the inverse of self._doc.set) to remove those paths (e.g.,
self._doc.delete(path, "revision") etc.) so the manifest reflects only the
current pins.
- Around line 223-224: The code currently calls text.read() (in the manifest
logic around from_file / the block that assigns to variable text) while files
are opened in default text mode so CRLF gets normalized to LF; change the file
open in from_file to use open(path, encoding="utf-8", newline="") so reads
preserve raw CRLF when intended, and keep the existing text.read() usage (or
ensure the wrapper passed into the block is opened with newline="") so
YamlDocument/update_dump sees the original newlines; look for the from_file
function and the variable text / YamlDocument construction to update the
open(...) call accordingly.
🪄 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: 4bd0d78b-1cc3-4190-872f-0b6b42b41f4b
📒 Files selected for processing (5)
dfetch/commands/add.pydfetch/manifest/manifest.pydfetch/util/yaml.pytests/test_add.pytests/test_yaml.py
… update_dump tests - manifest.py _check_all_names_found: deduplicate incoming names with dict.fromkeys() before length comparison and unfound computation so repeated names in the CLI args don't cause spurious errors - manifest.py from_file: open with newline="" to preserve CRLF so YamlDocument.eol detection sees the original line endings and update_dump writes them back unchanged - yaml.py _find_field: after matching each path segment, bound the child search range to that node's block via _block_end(); this prevents a missing field in projects[0] from being found inside projects[1]; also adds optional end param to _find_field_at_indent - tests/test_yaml.py: three regression tests for the scope-leak fix (_TWO_PROJECT_MANIFEST with revision only in second project) - tests/test_add.py: add update_dump.assert_called_once() / assert_not_called() after every append_project_entry assertion so the persistence step is verified in all 12 affected tests https://claude.ai/code/session_01Xd8EcAUkSoJo9YZimGzEuA
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/test_add.py (1)
220-240:⚠️ Potential issue | 🟡 MinorAdd persistence assertions in the remaining success-path tests.
These tests still only inspect
call_args, which does not verify flush-to-disk behavior (update_dump) and can miss multi-append regressions. Please align them with the other updated tests by assertingappend_project_entry.assert_called_once()andupdate_dump.assert_called_once().✅ Suggested test tightening
def test_add_command_suffixes_duplicate_name(): @@ - entry: ProjectEntry = fake_superproject.manifest.append_project_entry.call_args[0][0] + fake_superproject.manifest.append_project_entry.assert_called_once() + fake_superproject.manifest.update_dump.assert_called_once() + entry: ProjectEntry = fake_superproject.manifest.append_project_entry.call_args[0][0] assert entry.name == "myrepo-1" def test_add_command_interactive_branch_by_number(): @@ - entry: ProjectEntry = fake_superproject.manifest.append_project_entry.call_args[0][0] + fake_superproject.manifest.append_project_entry.assert_called_once() + fake_superproject.manifest.update_dump.assert_called_once() + entry: ProjectEntry = fake_superproject.manifest.append_project_entry.call_args[0][0] assert entry.branch == "dev" def test_add_command_interactive_run_update(): @@ mock_update.assert_called_once() + fake_superproject.manifest.append_project_entry.assert_called_once() + fake_superproject.manifest.update_dump.assert_called_once() def test_add_command_interactive_svn_custom_branch(): @@ - entry: ProjectEntry = fake_superproject.manifest.append_project_entry.call_args[0][0] + fake_superproject.manifest.append_project_entry.assert_called_once() + fake_superproject.manifest.update_dump.assert_called_once() + entry: ProjectEntry = fake_superproject.manifest.append_project_entry.call_args[0][0] assert entry.branch == "feature-x" def test_add_command_interactive_svn_tag(): @@ - entry: ProjectEntry = fake_superproject.manifest.append_project_entry.call_args[0][0] + fake_superproject.manifest.append_project_entry.assert_called_once() + fake_superproject.manifest.update_dump.assert_called_once() + entry: ProjectEntry = fake_superproject.manifest.append_project_entry.call_args[0][0] assert entry.tag == "v2.0" assert entry.branch == "" def test_add_command_interactive_svn_branch_by_number(): @@ - entry: ProjectEntry = fake_superproject.manifest.append_project_entry.call_args[0][0] + fake_superproject.manifest.append_project_entry.assert_called_once() + fake_superproject.manifest.update_dump.assert_called_once() + entry: ProjectEntry = fake_superproject.manifest.append_project_entry.call_args[0][0] assert entry.branch == "feature-x"Also applies to: 313-321, 494-505, 678-681, 709-713, 741-744
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_add.py` around lines 220 - 240, Update the test_add_command_suffixes_duplicate_name test (and the other listed tests) to assert that persistence occurred by adding append_project_entry.assert_called_once() and update_dump.assert_called_once() after the existing call_args inspection; locate the assertions around the fake_superproject.manifest mocks used with Add() and ensure you call fake_superproject.manifest.append_project_entry.assert_called_once() and fake_superproject.manifest.update_dump.assert_called_once() (or the exact mock names used) to verify single append and a single flush to disk.dfetch/manifest/manifest.py (1)
415-425:⚠️ Potential issue | 🟠 MajorClear obsolete freeze fields before writing the new ones.
This method only updates non-empty values. Re-freezing from
branchtorevision, or removingintegrity.hash, will leave the old keys in place, so the in-place path can persist contradictory selectors that the regenerated manifest would not. Please delete absentrevision/tag/branchkeys before setting the remaining ones, and removeintegritywhen no hash remains.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dfetch/manifest/manifest.py` around lines 415 - 425, The code currently only sets non-empty version fields and integrity.hash, leaving stale keys behind; update the block that iterates project.version._asdict() to explicitly remove absent freeze fields: for the keys 'revision','tag','branch' call self._doc.delete(path, key) (or the doc API's removal method) when the value is None or empty, and only call self._doc.set(path, name, value) for non-empty values; likewise, if project.integrity is falsy or project.integrity.hash is empty, remove the entire 'integrity' node (e.g. self._doc.delete(path, "integrity")) instead of leaving an empty/old hash, otherwise set "integrity.hash" when present.
🤖 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/manifest/manifest.py`:
- Around line 415-425: The code currently only sets non-empty version fields and
integrity.hash, leaving stale keys behind; update the block that iterates
project.version._asdict() to explicitly remove absent freeze fields: for the
keys 'revision','tag','branch' call self._doc.delete(path, key) (or the doc
API's removal method) when the value is None or empty, and only call
self._doc.set(path, name, value) for non-empty values; likewise, if
project.integrity is falsy or project.integrity.hash is empty, remove the entire
'integrity' node (e.g. self._doc.delete(path, "integrity")) instead of leaving
an empty/old hash, otherwise set "integrity.hash" when present.
In `@tests/test_add.py`:
- Around line 220-240: Update the test_add_command_suffixes_duplicate_name test
(and the other listed tests) to assert that persistence occurred by adding
append_project_entry.assert_called_once() and update_dump.assert_called_once()
after the existing call_args inspection; locate the assertions around the
fake_superproject.manifest mocks used with Add() and ensure you call
fake_superproject.manifest.append_project_entry.assert_called_once() and
fake_superproject.manifest.update_dump.assert_called_once() (or the exact mock
names used) to verify single append and a single flush to disk.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3aa6ffa5-23e4-414b-8b7f-ff0dbb579c11
📒 Files selected for processing (4)
dfetch/manifest/manifest.pydfetch/util/yaml.pytests/test_add.pytests/test_yaml.py
…ete test assertions - yaml.py: add delete(jsonpath, field) — mirrors set() but removes the field and its child block; _delete_by_parts stops at blank lines to preserve inter-item separators; 5 unit tests added in test_yaml.py - manifest.py update_project_version: call _doc.delete() for each version key that is now empty/absent (removes stale 'revision', 'tag', 'branch') and remove the entire 'integrity' block when no hash is present; regression test added in test_manifest.py - test_add.py: add append_project_entry.assert_called_once() + update_dump.assert_called_once() to the 5 tests that previously only accessed call_args without asserting call count: suffixes_duplicate_name, interactive_branch_by_number, svn_custom_branch, svn_tag, svn_branch_by_number https://claude.ai/code/session_01Xd8EcAUkSoJo9YZimGzEuA
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 394-410: append_project_entry currently hardcodes a 2-space prefix
when appending YAML lines (the new_entry loop prepends " "), which will
misalign manifests that use a different indent width; update
append_project_entry to detect the existing indentation used under the projects:
section (e.g., scan self._doc.lines for the "projects:" line then inspect the
leading whitespace of the next non-empty line or an existing project entry) and
use that whitespace string when prefixing each line instead of the hardcoded "
", then keep calling update_dump to persist as before; reference
append_project_entry, new_entry, and self._doc.lines when making the change.
🪄 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: 81f2fd8c-3fe7-4792-aded-de1aa4f2a5b5
📒 Files selected for processing (5)
dfetch/manifest/manifest.pydfetch/util/yaml.pytests/test_add.pytests/test_manifest.pytests/test_yaml.py
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
2 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.
dfetch freezeso users canfreeze individual projects rather than all at once.
edits the manifest file in-place (preserving comments, blank lines and
indentation) instead of creating a
.backupcopy and regenerating fromscratch.
update_project_in_manifest_fileand helper functions indfetch/manifest/manifest.pyfor in-place YAML text editing.freeze-specific-projects.featureandfreeze-inplace.feature, plus the required step definitions.https://claude.ai/code/session_01Xd8EcAUkSoJo9YZimGzEuA
Summary by CodeRabbit
New Features
Data
Tests
Changelog
Behavior