fix(pr-bot): classify renames of core paths as core#506
Conversation
trust-gate.yml, auto-merge.yml, and comment-command.yml built the
changed-file list from `gh pr view --json files`, which is backed by
the GraphQL PullRequestFile type and carries no previous-filename
field. When GitHub detects a rename, only the new path appears there,
so `git mv src/vouch/http_server.py src/vouch/web_server.py` (plus any
edit inside it) made the changed core file invisible to pr_bot's exact-
match CORE_GLOBS check: trust-gate reported no core paths touched for
an untrusted author, and the PR was eligible for auto-merge arming as
klass=code instead of being permanently blocked as klass=core.
switch all three workflows to the REST pulls/{n}/files endpoint, which
does populate previous_filename on renamed entries, and add a
`changed-files` pr_bot subcommand that flattens that payload into both
the new and previous filename so a rename can no longer drop a core
path off the radar.
Fixes vouchdev#505
WalkthroughChanged-file extraction now uses GitHub’s paginated REST pull-request files endpoint and preserves both current and previous filenames for renames. Three workflows invoke the new CLI command before existing classification and gating logic. ChangesRename-aware classification
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
github-actions
Bot
added
docs
plind-junior
added
the
auto-merge
…lassification-bypass # Conflicts: # .github/workflows/comment-command.yml
Head branch was pushed to by a user without write access
github-actions
Bot
removed
the
auto-merge
|
new commits were pushed after authorization, so the prior verification no longer matches the head. auto-merge is disarmed and the label removed — re-authorize (add the auto-merge label or comment /auto-merge) to re-verify the new commits. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@src/vouch/pr_bot.py`:
- Around line 171-178: Update the docstring near the REST files payload
flattening logic to use lowercase prose throughout, including its descriptive
sentences and references to callers. Preserve the existing technical terms,
command examples, and behavior documentation unchanged.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 161634fc-2a41-4137-9ae7-5988e842fcfd
📒 Files selected for processing (6)
.github/workflows/auto-merge.yml.github/workflows/comment-command.yml.github/workflows/trust-gate.ymlCHANGELOG.mdsrc/vouch/pr_bot.pytests/test_pr_bot.py
| """Flatten a REST ``/pulls/{n}/files`` payload into a path list. | ||
|
|
||
| Emits both ``filename`` and, for a renamed entry, ``previous_filename`` — | ||
| a rename that lands a core path under a new name must still classify as | ||
| core. ``gh pr view --json files`` (the GraphQL-backed shortcut) carries no | ||
| previous-filename field and silently drops this; callers must use the | ||
| REST files endpoint (``gh api repos/{o}/{r}/pulls/{n}/files``) instead. | ||
| """ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use lowercase prose in the docstring.
The docstring uses standard capitalization, which violates the repository's convention. As per path instructions for src/vouch/**, use lowercase prose in comments and review notes.
♻️ Proposed fix
- """Flatten a REST ``/pulls/{n}/files`` payload into a path list.
-
- Emits both ``filename`` and, for a renamed entry, ``previous_filename`` —
- a rename that lands a core path under a new name must still classify as
- core. ``gh pr view --json files`` (the GraphQL-backed shortcut) carries no
- previous-filename field and silently drops this; callers must use the
- REST files endpoint (``gh api repos/{o}/{r}/pulls/{n}/files``) instead.
- """
+ """flatten a rest ``/pulls/{n}/files`` payload into a path list.
+
+ emits both ``filename`` and, for a renamed entry, ``previous_filename`` —
+ a rename that lands a core path under a new name must still classify as
+ core. ``gh pr view --json files`` (the graphql-backed shortcut) carries no
+ previous-filename field and silently drops this; callers must use the
+ rest files endpoint (``gh api repos/{o}/{r}/pulls/{n}/files``) instead.
+ """📝 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.
| """Flatten a REST ``/pulls/{n}/files`` payload into a path list. | |
| Emits both ``filename`` and, for a renamed entry, ``previous_filename`` — | |
| a rename that lands a core path under a new name must still classify as | |
| core. ``gh pr view --json files`` (the GraphQL-backed shortcut) carries no | |
| previous-filename field and silently drops this; callers must use the | |
| REST files endpoint (``gh api repos/{o}/{r}/pulls/{n}/files``) instead. | |
| """ | |
| """flatten a rest ``/pulls/{n}/files`` payload into a path list. | |
| emits both ``filename`` and, for a renamed entry, ``previous_filename`` — | |
| a rename that lands a core path under a new name must still classify as | |
| core. ``gh pr view --json files`` (the graphql-backed shortcut) carries no | |
| previous-filename field and silently drops this; callers must use the | |
| rest files endpoint (``gh api repos/{o}/{r}/pulls/{n}/files``) instead. | |
| """ |
🤖 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 `@src/vouch/pr_bot.py` around lines 171 - 178, Update the docstring near the
REST files payload flattening logic to use lowercase prose throughout, including
its descriptive sentences and references to callers. Preserve the existing
technical terms, command examples, and behavior documentation unchanged.
Source: Path instructions
matches house style per CodeRabbit review on vouchdev#506.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/vouch/pr_bot.py (1)
170-189: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winhandle paginated files payloads consistently.
the workflows write
gh api ... --paginateoutput straight tofiles.json, so multi-page prs produce multiple json arrays here.json.loads()will fail on that shape, and changed-file classification will break for larger prs. add--slurpat the call sites or flatten page arrays inextract_changed_paths()before parsing.🤖 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 `@src/vouch/pr_bot.py` around lines 170 - 189, Update extract_changed_paths to accept paginated REST output containing multiple JSON arrays, flattening all page entries before collecting filename and previous_filename values. Preserve handling of the existing single-array payload and the rename path behavior, or alternatively add --slurp consistently at every workflow call site that writes files.json.Source: MCP tools
🤖 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.
Outside diff comments:
In `@src/vouch/pr_bot.py`:
- Around line 170-189: Update extract_changed_paths to accept paginated REST
output containing multiple JSON arrays, flattening all page entries before
collecting filename and previous_filename values. Preserve handling of the
existing single-array payload and the rename path behavior, or alternatively add
--slurp consistently at every workflow call site that writes files.json.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 764c3541-bc67-4147-8b6a-929edecfb885
📒 Files selected for processing (1)
src/vouch/pr_bot.py
2 participants
What changed
trust-gate.yml,auto-merge.yml, andcomment-command.ymlnow build thechanged-file list from the REST
pulls/{n}/filesendpoint(
gh api repos/{o}/{r}/pulls/{n}/files --paginate) instead ofgh pr view --json files, and feed both the new and pre-rename filenameinto
pr_bot's classification. A newpr_bot changed-filessubcommandflattens that REST payload (
filename+previous_filenamewhen present)into the plain one-path-per-line format the rest of
pr_botalreadyconsumes.
Why
gh pr view --json filesis backed by the GraphQLPullRequestFiletype,which carries only
path/additions/deletions— no previous-filenamefield. When GitHub detects a rename, only the new path appears in that
list.
pr_bot._match()matchesCORE_GLOBSentries by exact path, sogit mv src/vouch/http_server.py src/vouch/web_server.py(plus any editinside it) made the changed core file invisible to classification:
trust-gate.ymlreported "untrusted author, no core paths touched — ok",and the renamed PR was eligible for auto-merge arming as
klass=codeinstead of being permanently blocked as
klass=core— a bypass of theone detection mechanism that's supposed to guarantee "a PR touching [a
core path] needs the owner's review and is never merged by automation"
(
pr_bot.py's own docstring).The REST files endpoint does populate
previous_filenameon renamedentries, so switching the source and folding both filenames into the
classified list closes the gap without changing
pr_bot's classificationlogic itself.
Fixes #505
What might break
Nothing for users with an existing
.vouch/directory — this only touchesCI workflow files and
pr_bot.py's CLI surface (classify,core-touched,should-armall behave identically for non-renamed filelists; the new
changed-filessubcommand is additive). Nokb.*method,object model, or on-disk shape changes.
VEP
Not applicable — no object model,
kb.*method, on-disk layout, bundleformat, or audit-log shape change. This is CI-workflow and
pr_bot.pyCLI-surface logic only, and this PR itself touches
.github/**/pr_bot.py(bothCORE_GLOBSpaths), so per the trust-gate's own rule itneeds owner review and cannot self-auto-merge.
Tests
make checklocally: ruff clean; mypy clean onpr_bot.py; all 25tests/test_pr_bot.pycases pass (21 pre-existing + 4 new)test_extract_changed_paths_plain_file,test_extract_changed_paths_includes_previous_filename_on_rename,test_rename_of_core_path_still_classifies_core,test_cli_changed_files_emits_previous_filenameCHANGELOG.mdupdated under## [Unreleased]yaml.safe_load)Summary by CodeRabbit
New Features
changed-filesmode to output newline-delimited changed paths for a pull request from a provided file-list payload.Bug Fixes
Documentation