Enhancement: package prs devcontainer tool#2427
Conversation
…entation on the feature, and an update to help-dev to reference the new tool
📝 WalkthroughWalkthroughAdds a new Bash CLI script Changespackage-prs Tool
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
Reviewer's GuideAdds a new devcontainer CLI script Flow diagram for package-prs cherry-pick workflowflowchart LR
User["Run package-prs with --pr, --source-repo, --target-base, --branch"]
CheckTree["Check working tree is clean"]
Fetch["git fetch <source-remote> <source-branch>"]
ResolvePRs["Resolve PR merge commits via git log --grep"]
VerifySubjects["Verify merge commit subjects and owners"]
DryRun{Dry run?}
Plan["Print plan table (PR, SHA, subject, stats)"]
CreateBranch["Create branch from --target-base"]
CherryPick["git cherry-pick -m 1 for each merge commit"]
Conflict{Cherry-pick conflict?}
Abort["Abort cherry-pick and exit non-zero"]
Success["Print suggested next steps (e.g., push, gh pr create)"]
User --> CheckTree --> Fetch --> ResolvePRs --> VerifySubjects --> DryRun
DryRun -->|yes| Plan --> Success
DryRun -->|no| CreateBranch --> CherryPick --> Conflict
Conflict -->|yes| Abort
Conflict -->|no| Success
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new Bash script, package-prs, designed to automate the creation of 'package' branches by cherry-picking merge commits from a list of pull requests. It includes documentation and integration into the development help script. Feedback highlights several areas for improvement: the owner validation check is too restrictive for PRs from forks, the PR list parsing is vulnerable to empty strings which could lead to incorrect commit matching, and the use of '|' as a delimiter for internal data passing is fragile. Additionally, the script requires the GitHub CLI (gh) despite not using it for core logic, and it lacks a mechanism to ensure the target base reference is up to date before branching.
|
@SourceryAI review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.devcontainer/development/scripts/package-prs:
- Around line 55-67: The case block directly assigns "$2" to variables like
PR_LIST, SOURCE_REPO, SOURCE_BRANCH, SOURCE_REMOTE, TARGET_BASE, and BRANCH
which will trigger an unbound-variable error under set -u if a flag is the last
arg; add a small guard function (e.g., ensure_arg_or_err) or inline checks
before each assignment to verify that a next positional argument exists ($# -ge
2 or test -n "${2-}") and call err/usage when missing, then perform the
shift(s); update the --pr, --source-repo, --source-branch, --source-remote,
--target-base, and --branch branches in the case to use that guard so flags
without values produce a friendly error instead of crashing.
- Around line 82-83: Remove the hard failure on missing GitHub CLI by deleting
or making conditional the `command -v gh >/dev/null || { err "gh (GitHub CLI) is
required"; exit 1; }` check; keep `command -v git` as the only mandatory
dependency. Additionally, update the post-run suggestion that mentions `gh` (the
message printed around line ~227) to either only show that suggestion when
`command -v gh` succeeds or replace it with a generic Git-based next-step (e.g.,
push and open a PR via the GitHub web UI) so the script does not assume `gh` is
installed. Ensure references to `command -v gh` and the post-run suggestion
string are updated consistently.
In @.devcontainer/development/scripts/package-prs.md:
- Around line 63-69: The fenced sample-output block in the markdown lacks a
language tag (MD040); update the triple-backtick fence that contains the
PR/SHA/SUBJECT/STATS table so it begins with ```text (i.e., add the "text"
language identifier to the opening ``` fence) to mark the block as plain output.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5fa70dd7-7d22-4efb-a330-e9f0e6b68874
📒 Files selected for processing (3)
.devcontainer/development/scripts/help-dev.devcontainer/development/scripts/package-prs.devcontainer/development/scripts/package-prs.md
…empty PRs and missing flag values, warn on fork PRs, fetch target base, tab-delimit resolve_one output, mark md code fence)
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@SourceryAI review |
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 (1)
.devcontainer/development/scripts/package-prs (1)
126-140:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate that each PR token is a positive integer before interpolating into
--grep.
$pris splatted directly into the regex passed togit log --grep(line 132).git log --grepuses basic regex by default, so a malformed PR token (e.g.,12.3,12*,1[23]) silently matches unintended merges or yields confusing results. A one-line numeric check up front turns these cases into a clean error.🛡️ Proposed guard
resolve_one() { local pr="$1" local subject sha stat + if [[ ! "$pr" =~ ^[0-9]+$ ]]; then + err "invalid PR number: '${pr}' (expected a positive integer)" + return 1 + fi + # Look up by PR number in the merge commit subject sha=$(git log --merges \🤖 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 @.devcontainer/development/scripts/package-prs around lines 126 - 140, In resolve_one(), validate the incoming PR token ($pr) is a positive integer before it’s interpolated into the git --grep pattern: add a guard right after the function entry that checks $pr against a digits-only regex (e.g. ^[0-9]+$) and on failure emit an err message and return non-zero; this prevents malformed tokens (like 12.3, 1[23], etc.) from being used in git log --grep and keeps the subsequent sha lookup safe.
🤖 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 @.devcontainer/development/scripts/package-prs:
- Around line 51-52: The info() helper writes to stdout and contaminates
resolve_one's output; change info() to write to stderr (redirect its printf to
>&2) or update the two info calls inside resolve_one (the fork-PR warning
branch) to redirect to stderr so resolve_one echoes only the single
tab-separated data line; update references to the info() helper or the specific
calls in resolve_one accordingly so command substitution (line=$(resolve_one
"$pr")) receives only the intended data.
- Around line 166-183: After splitting and filtering RAW_PRS into PRS, add a
post-loop guard to detect an effectively-empty PR list (e.g., inputs like "--pr
','") by checking if the PRS array is empty and aborting with a clear message;
locate the for loop that reads RAW_PRS into PRS/SHAS/SUBJECTS/STATS (uses
symbols RAW_PRS, PRS, SHAS, SUBJECTS, STATS and function resolve_one) and
immediately after that loop add a conditional that exits non-zero (with an err
message referencing the original PR_LIST/RAW_PRS) when PRS has zero elements to
prevent creating a 0-commit branch.
---
Outside diff comments:
In @.devcontainer/development/scripts/package-prs:
- Around line 126-140: In resolve_one(), validate the incoming PR token ($pr) is
a positive integer before it’s interpolated into the git --grep pattern: add a
guard right after the function entry that checks $pr against a digits-only regex
(e.g. ^[0-9]+$) and on failure emit an err message and return non-zero; this
prevents malformed tokens (like 12.3, 1[23], etc.) from being used in git log
--grep and keeps the subsequent sha lookup safe.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: bc002ecc-b276-443e-9cff-70eaad7272c4
📒 Files selected for processing (2)
.devcontainer/development/scripts/package-prs.devcontainer/development/scripts/package-prs.md
✅ Files skipped from review due to trivial changes (1)
- .devcontainer/development/scripts/package-prs.md
| err() { printf 'Error: %s\n' "$*" >&2; } | ||
| info() { printf '==> %s\n' "$*"; } |
There was a problem hiding this comment.
info() writes to stdout and corrupts resolve_one's captured output for fork PRs.
resolve_one is invoked via command substitution (line=$(resolve_one "$pr"), line 174), and its contract per the doc-comment on line 124 is to echo a single tab-separated data line. However, info() (line 52) writes to stdout, so when the fork-PR branch fires at lines 154–155, both info lines get prepended into $line. The read at line 178 only consumes the first line, so sha is assigned ==>, subject gets the warning text, and the real SHA/subject/stat data is dropped. The script then attempts git cherry-pick -m 1 "==>", which fails with a misleading "bad object" error.
This is a regression introduced by the new fork-warning feature in this PR.
🛠️ Suggested fix — route `info` to stderr
err() { printf 'Error: %s\n' "$*" >&2; }
-info() { printf '==> %s\n' "$*"; }
+info() { printf '==> %s\n' "$*" >&2; }Alternatively, redirect just the two calls inside resolve_one:
if [[ "$subject" != *"from ${SOURCE_OWNER}/"* ]]; then
- info "PR #${pr}: merge subject not 'from ${SOURCE_OWNER}/...' (likely a fork PR)"
- info " got: $subject"
+ info "PR #${pr}: merge subject not 'from ${SOURCE_OWNER}/...' (likely a fork PR)" >&2
+ info " got: $subject" >&2
fiAlso applies to: 153-156, 174-178
🤖 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 @.devcontainer/development/scripts/package-prs around lines 51 - 52, The
info() helper writes to stdout and contaminates resolve_one's output; change
info() to write to stderr (redirect its printf to >&2) or update the two info
calls inside resolve_one (the fork-PR warning branch) to redirect to stderr so
resolve_one echoes only the single tab-separated data line; update references to
the info() helper or the specific calls in resolve_one accordingly so command
substitution (line=$(resolve_one "$pr")) receives only the intended data.
| info "Resolving merge SHAs for PRs: ${PR_LIST}" | ||
|
|
||
| IFS=',' read -ra RAW_PRS <<< "$PR_LIST" | ||
| declare -a PRS=() SHAS=() SUBJECTS=() STATS=() | ||
|
|
||
| for pr in "${RAW_PRS[@]}"; do | ||
| # Skip empty elements from a stray comma in --pr (e.g., "1,,2" or ",1"). | ||
| [[ -z "$pr" ]] && continue | ||
| if ! line=$(resolve_one "$pr"); then | ||
| err "aborting; resolution failed for PR #${pr}" | ||
| exit 1 | ||
| fi | ||
| IFS=$'\t' read -r sha subject stat <<< "$line" | ||
| PRS+=("$pr") | ||
| SHAS+=("$sha") | ||
| SUBJECTS+=("$subject") | ||
| STATS+=("$stat") | ||
| done |
There was a problem hiding this comment.
Guard against an effectively-empty PR list after normalization.
PR_LIST non-emptiness is checked pre-normalization (line 79), but inputs like --pr "," or --pr ",,," survive that check, get whitespace-stripped, then every element is skipped by the empty-token filter at line 173. The loop completes with empty arrays, the plan table prints just a header, and the script proceeds to create a 0-commit branch. Adding a post-loop assertion keeps the error message close to the user's intent.
🛡️ Proposed guard
done
+
+if (( ${`#PRS`[@]} == 0 )); then
+ err "no valid PR numbers in --pr='${PR_LIST}'"
+ exit 2
+fi
# --------------------------------------------------------------- print plan🤖 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 @.devcontainer/development/scripts/package-prs around lines 166 - 183, After
splitting and filtering RAW_PRS into PRS, add a post-loop guard to detect an
effectively-empty PR list (e.g., inputs like "--pr ','") by checking if the PRS
array is empty and aborting with a clear message; locate the for loop that reads
RAW_PRS into PRS/SHAS/SUBJECTS/STATS (uses symbols RAW_PRS, PRS, SHAS, SUBJECTS,
STATS and function resolve_one) and immediately after that loop add a
conditional that exits non-zero (with an err message referencing the original
PR_LIST/RAW_PRS) when PRS has zero elements to prevent creating a 0-commit
branch.
1 participant
Summary by Sourcery
Introduce a devcontainer-only CLI for assembling package branches from multiple source PRs and document its usage and limitations.
New Features:
Documentation:
Summary by cubic
Adds
package-prs, a devcontainer CLI to build a single “package” branch by cherry‑picking merge commits from multiple PRs. Includes docs, ahelp-deventry, and fixes for safer arg handling and remote fetching.New Features
git logand cherry‑picks with-m 1onto a branch from--target-base.--dry-run,--force,--source-branch, and--source-remote; checks clean working tree and verifies merge subjects (warns on owner mismatch).package-prs.mdwith usage and recovery steps;help-devnow listspackage-prs --help.Bug Fixes
ghrequirement; only suggestsgh pr createif available.--pr; warn on fork PRs rather than fail.--target-baseremote when specified to avoid stale refs; more robust plan output; fixed code fences in docs.Written for commit 4a1c77a. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Documentation