Skip to content

fix(review): stop requiring routine PR rebases#589

Draft
hxy91819 wants to merge 1 commit into
mainfrom
investigate/rebase-main-review-rule
Draft

fix(review): stop requiring routine PR rebases#589
hxy91819 wants to merge 1 commit into
mainfrom
investigate/rebase-main-review-rule

Conversation

@hxy91819

Copy link
Copy Markdown
Member

Summary

  • stop treating an ordinary behind branch as a contributor blocker or recommending a routine rebase
  • reserve rebase/resolve guidance for conflicts, unavailable merge trees, or concrete merge-result integration failures
  • add a non-blocking maintainer-facing Base freshness metric when the merge base is at least 7 days old
  • avoid daily review-cache churn by invalidating only when freshness state or merge-base identity changes

Motivation

High-throughput repositories can advance by thousands of commits per day. Commit-count drift alone therefore creates noisy, repeated contributor work without proving that a PR is unsafe to merge. This keeps contributor action tied to concrete integration evidence while retaining a simple signal for genuinely old branches.

Impact

Only PRs whose GitHub mergeable state is behind trigger the compare lookup. A stale result adds review context and a non-blocking metric for maintainers or merge automation; it does not lower the PR rating or add contributor next steps. Compare failures fail soft.

Validation

  • Node 24.15.0: build passed
  • Node 24.15.0: 49 focused review prompt/content-cache tests passed
  • formatting, TypeScript compilation, and lint passed as part of pnpm run check
  • full pnpm run check did not complete because Codex runner tests reached the real Codex API and received 401 Unauthorized
  • autoreview was attempted with the bundled helper, but the same Codex API 401 prevented a review result

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. labels Jul 16, 2026
@clawsweeper

clawsweeper Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 24, 2026, 4:38 AM ET / 08:38 UTC.

ClawSweeper review

What this changes

This PR updates ClawSweeper’s PR-review guidance, base-drift metadata, cache invalidation, and focused tests so ordinary behind branches are non-blocking while conflicts and stale merge bases receive separate handling.

Merge readiness

⚠️ Ready for maintainer review - 3 items remain

Keep this PR open: the requested policy is still necessary and was explicitly adopted in the maintainer decision, but the current head is draft and has a real dirty merge state against current main, so it needs conflict resolution and a refreshed review before landing.

Priority: P2
Reviewed head: c0785de3001bd32587eb4d518f6fc6da818d9393

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) The policy direction has explicit maintainer approval and focused tests, but the dirty current-main merge state prevents treating the existing checks as final integration proof.
Proof confidence 🌊 off-meta tidepool Not applicable: This member-authored automation-policy PR is not in the external-contributor real-behavior-proof lane; its merge gate is instead a clean current-main integration plus refreshed focused validation.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: This member-authored automation-policy PR is not in the external-contributor real-behavior-proof lane; its merge gate is instead a clean current-main integration plus refreshed focused validation.
Evidence reviewed 5 items Maintainer policy decision: The maintainer-relayed decision explicitly adopts the proposed policy: ordinary behind branches are not contributor blockers, while conflicts and concrete merge-result failures remain valid reasons to request resolution.
Current merge state requires integration work: The supplied PR state reports mergeable: false, mergeableState: dirty, and a draft head. This is a concrete conflict case, not routine behind-branch drift, so conflict resolution is appropriate under the policy this PR adds.
Prompt behavior is narrowly aligned: The branch changes the review policy text to prohibit routine rebase advice for ordinary behind state, while retaining rebase or resolve guidance for dirty/conflicting merge states, unavailable merge trees, and concrete integration failures.
Findings None None.
Security None None.

How this fits together

ClawSweeper hydrates GitHub pull-request state, derives review context, and sends that context to Codex for a maintainer-facing decision. The changed path turns mergeability and merge-base information into prompt guidance, a review-cache key, and a non-blocking freshness metric.

flowchart LR
  A[Pull request metadata] --> B[Mergeability assessment]
  B --> C[Merge-base comparison]
  C --> D[Base freshness state]
  D --> E[Review prompt and cache digest]
  E --> F[Codex review output]
  F --> G[Maintainer-facing metric]
Loading

Before merge

  • Resolve merge risk (P1) - The current head is dirty against main; resolving conflicts can alter the prompt and cache changes, so the existing successful checks do not prove the final merged result.
  • Resolve merge risk (P1) - This PR changes review-context and cache behavior used by automation; the refreshed branch should preserve the focused cache-boundary coverage after conflict resolution.
  • Complete next step (P2) - The adopted policy remains useful, but the member-authored branch has a concrete merge conflict and needs a maintainer-reviewed refresh rather than automated cleanup or closure.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Review surface 4 files affected; 223 added, 9 removed The patch spans policy text, review metadata/cache behavior, and focused regression tests rather than a documentation-only change.
Recorded CI 13 completed checks succeeded The branch has passing hosted checks, but they predate resolution of the current dirty merge state.

Merge-risk options

Maintainer options:

  1. Refresh the conflicted branch (recommended)
    Resolve the current main conflicts, retain the adopted policy distinction between ordinary drift and real integration failures, and rerun the focused review-context/cache coverage before merge review.
  2. Pause for a narrower replacement
    If conflict resolution substantially changes the intended prompt or cache contract, close this branch only after a smaller current-main replacement preserves the maintainer-approved policy.

Technical review

Best possible solution:

Rebase onto current main, resolve the concrete conflicts while preserving the adopted non-blocking-behind policy, then rerun the focused prompt/cache tests and repository check before maintainer merge review.

Do we have a high-confidence way to reproduce the issue?

Not applicable: this PR changes review-automation policy rather than fixing a user-reported runtime failure. The relevant verification is the focused prompt and cache regression coverage plus a refreshed merge result.

Is this the best way to solve the issue?

Yes, the adopted distinction is the right direction: ordinary behind state should not create contributor churn, while actual conflicts and demonstrated integration failures still require resolution. The branch must first be reconciled with current main because its current merge tree is unavailable.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 822608f7ec54.

Labels

Label justifications:

  • P2: This is a bounded review-automation policy and cache change with repository-wide but non-emergency maintainer impact.
  • merge-risk: 🚨 automation: The PR changes the prompt and cache inputs that control automated review output, and its conflicted head must be refreshed before the merged behavior is known.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: This member-authored automation-policy PR is not in the external-contributor real-behavior-proof lane; its merge gate is instead a clean current-main integration plus refreshed focused validation.

Evidence

What I checked:

  • Maintainer policy decision: The maintainer-relayed decision explicitly adopts the proposed policy: ordinary behind branches are not contributor blockers, while conflicts and concrete merge-result failures remain valid reasons to request resolution.
  • Current merge state requires integration work: The supplied PR state reports mergeable: false, mergeableState: dirty, and a draft head. This is a concrete conflict case, not routine behind-branch drift, so conflict resolution is appropriate under the policy this PR adds. (c0785de3001b)
  • Prompt behavior is narrowly aligned: The branch changes the review policy text to prohibit routine rebase advice for ordinary behind state, while retaining rebase or resolve guidance for dirty/conflicting merge states, unavailable merge trees, and concrete integration failures. (prompts/review-item.md:234, c0785de3001b)
  • Cache behavior has focused regression coverage: The branch adds tests that invalidate review content when freshness crosses the seven-day threshold but ignore day-by-day age changes within the same freshness state, limiting avoidable review-cache churn. (test/review-content-cache.test.ts:131, c0785de3001b)
  • Adjacent queue repair is distinct: The related merged PR explicitly identifies this PR as open and unmerged but outside its publication-queue causal path; it is not a replacement for this review-policy work. (cf917b60a224)

Likely related people:

  • steipete: The recorded maintainer-relayed comment adopts this PR’s core policy and specifies the remaining readiness condition: rebase onto current main and land once the refreshed branch is green. (role: policy decision source; confidence: high; files: prompts/review-item.md, src/clawsweeper.ts)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Resolve the current main conflicts and rerun the focused review-context/cache tests plus pnpm run check on the refreshed head.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (1 earlier review cycle)
  • reviewed 2026-07-16T02:29:11.110Z sha c0785de :: needs maintainer review before merge. :: none

@steipete

Copy link
Copy Markdown
Contributor

Maintainer decision (relayed from @steipete): policy adopted. Behind-only branches stop being a contributor blocker; rebase/resolve guidance is reserved for conflicts, unavailable merge trees, and concrete merge-result integration failures, with the non-blocking base-freshness note at 7+ days as proposed. Please move this out of draft and rebase onto current main — the review prompt and cache have shifted since Jul 15. Happy to land it once CI is green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. P2 Normal priority bug or improvement with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants