Copier update: claude permissions#113
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis PR introduces a complete Claude Code skill for addressing GitHub PR review comments via a two-phase workflow (user decision collection followed by batched code-change execution), alongside comprehensive infrastructure updates including devcontainer upgrades, CI/CD workflow enhancements, permission broadening for Claude Code, dependency bumping, and template synchronization across numerous configuration files. ChangesClaude Code PR Comments Skill
Infrastructure, Permissions, and Configuration Updates
Dependency Bumping and Code Quality Rules
Type Cleanup and Test Infrastructure
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Suggested Reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
zendern
approved these changes
Jun 4, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 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 @.claude/skills/address-pr-comments/fetch-pr-comments.py:
- Around line 225-241: The filter that drops threads whose last author is
current_user in _build_pulls_threads removes the replies needed for resume-mode
dedupe; change _build_pulls_threads signature to accept an optional flag
include_latest_by_current_user: bool = False, replace the hard `if
_last_author(t) == current_user: continue` with a conditional that only skips
when include_latest_by_current_user is False, and update any callers (notably
the resume inventory caller) to pass True when they need to retain threads
latest-authored-by-current-user so resume detection can inspect replies; keep
the existing behavior (dropping those threads) when the flag is not set and
still strip out "_html_url" before appending.
In @.claude/skills/address-pr-comments/SKILL.md:
- Around line 69-79: Several markdown fenced-code blocks in the SKILL.md
examples (for instance the block beginning "Resume inventory for PR <n>:" and
the command/prompt examples referencing pr-reply-12345.txt/pr-reply-12346.txt)
are missing explicit language tags and surrounding blank lines; update each
fenced block to use an explicit language label (e.g., ```text, ```bash, or
```json as appropriate) and ensure there is a blank line before and after the
fenced block so it conforms to MD040/MD031. Locate every example/fence in the
document and normalize all triple-backtick fences to include the language and
proper spacing consistently.
In @.github/workflows/ci.yaml:
- Line 182: Replace the mutable tag "actions/cache@v5.0.5" with an immutable
commit SHA for the actions/cache action; locate the uses entry "uses:
actions/cache@v5.0.5" in the workflow and update it to pin the action to its
corresponding full commit hash (the 40‑char SHA) so the workflow references an
immutable revision.
In @.github/workflows/get-values.yaml:
- Around line 52-64: Replace the direct expansion of github.event.number inside
the run block by exporting it as an env var (e.g., PR_NUMBER: ${{
github.event.number }}) alongside MERGE_MSG, then change the run logic to set
full_number from $PR_NUMBER (e.g., [[ '${{ github.event_name }}' =
'pull_request' ]] && full_number="$PR_NUMBER") and keep the existing merge_group
parsing for MERGE_MSG; update references to use full_number and ensure values
are quoted when assigned to avoid word-splitting or injection.
In `@extensions/context.py`:
- Around line 133-134: Update documentation and any template generation logic to
reflect the feature-flag rename and behavior change: replace references to the
old symbol template_uses_javascript with the new template_uses_typescript and
ensure template_uses_python = True is intentionally set; update README /
changelog and devcontainer feature selection logic that consumes
context["template_uses_typescript"] and context["template_uses_python"] so
Python devcontainer features (pytest-mock) are generated correctly and Node
tooling is adjusted, and add a brief note in PR/changelog describing the change.
In `@pyproject.toml`:
- Around line 21-24: The current PyPI index configuration under
[[tool.uv.index]] uses exclude-newer = "1 day", which is likely too short and
can exclude recently published dependencies causing build failures; update the
exclude-newer value for the index with name = "pypi" from "1 day" to a longer
window (e.g., "7 days") to improve stability, and ensure any config comments or
repository template documentation reflecting tool.uv.index are updated to
explain the chosen window so consumers know why it's longer.
In `@template/.claude/settings/permissions/bash.jsonc`:
- Line 117: The current permission "Bash(pnpm test*)" is too broad; restrict it
to explicit allowed test scripts by replacing the wildcard with concrete script
names (e.g., "Bash(pnpm test-unit)" and any other approved scripts like
"Bash(pnpm test-integration)" if needed). Edit the permissions entry that
currently references Bash(pnpm test*) to list only the exact pnpm test script
names your project uses (match package.json script keys) so only those
known-safe commands can run. Ensure you update any related documentation or
comments to note the approved script names and avoid allowing direct invocations
like "pnpm vitest" or "npx vitest".
In `@template/.claude/skills/address-pr-comments/fetch-pr-comments.py`:
- Line 84: The inline noqa directive on the function signature gh_graphql(query:
str, variables: dict[str, str | int]) -> Any currently disables ANN401 without
explanation; update that line to keep the noqa but append a brief comment
explaining the rationale (e.g., why return type Any is acceptable or why precise
annotation isn’t feasible here) so the noqa has a justification, referencing the
gh_graphql function and ANN401 rule in the comment.
- Around line 90-91: The inline "noqa: S603" on the subprocess.run call lacks an
explanation; update the subprocess.run invocation (the line with "result =
subprocess.run(... args,") to retain the noqa but add a short comment explaining
why rule S603 is being suppressed (e.g., safe use of subprocess with validated
args or controlled input), mentioning the rationale and any mitigation (input
validation/escaping) so reviewers can see why subprocess.run is acceptable here.
In `@template/.github/workflows/get-values.yaml`:
- Around line 52-64: The workflow currently expands ${{ github.event.number }}
directly inside the run block; add a new environment variable (e.g., PR_NUMBER)
at the same env level as MERGE_MSG and set it to ${{ github.event.number }},
then update the run script to reference $PR_NUMBER instead of expanding
github.event.number inline (adjust the conditional that sets full_number from
full_number=${{ github.event.number }} to full_number=$PR_NUMBER and keep the
existing merge_group extraction logic using $MERGE_MSG and $full_number). Ensure
the conditional comparison of '${{ github.event_name }}' remains unchanged but
any occurrence of inline github.event.* values in the run block is replaced with
corresponding env vars like PR_NUMBER for consistency and safer escaping.
In `@tests/unit/copier_tasks/helpers.py`:
- Around line 6-10: Change run_copier_task so its multi-argument API is
keyword-only: update the function signature (run_copier_task) to force callers
to pass script_path, args (or a single variadic collection param) and env by
name (use a leading * in the signature) and keep the return type
subprocess.CompletedProcess[str]; then update all callers to call
run_copier_task(script_path=..., args=..., env=...) (or script_path=... and
positional args moved into an explicit args parameter) to match the new
signature and types.
🪄 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: b5deabbb-003b-41bd-b435-611904979bdf
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (45)
.claude/settings/permissions/additional-dirs.jsonc.claude/settings/permissions/bash.jsonc.claude/settings/permissions/write.jsonc.claude/skills/address-pr-comments/SKILL.md.claude/skills/address-pr-comments/check-footer.py.claude/skills/address-pr-comments/fetch-pr-comments.py.claude/skills/address-pr-comments/post-reply.py.claude/skills/address-pr-comments/utils.py.copier-answers.yml.devcontainer/Dockerfile.devcontainer/devcontainer.json.devcontainer/install-ci-tooling.py.github/reusable_workflows/build-docker-image.yaml.github/workflows/ci.yaml.github/workflows/get-values.yaml.github/workflows/hash_git_files.py.github/workflows/pre-commit.yaml.gitignore.pre-commit-config.yamlAGENTS.mdcopier.ymlextensions/context.pypyproject.tomlruff.tomltemplate/.claude/settings/permissions/additional-dirs.jsonctemplate/.claude/settings/permissions/bash.jsonctemplate/.claude/settings/permissions/write.jsonctemplate/.claude/skills/address-pr-comments/SKILL.mdtemplate/.claude/skills/address-pr-comments/check-footer.pytemplate/.claude/skills/address-pr-comments/fetch-pr-comments.pytemplate/.claude/skills/address-pr-comments/post-reply.pytemplate/.claude/skills/address-pr-comments/utils.pytemplate/.devcontainer/Dockerfiletemplate/.devcontainer/devcontainer.json.jinjatemplate/.github/workflows/get-values.yamltemplate/.github/workflows/hash_git_files.pytemplate/.github/workflows/pre-commit.yamltemplate/.gitignoretemplate/.pre-commit-config.yamltemplate/AGENTS.mdtemplate/pyproject.toml.jinjatemplate/ruff.tomltemplate/src/aws_central_infrastructure/artifact_stores/lib/image_builder.pytests/unit/copier_tasks/helpers.pytests/unit/copier_tasks/test_remove_precommit_hooks.py
Comment on lines
+225
to
+241
| def _build_pulls_threads( | ||
| pulls_raw: list[dict[str, Any]], | ||
| resolved_urls: set[str], | ||
| current_user: str, | ||
| ) -> list[dict[str, Any]]: | ||
| """Group inline review comments into threads, filtering resolved + already-addressed.""" | ||
| roots = _collect_thread_roots(pulls_raw) | ||
| _attach_replies(pulls_raw, roots) | ||
|
|
||
| threads: list[dict[str, Any]] = [] | ||
| for t in roots.values(): | ||
| if t["_html_url"] in resolved_urls: | ||
| continue | ||
| if _last_author(t) == current_user: | ||
| continue | ||
| threads.append({k: v for k, v in t.items() if k != "_html_url"}) | ||
| return threads |
There was a problem hiding this comment.
Resume-mode dedupe signal is dropped by the current-user-last filter.
On Line 238, threads whose latest reply is by current_user are removed, but resume mode in .claude/skills/address-pr-comments/SKILL.md relies on thread replies to detect “already manually replied, do not repost.” This makes that invariant unreachable for those threads and can misclassify resume artifacts.
A robust fix is to make this filter optional (e.g., an include_latest_by_current_user mode for resume inventory) and enable that mode when Step 1 resume inventory calls the fetch script.
Suggested direction
-def _build_pulls_threads(
+def _build_pulls_threads(
pulls_raw: list[dict[str, Any]],
resolved_urls: set[str],
current_user: str,
+ *,
+ include_latest_by_current_user: bool,
) -> list[dict[str, Any]]:
@@
- if _last_author(t) == current_user:
+ if not include_latest_by_current_user and _last_author(t) == current_user:
continue🤖 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 @.claude/skills/address-pr-comments/fetch-pr-comments.py around lines 225 -
241, The filter that drops threads whose last author is current_user in
_build_pulls_threads removes the replies needed for resume-mode dedupe; change
_build_pulls_threads signature to accept an optional flag
include_latest_by_current_user: bool = False, replace the hard `if
_last_author(t) == current_user: continue` with a conditional that only skips
when include_latest_by_current_user is False, and update any callers (notably
the resume inventory caller) to pass True when they need to retain threads
latest-authored-by-current-user so resume detection can inspect replies; keep
the existing behavior (dropping those threads) when the flag is not set and
still strip out "_html_url" before appending.
Comment on lines
+69
to
+79
| ``` | ||
| Resume inventory for PR <n>: | ||
| - <X> unpushed commits: <hash list> | ||
| - <Y> reply drafts: | ||
| - pr-reply-12345.txt — has [COMMIT LINK] placeholder, needs commit | ||
| - pr-reply-12346.txt — finalized, paired with commit abc1234 | ||
| - pr-reply-12347.txt — orphan (comment 12347 no longer unresolved) | ||
| - <Z> in_progress bd issues: bd-87, bd-88 | ||
|
|
||
| How to proceed? | ||
| ``` |
There was a problem hiding this comment.
Fix markdown fence lint violations in command/prompt examples.
Several fenced blocks are missing explicit languages and/or surrounding blank lines, which matches the current markdownlint findings (MD040/MD031). Please normalize those blocks so docs lint stays clean.
Also applies to: 139-149, 163-175, 224-238
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 69-69: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/skills/address-pr-comments/SKILL.md around lines 69 - 79, Several
markdown fenced-code blocks in the SKILL.md examples (for instance the block
beginning "Resume inventory for PR <n>:" and the command/prompt examples
referencing pr-reply-12345.txt/pr-reply-12346.txt) are missing explicit language
tags and surrounding blank lines; update each fenced block to use an explicit
language label (e.g., ```text, ```bash, or ```json as appropriate) and ensure
there is a blank line before and after the fenced block so it conforms to
MD040/MD031. Locate every example/fence in the document and normalize all
triple-backtick fences to include the language and proper spacing consistently.
|
|
||
| - name: Cache Pre-commit hooks | ||
| uses: actions/cache@v5.0.4 | ||
| uses: actions/cache@v5.0.5 |
There was a problem hiding this comment.
Pin actions/cache to an immutable commit SHA.
Line 182 uses a mutable tag (v5.0.5), which weakens workflow supply-chain guarantees.
Suggested change
- uses: actions/cache@v5.0.5
+ uses: actions/cache@<40-char-commit-sha> # v5.0.5Use this read-only check to find unpinned uses entries in this workflow before/after the fix:
#!/bin/bash
rg -n '^\s*uses:\s*[^@]+@(v[0-9]|[0-9]+\.[0-9]+)' .github/workflows/ci.yaml
rg -n '^\s*uses:\s*[^@]+@[0-9a-f]{40}\b' .github/workflows/ci.yaml🧰 Tools
🪛 zizmor (1.25.2)
[error] 182-182: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yaml at line 182, Replace the mutable tag
"actions/cache@v5.0.5" with an immutable commit SHA for the actions/cache
action; locate the uses entry "uses: actions/cache@v5.0.5" in the workflow and
update it to pin the action to its corresponding full commit hash (the 40‑char
SHA) so the workflow references an immutable revision.
Comment on lines
+52
to
+64
| env: | ||
| MERGE_MSG: ${{ github.event.merge_group.head_commit.message }} | ||
| run: | | ||
|
|
||
| [[ '${{ github.event_name }}' = 'pull_request' ]] && full_number=${{ github.event.number }} | ||
|
|
||
| # example message in merge group context: "Merge pull request #10 from org-name/branch-name\n\ncommit message" | ||
| [[ '${{ github.event_name }}' = 'merge_group' ]] && message='${{ github.event.merge_group.head_commit.message }}' && echo Extracting from $message && number_and_following_text=${message##*#} && full_number=${number_and_following_text%%[!0-9]*} | ||
| if [[ '${{ github.event_name }}' = 'merge_group' ]]; then | ||
| message="$MERGE_MSG" | ||
| echo "Extracting from $message" | ||
| number_and_following_text="${message##*#}" | ||
| full_number="${number_and_following_text%%[!0-9]*}" | ||
| fi |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider moving all GitHub context values to environment variables for consistency.
The refactoring to use MERGE_MSG env variable for the merge commit message (lines 52-53) is a security improvement because GitHub Actions automatically escapes env variables, preventing template injection. However, line 56 still directly expands ${{ github.event.number }} in the run block.
For defense-in-depth and consistency, consider moving github.event.number to an env variable as well:
🔒 Proposed hardening
env:
MERGE_MSG: ${{ github.event.merge_group.head_commit.message }}
+ PR_NUMBER: ${{ github.event.number }}
run: |
- [[ '${{ github.event_name }}' = 'pull_request' ]] && full_number=${{ github.event.number }}
+ [[ '${{ github.event_name }}' = 'pull_request' ]] && full_number="$PR_NUMBER"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| env: | |
| MERGE_MSG: ${{ github.event.merge_group.head_commit.message }} | |
| run: | | |
| [[ '${{ github.event_name }}' = 'pull_request' ]] && full_number=${{ github.event.number }} | |
| # example message in merge group context: "Merge pull request #10 from org-name/branch-name\n\ncommit message" | |
| [[ '${{ github.event_name }}' = 'merge_group' ]] && message='${{ github.event.merge_group.head_commit.message }}' && echo Extracting from $message && number_and_following_text=${message##*#} && full_number=${number_and_following_text%%[!0-9]*} | |
| if [[ '${{ github.event_name }}' = 'merge_group' ]]; then | |
| message="$MERGE_MSG" | |
| echo "Extracting from $message" | |
| number_and_following_text="${message##*#}" | |
| full_number="${number_and_following_text%%[!0-9]*}" | |
| fi | |
| env: | |
| MERGE_MSG: ${{ github.event.merge_group.head_commit.message }} | |
| PR_NUMBER: ${{ github.event.number }} | |
| run: | | |
| [[ '${{ github.event_name }}' = 'pull_request' ]] && full_number="$PR_NUMBER" | |
| # example message in merge group context: "Merge pull request `#10` from org-name/branch-name\n\ncommit message" | |
| if [[ '${{ github.event_name }}' = 'merge_group' ]]; then | |
| message="$MERGE_MSG" | |
| echo "Extracting from $message" | |
| number_and_following_text="${message##*#}" | |
| full_number="${number_and_following_text%%[!0-9]*}" | |
| fi |
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 56-56: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 56-56: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 59-59: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/get-values.yaml around lines 52 - 64, Replace the direct
expansion of github.event.number inside the run block by exporting it as an env
var (e.g., PR_NUMBER: ${{ github.event.number }}) alongside MERGE_MSG, then
change the run logic to set full_number from $PR_NUMBER (e.g., [[ '${{
github.event_name }}' = 'pull_request' ]] && full_number="$PR_NUMBER") and keep
the existing merge_group parsing for MERGE_MSG; update references to use
full_number and ensure values are quoted when assigned to avoid word-splitting
or injection.
Comment on lines
+133
to
+134
| context["template_uses_typescript"] = False | ||
| context["template_uses_python"] = True |
There was a problem hiding this comment.
Document the template feature flag change.
The PR description mentions adding pytest-mock but doesn't document this behavioral change:
template_uses_javascriptwas replaced withtemplate_uses_typescripttemplate_uses_pythonis now set toTrue
This affects which devcontainer features are generated (Python vs Node tooling). Verify this change was intentional and aligns with the template's evolution.
🤖 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 `@extensions/context.py` around lines 133 - 134, Update documentation and any
template generation logic to reflect the feature-flag rename and behavior
change: replace references to the old symbol template_uses_javascript with the
new template_uses_typescript and ensure template_uses_python = True is
intentionally set; update README / changelog and devcontainer feature selection
logic that consumes context["template_uses_typescript"] and
context["template_uses_python"] so Python devcontainer features (pytest-mock)
are generated correctly and Node tooling is adjusted, and add a brief note in
PR/changelog describing the change.
| "Bash(pnpm test-unit *)", | ||
| "Bash(pnpm test-e2e *)", | ||
| "Bash(pnpm list *)", | ||
| "Bash(pnpm test*)", |
There was a problem hiding this comment.
Narrow the pnpm allow-list to explicit test scripts.
Line [117] ("Bash(pnpm test*)" ) allows any test* script name, which can execute unintended high-impact commands defined in package.json. Keep this permission explicit to known-safe scripts.
Suggested change
- "Bash(pnpm test*)",
+ "Bash(pnpm test-unit *)",
+ "Bash(pnpm test-e2e *)",Based on learnings: “For frontend tests, run commands via pnpm scripts … Always use pnpm test-unit, never pnpm vitest ... or npx vitest ...”.
🤖 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 `@template/.claude/settings/permissions/bash.jsonc` at line 117, The current
permission "Bash(pnpm test*)" is too broad; restrict it to explicit allowed test
scripts by replacing the wildcard with concrete script names (e.g., "Bash(pnpm
test-unit)" and any other approved scripts like "Bash(pnpm test-integration)" if
needed). Edit the permissions entry that currently references Bash(pnpm test*)
to list only the exact pnpm test script names your project uses (match
package.json script keys) so only those known-safe commands can run. Ensure you
update any related documentation or comments to note the approved script names
and avoid allowing direct invocations like "pnpm vitest" or "npx vitest".
| return [item for page in data for item in page] | ||
|
|
||
|
|
||
| def gh_graphql(query: str, variables: dict[str, str | int]) -> Any: # noqa: ANN401 |
There was a problem hiding this comment.
Add explanation to noqa comment.
The noqa: ANN401 directive is missing an explanation. As per coding guidelines, when disabling a linting rule, provide a comment describing the reasoning.
📝 Suggested fix
-def gh_graphql(query: str, variables: dict[str, str | int]) -> Any: # noqa: ANN401
+def gh_graphql(query: str, variables: dict[str, str | int]) -> Any: # noqa: ANN401 — return type is genuinely unknown JSONAs per coding guidelines, when disabling a linting rule with an inline directive, provide a comment describing the reasoning for disabling the rule.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def gh_graphql(query: str, variables: dict[str, str | int]) -> Any: # noqa: ANN401 | |
| def gh_graphql(query: str, variables: dict[str, str | int]) -> Any: # noqa: ANN401 — return type is genuinely unknown JSON |
🤖 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 `@template/.claude/skills/address-pr-comments/fetch-pr-comments.py` at line 84,
The inline noqa directive on the function signature gh_graphql(query: str,
variables: dict[str, str | int]) -> Any currently disables ANN401 without
explanation; update that line to keep the noqa but append a brief comment
explaining the rationale (e.g., why return type Any is acceptable or why precise
annotation isn’t feasible here) so the noqa has a justification, referencing the
gh_graphql function and ANN401 rule in the comment.
Comment on lines
+90
to
+91
| result = subprocess.run( # noqa: S603 | ||
| args, |
There was a problem hiding this comment.
Add explanation to noqa comment.
The noqa: S603 directive is missing an explanation. As per coding guidelines, when disabling a linting rule, provide a comment describing the reasoning.
📝 Suggested fix
- result = subprocess.run( # noqa: S603
+ result = subprocess.run( # noqa: S603 — args constructed from validated inputsAs per coding guidelines, when disabling a linting rule with an inline directive, provide a comment describing the reasoning for disabling the rule.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| result = subprocess.run( # noqa: S603 | |
| args, | |
| result = subprocess.run( # noqa: S603 — args constructed from validated inputs | |
| args, |
🤖 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 `@template/.claude/skills/address-pr-comments/fetch-pr-comments.py` around
lines 90 - 91, The inline "noqa: S603" on the subprocess.run call lacks an
explanation; update the subprocess.run invocation (the line with "result =
subprocess.run(... args,") to retain the noqa but add a short comment explaining
why rule S603 is being suppressed (e.g., safe use of subprocess with validated
args or controlled input), mentioning the rationale and any mitigation (input
validation/escaping) so reviewers can see why subprocess.run is acceptable here.
Comment on lines
+52
to
+64
| env: | ||
| MERGE_MSG: ${{ github.event.merge_group.head_commit.message }} | ||
| run: | | ||
|
|
||
| [[ '${{ github.event_name }}' = 'pull_request' ]] && full_number=${{ github.event.number }} | ||
|
|
||
| # example message in merge group context: "Merge pull request #10 from org-name/branch-name\n\ncommit message" | ||
| [[ '${{ github.event_name }}' = 'merge_group' ]] && message='${{ github.event.merge_group.head_commit.message }}' && echo Extracting from $message && number_and_following_text=${message##*#} && full_number=${number_and_following_text%%[!0-9]*} | ||
| if [[ '${{ github.event_name }}' = 'merge_group' ]]; then | ||
| message="$MERGE_MSG" | ||
| echo "Extracting from $message" | ||
| number_and_following_text="${message##*#}" | ||
| full_number="${number_and_following_text%%[!0-9]*}" | ||
| fi |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider moving all GitHub context values to environment variables for consistency.
The refactoring to use MERGE_MSG env variable for the merge commit message (lines 52-53) is a security improvement because GitHub Actions automatically escapes env variables, preventing template injection. However, line 56 still directly expands ${{ github.event.number }} in the run block.
For defense-in-depth and consistency, consider moving github.event.number to an env variable as well:
🔒 Proposed hardening
env:
MERGE_MSG: ${{ github.event.merge_group.head_commit.message }}
+ PR_NUMBER: ${{ github.event.number }}
run: |
- [[ '${{ github.event_name }}' = 'pull_request' ]] && full_number=${{ github.event.number }}
+ [[ '${{ github.event_name }}' = 'pull_request' ]] && full_number="$PR_NUMBER"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| env: | |
| MERGE_MSG: ${{ github.event.merge_group.head_commit.message }} | |
| run: | | |
| [[ '${{ github.event_name }}' = 'pull_request' ]] && full_number=${{ github.event.number }} | |
| # example message in merge group context: "Merge pull request #10 from org-name/branch-name\n\ncommit message" | |
| [[ '${{ github.event_name }}' = 'merge_group' ]] && message='${{ github.event.merge_group.head_commit.message }}' && echo Extracting from $message && number_and_following_text=${message##*#} && full_number=${number_and_following_text%%[!0-9]*} | |
| if [[ '${{ github.event_name }}' = 'merge_group' ]]; then | |
| message="$MERGE_MSG" | |
| echo "Extracting from $message" | |
| number_and_following_text="${message##*#}" | |
| full_number="${number_and_following_text%%[!0-9]*}" | |
| fi | |
| env: | |
| MERGE_MSG: ${{ github.event.merge_group.head_commit.message }} | |
| PR_NUMBER: ${{ github.event.number }} | |
| run: | | |
| [[ '${{ github.event_name }}' = 'pull_request' ]] && full_number="$PR_NUMBER" | |
| # example message in merge group context: "Merge pull request `#10` from org-name/branch-name\n\ncommit message" | |
| if [[ '${{ github.event_name }}' = 'merge_group' ]]; then | |
| message="$MERGE_MSG" | |
| echo "Extracting from $message" | |
| number_and_following_text="${message##*#}" | |
| full_number="${number_and_following_text%%[!0-9]*}" | |
| fi |
🤖 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 `@template/.github/workflows/get-values.yaml` around lines 52 - 64, The
workflow currently expands ${{ github.event.number }} directly inside the run
block; add a new environment variable (e.g., PR_NUMBER) at the same env level as
MERGE_MSG and set it to ${{ github.event.number }}, then update the run script
to reference $PR_NUMBER instead of expanding github.event.number inline (adjust
the conditional that sets full_number from full_number=${{ github.event.number
}} to full_number=$PR_NUMBER and keep the existing merge_group extraction logic
using $MERGE_MSG and $full_number). Ensure the conditional comparison of '${{
github.event_name }}' remains unchanged but any occurrence of inline
github.event.* values in the run block is replaced with corresponding env vars
like PR_NUMBER for consistency and safer escaping.
Comment on lines
+6
to
+10
| def run_copier_task( | ||
| script_path: Path, | ||
| *args: str, | ||
| env: dict[str, str] | None = None, | ||
| ) -> subprocess.CompletedProcess[str]: |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Make run_copier_task keyword-only to match project convention.
Line 6 exposes positional parameters for a multi-argument helper. Convert this API to keyword-only and update callers for explicitness and consistency.
Proposed refactor
diff --git a/tests/unit/copier_tasks/helpers.py b/tests/unit/copier_tasks/helpers.py
@@
-def run_copier_task(
- script_path: Path,
- *args: str,
- env: dict[str, str] | None = None,
-) -> subprocess.CompletedProcess[str]:
+def run_copier_task(
+ *,
+ script_path: Path,
+ args: tuple[str, ...] = (),
+ env: dict[str, str] | None = None,
+) -> subprocess.CompletedProcess[str]:
return subprocess.run( # noqa: S603 -- these are our own scripts
- [sys.executable, str(script_path), *args],
+ [sys.executable, str(script_path), *args],
check=False,
capture_output=True,
text=True,
env=env,
)diff --git a/tests/unit/copier_tasks/test_remove_precommit_hooks.py b/tests/unit/copier_tasks/test_remove_precommit_hooks.py
@@
- return run_copier_task(_SCRIPT_PATH, "--hook-id-regex", hook_id_regex, "--target-file", str(target_file))
+ return run_copier_task(
+ script_path=_SCRIPT_PATH,
+ args=("--hook-id-regex", hook_id_regex, "--target-file", str(target_file)),
+ )As per coding guidelines, "**/*.{py,ts,tsx}: Prefer keyword-only parameters (unless a very clear single-argument function): use * in Python signatures."
🤖 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/helpers.py` around lines 6 - 10, Change
run_copier_task so its multi-argument API is keyword-only: update the function
signature (run_copier_task) to force callers to pass script_path, args (or a
single variadic collection param) and env by name (use a leading * in the
signature) and keep the return type subprocess.CompletedProcess[str]; then
update all callers to call run_copier_task(script_path=..., args=..., env=...)
(or script_path=... and positional args moved into an explicit args parameter)
to match the new signature and types.
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.
Pull in upstream
Also added pytest-mock to dev dependencies in template
Summary by CodeRabbit
New Features
Chores
Documentation