tools: pin pr-review-cycle rebase target to fork/trunk, not origin/trunk#33
Conversation
The skill said "rebase on fresh trunk" and used `origin/trunk` everywhere — fine for a workflow where PRs target Automattic/jetpack, dangerous for this one. PRs in this workflow land on dognose24/jetpack (the `fork` remote), which carries fork-only harness infrastructure (tools/ai-sandbox/**, .agents/skills/**, .claude/commands/**, the entire wp-verify suite, etc.). `origin/trunk` (Automattic) lags none of these — they don't exist there. Rebasing onto origin/trunk treats fork-only files as "deleted by us"; a subsequent force-push destroys the harness on the PR branch. Near-miss in PR #30: agent reached for `git rebase origin/trunk` mid-cycle, got cherry-pick conflicts complaining the dashboard-mount.spec.ts and pie-chart-tooltip.spec.ts files were "deleted in HEAD", noticed the skip would erase the verify-ui suite, and aborted manually. Without that awareness, the force-push would have landed. Changes: * Step (e) — replace four `origin/trunk` references with `fork/trunk`; replace `git fetch origin trunk` with `git fetch fork trunk`. Add an explanatory paragraph at the top of step (e) explaining the fork-only-infrastructure risk. * HARD rules — add a rule pinning rebase target to fork/trunk with explicit explanation of why; update the existing "never let a round end behind trunk" rule to reference fork/trunk. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
There was a problem hiding this comment.
Pull request overview
This PR updates the /jetpack-pr-review-cycle slash command to rebase PR branches onto the fork’s trunk (intended to preserve fork-only harness infrastructure) instead of rebasing onto Automattic upstream.
Changes:
- Switches the step (e) rebase/fetch target from
origin/trunktofork/trunk. - Adds explicit rationale/warnings in step (e) and HARD rules to prevent accidental rebases onto upstream.
|
|
||
| ```bash |
There was a problem hiding this comment.
Good catch — fixed in a67dca1. Step (e) now starts with git remote get-url fork >/dev/null 2>&1 || { echo ERROR + setup hint; exit 1; }. If the fork remote isn't configured the skill exits 1 with a one-line setup instruction (git remote add fork git@github.com:dognose24/jetpack.git), instead of falling back to wrong behavior or failing opaquely later in the rebase logic.
The skill stays hardcoded to the fork remote name (matches the user's two-remote convention: origin → Automattic upstream, fork → dognose24 fork). The pre-flight makes that assumption explicit at the entry point of step (e).
Addresses Copilot review on PR #33: `git fetch fork trunk` / `fork/trunk` assumes a `fork` git remote is configured. A checkout missing it would either silently fall back to wrong behavior or fail mid-rebase with a less-clear error. Add a pre-flight `git remote get-url fork` check at the top of step (e) that exits 1 with a one-line setup hint (`git remote add fork git@github.com:dognose24/jetpack.git` + `git fetch fork`) if the remote isn't there. The skill is hardcoded to a single-remote-name convention (matches the user's two-remote setup: `origin` → Automattic upstream, `fork` → dognose24 fork). The pre-flight makes the assumption explicit rather than letting it fail opaquely deep in the rebase logic. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| # remote name; a checkout missing it would silently fall back to `origin` or | ||
| # fail mid-rebase. Stop early with a clear setup hint instead. |
There was a problem hiding this comment.
Fair — fixed in 38af40c. The comment now accurately describes what happens: git fetch fork trunk errors immediately with fatal: 'fork' does not appear to be a git repository, no silent fallback. The pre-flight catches that deep-but-clear error and replaces it with a one-line setup hint at the top of step (e).
Addresses Copilot review on PR #33: the comment said a missing `fork` remote would "silently fall back to `origin` or fail mid-rebase", but `git fetch fork trunk` actually errors immediately with `fatal: 'fork' does not appear to be a git repository`. No silent fallback. Update the wording so readers understand what the pre-flight is protecting against (a clear-but-deep error in step (e)) rather than an inaccurate fallback scenario. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| echo "ERROR: 'fork' git remote not configured. Set it up once with:" | ||
| echo " git remote add fork git@github.com:dognose24/jetpack.git" |
There was a problem hiding this comment.
Fixed in 5ce50ba. Setup hint now shows both URLs explicitly:
git remote add fork https://github.com/dognose24/jetpack.git # HTTPS (works in sandboxes/CI without SSH keys)
git remote add fork git@github.com:dognose24/jetpack.git # SSH (when SSH keys are configured)
HTTPS listed first so the keyless case gets the path of least resistance. Followed by a single git fetch fork line.
Addresses Copilot review on PR #33: the setup hint only showed `git remote add fork git@github.com:...` (SSH). Environments without SSH keys configured (fresh sandboxes, CI) would have `git fetch fork` fail with an auth error, getting the user stuck on a step that should have a working alternative. Show both URLs explicitly: HTTPS first (works in keyless environments), SSH second (when keys are configured). Each with a one-line comment explaining when to pick which. The follow-up `git fetch fork` step stays one line. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Fixes RSM-3270
Proposed changes
Pin
/jetpack-pr-review-cycle's rebase target tofork/trunk(dognose24/jetpack) instead of the previousorigin/trunk(Automattic/jetpack).Why this matters
This workflow lands PRs on the fork, which carries fork-only harness infrastructure that doesn't exist on Automattic upstream:
tools/ai-sandbox/**— sandbox Dockerfile, wp-verify scriptstools/ai-sandbox/wp-verify/**— the entire Playwright Test suite (dashboard-mount.spec.ts,pie-chart-tooltip.spec.ts,playwright.config.ts,global-setup.ts, etc.).agents/skills/**— the premium-analytics agent skill bodies.claude/commands/**— the slash command stubsThe fork is currently ~309 commits behind upstream and accumulates harness work on top of that gap. Rebasing onto
origin/trunktreats every fork-only file as "deleted by us" during conflict resolution. If the agent accepts that duringgit rebase --continue, a subsequentgit push --force-with-leasedestroys the harness on the PR's branch — the worktree on disk is gone, and only reflog / other PRs would still hold the files.Near-miss
Hit this in PR #30 (dashboard-pie-chart sandbox run). The agent reached for
git rebase origin/trunkmid-cycle, got cherry-pick conflicts complainingtools/ai-sandbox/wp-verify/tests/pie-chart-tooltip.spec.tsandtools/ai-sandbox/wp-verify/tests/dashboard-mount.spec.tswere "deleted in HEAD", noticed the skip would erase the verify-ui suite the run depended on, and aborted manually. Without that observation the force-push would have landed.Changes
Step (e) — Keep the branch current with trunk
origin/trunkreferences withfork/trunk(thegit fetch, theBEHINDcalculation, theMERGEABLErebase, theCONFLICTINGrebase).git fetch origin trunk→git fetch fork trunk.origin/trunkreflexively.HARD rules
fork/trunk, neverorigin/trunk." with the rationale and a pointer to step (e). Includes the explicit instruction to stop and re-read step (e) if the impulse to useorigin/trunksurfaces.origin/trunk" rule to referencefork/trunk.Does this pull request change what data or activity we track or use?
No.
Testing instructions
After merge, in the sandbox or on host, invoke
/jetpack-pr-review-cycle <PR>against any open PR. Confirm step (e) fetches and rebases againstfork/trunk. Confirm acat .claude/commands/jetpack-pr-review-cycle.md | grep origin/trunkshows only the cautionary paragraph references, not any executable command.Related
local-trunk-tracks-fork— samefork/trunkvsorigin/trunkdistinction applies to the user's local trunk tracking config