Skip to content

tools: pin pr-review-cycle rebase target to fork/trunk, not origin/trunk#33

Merged
dognose24 merged 4 commits into
trunkfrom
tools/pr-review-cycle-fork-rebase-target
May 15, 2026
Merged

tools: pin pr-review-cycle rebase target to fork/trunk, not origin/trunk#33
dognose24 merged 4 commits into
trunkfrom
tools/pr-review-cycle-fork-rebase-target

Conversation

@dognose24
Copy link
Copy Markdown
Owner

Fixes RSM-3270

Proposed changes

Pin /jetpack-pr-review-cycle's rebase target to fork/trunk (dognose24/jetpack) instead of the previous origin/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 scripts
  • tools/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 stubs

The fork is currently ~309 commits behind upstream and accumulates harness work on top of that gap. Rebasing onto origin/trunk treats every fork-only file as "deleted by us" during conflict resolution. If the agent accepts that during git rebase --continue, a subsequent git push --force-with-lease destroys 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/trunk mid-cycle, got cherry-pick conflicts complaining tools/ai-sandbox/wp-verify/tests/pie-chart-tooltip.spec.ts and tools/ai-sandbox/wp-verify/tests/dashboard-mount.spec.ts were "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

    • Replace four origin/trunk references with fork/trunk (the git fetch, the BEHIND calculation, the MERGEABLE rebase, the CONFLICTING rebase).
    • git fetch origin trunkgit fetch fork trunk.
    • Add an explanatory paragraph at the top of step (e) calling out the fork-only-infrastructure risk so future contributors can't reach for origin/trunk reflexively.
  • HARD rules

    • Add: "Rebase target is fork/trunk, never origin/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 use origin/trunk surfaces.
    • Update the existing "never let a round end with the branch behind origin/trunk" rule to reference fork/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 against fork/trunk. Confirm a cat .claude/commands/jetpack-pr-review-cycle.md | grep origin/trunk shows only the cautionary paragraph references, not any executable command.

Related

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>
@github-actions github-actions Bot added the Docs label May 15, 2026
@github-actions
Copy link
Copy Markdown

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • ✅ Include a description of your PR changes.
  • 🔴 Add a "[Status]" label (In Progress, Needs Review, ...).
  • ✅ Add testing instructions.
  • ✅ Specify whether this PR includes any changes to data or privacy.
  • ✅ Add changelog entries to affected projects

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:

  1. Ensure all required checks appearing at the bottom of this PR are passing.
  2. Make sure to test your changes on all platforms that it applies to. You're responsible for the quality of the code you ship.
  3. You can use GitHub's Reviewers functionality to request a review.
  4. When it's reviewed and merged, you will be pinged in Slack to deploy the changes to WordPress.com simple once the build is done.

If you have questions about anything, reach out in #jetpack-developers for guidance!

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/trunk to fork/trunk.
  • Adds explicit rationale/warnings in step (e) and HARD rules to prevent accidental rebases onto upstream.

Comment on lines +103 to 104

```bash
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Comment on lines +106 to +107
# remote name; a checkout missing it would silently fall back to `origin` or
# fail mid-rebase. Stop early with a clear setup hint instead.
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Comment on lines +110 to +111
echo "ERROR: 'fork' git remote not configured. Set it up once with:"
echo " git remote add fork git@github.com:dognose24/jetpack.git"
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
@dognose24 dognose24 requested a review from Copilot May 15, 2026 03:23
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

@dognose24 dognose24 merged commit 1087310 into trunk May 15, 2026
62 of 63 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants