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: found issues before merge. Reviewed August 2, 2026, 5:04 PM ET / 21:04 UTC.

ClawSweeper review

What this changes

This PR makes an ordinary behind-base PR non-blocking, adds a seven-day non-blocking base-freshness metric, and avoids cache churn from daily merge-base age changes.

Merge readiness

Blocked by patch quality or review findings - 7 items remain

Keep this PR open: the maintainer-approved policy is still needed, but the branch is draft and conflict-dirty, and its runtime changes target the former monolithic entrypoint rather than the active review modules on current main. Resolving the conflict mechanically could retain prompt text while leaving base-drift collection, digesting, and metric attachment inactive.

Priority: P1
Reviewed head: c0785de3001bd32587eb4d518f6fc6da818d9393

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The policy is well-supported, but the proposed implementation is not merge-ready because it misses the active review runtime.
Proof confidence 🌊 off-meta tidepool Not applicable: This member-authored PR is not subject to the external-contributor proof gate; its existing test claims remain supplemental and should be rerun against the rebuilt current-main path.
Patch quality 🦪 silver shellfish (2/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: This member-authored PR is not subject to the external-contributor proof gate; its existing test claims remain supplemental and should be rerun against the rebuilt current-main path.
Evidence reviewed 5 items Maintainer-approved direction: The July 23 maintainer decision explicitly adopts the proposed policy: ordinary behind-base state is not a contributor blocker, while a seven-day base-freshness note remains non-blocking.
Active cache implementation: Current main computes the review content digest in the extracted source-revision module, which has no pull-base-drift input; the branch instead adds that input only to the retired monolithic entrypoint.
Active runtime ownership: Current main composes the extracted source-revision and context-hydration subsystems into the review runtime; the branch does not modify those active modules.
Findings 1 actionable finding [P1] Port base-drift handling to the active review modules
Security None None.

How this fits together

ClawSweeper hydrates GitHub pull-request state, turns selected state into cache keys and a Codex review prompt, then publishes a structured review decision and metrics. This change affects how behind-base status is interpreted before a review is generated and reported.

flowchart TD
  A[GitHub pull request state] --> B[Review context hydration]
  B --> C[Content digest and cache]
  C --> D[Codex review prompt]
  D --> E[Structured review decision]
  E --> F[Metrics labels and review comment]
Loading

Before merge

  • Port base-drift handling to the active review modules (P1) - src/clawsweeper.ts was decomposed after this branch base: current main now hydrates context, computes cache digests, and runs reviews through focused modules. This new comparison helper is therefore not on the active path; resolve the conflict by rebuilding the behavior in those modules, or the merged prompt change can leave the intended runtime behavior absent.
  • Resolve merge risk (P1) - A mechanical conflict resolution can merge the prompt wording without wiring the base-drift behavior into the current hydration, digest, and review-runtime paths.
  • Resolve merge risk (P1) - The PR changes review automation semantics, so the rebuilt implementation needs focused validation against both clean-behind and conflict/integration-failure cases before landing.
  • Complete next step (P2) - A maintainer or current runtime owner must rebuild the approved behavior on the conflict-dirty draft branch; this is not a safe mechanical repair lane while the active architecture differs materially.
  • Improve patch quality - Port the logic and tests to the current hydration, source-revision, and runtime modules.
  • Improve patch quality - Resolve the dirty merge state only after the active-path port is complete.
  • Improve patch quality - Run focused current-main validation for clean-behind, stale-base, and genuine merge-conflict handling.

Findings

  • [P1] Port base-drift handling to the active review modules — src/clawsweeper.ts:5308-5355
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Branch scope 4 files changed; 223 added, 9 removed The runtime work is concentrated in a monolithic file that current main has substantially decomposed.
Active-module drift 3 active review modules absent from branch changes The current hydration, digest, and runtime paths must receive the behavior for the policy to take effect.

Merge-risk options

Maintainer options:

  1. Port the behavior into active modules (recommended)
    Reapply base-drift collection, digest normalization, and metric attachment to the current review modules, then validate the clean-behind and conflict paths before merging.
  2. Pause the stale branch
    Keep the approved policy direction but defer this branch if no owner can rebuild it against the decomposed runtime soon.

Technical review

Best possible solution:

Rebuild the approved policy in src/clawsweeper-context-hydration.ts, src/clawsweeper-source-revision.ts, and the active review runtime, then resolve the branch conflict and validate clean-behind, stale-base, and genuine-conflict behavior on current main.

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

Yes, source-reproducible: current main’s active hydration, digest, and runtime modules do not implement the branch’s pull-base-drift flow, while the branch adds it only to the retired entrypoint.

Is this the best way to solve the issue?

No. The policy direction is approved, but this branch is not the best implementation because its runtime changes do not reach the decomposed current-main review path.

Full review comments:

  • [P1] Port base-drift handling to the active review modules — src/clawsweeper.ts:5308-5355
    src/clawsweeper.ts was decomposed after this branch base: current main now hydrates context, computes cache digests, and runs reviews through focused modules. This new comparison helper is therefore not on the active path; resolve the conflict by rebuilding the behavior in those modules, or the merged prompt change can leave the intended runtime behavior absent.
    Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.99

AGENTS.md: found and applied where relevant.

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

Labels

Label changes:

  • add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: This member-authored PR is not subject to the external-contributor proof gate; its existing test claims remain supplemental and should be rerun against the rebuilt current-main path.
  • remove status: 📣 needs proof: Current PR status label is status: ⏳ waiting on author.

Label justifications:

  • P1: The PR concerns a recurring review-automation workflow that currently gives contributors incorrect rebase guidance.
  • merge-risk: 🚨 automation: Merging an incomplete port could alter review prompt wording while leaving the intended cache and metric behavior inactive.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🌊 off-meta tidepool and patch quality is 🦪 silver shellfish.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: This member-authored PR is not subject to the external-contributor proof gate; its existing test claims remain supplemental and should be rerun against the rebuilt current-main path.

Evidence

What I checked:

  • Maintainer-approved direction: The July 23 maintainer decision explicitly adopts the proposed policy: ordinary behind-base state is not a contributor blocker, while a seven-day base-freshness note remains non-blocking.
  • Active cache implementation: Current main computes the review content digest in the extracted source-revision module, which has no pull-base-drift input; the branch instead adds that input only to the retired monolithic entrypoint. (src/clawsweeper-source-revision.ts:250, 8fd1140e7aa9)
  • Active runtime ownership: Current main composes the extracted source-revision and context-hydration subsystems into the review runtime; the branch does not modify those active modules. (src/clawsweeper-runtime.ts:749, de31c9959070)
  • Branch code is in the superseded entrypoint: The branch adds comparison and freshness state in src/clawsweeper.ts, but current main has moved that responsibility into focused modules after the branch base. The branch currently has a dirty merge state. (src/clawsweeper.ts:5308, c0785de3001b)
  • Review continuity: The latest completed review already identified this same active-runtime gap at the identical branch SHA; no branch commit has addressed it. (src/clawsweeper.ts:5308, c0785de3001b)

Likely related people:

  • steipete: Peter Steinberger introduced the current source-revision and review-runtime refactors and relayed the maintainer decision adopting this policy direction. (role: current review-runtime refactor author and policy decision owner; confidence: high; commits: 8fd1140e7aa9, 922ebc673e12, de31c9959070; files: src/clawsweeper-source-revision.ts, src/clawsweeper-context-hydration.ts, src/clawsweeper-runtime.ts)

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 (22 earlier review cycles; latest 8 shown)
  • reviewed 2026-08-01T16:18:58.223Z sha c0785de :: needs maintainer review before merge. :: none
  • reviewed 2026-08-01T17:33:07.312Z sha c0785de :: found issues before merge. :: [P1] Rebuild this logic in the decomposed review runtime
  • reviewed 2026-08-01T21:23:27.645Z sha c0785de :: found issues before merge. :: [P1] Rebuild this logic in the decomposed review runtime
  • reviewed 2026-08-01T22:38:41.854Z sha c0785de :: needs changes before merge. :: [P1] Port base-drift behavior to the decomposed review runtime
  • reviewed 2026-08-02T08:54:27.666Z sha c0785de :: found issues before merge. :: [P1] Rebuild base-drift behavior in the decomposed review runtime
  • reviewed 2026-08-02T12:47:41.240Z sha c0785de :: found issues before merge. :: [P1] Port base-drift behavior to the decomposed review runtime
  • reviewed 2026-08-02T15:25:40.827Z sha c0785de :: needs changes before merge. :: [P1] Rebuild base-drift handling in active review modules
  • reviewed 2026-08-02T17:35:37.312Z sha c0785de :: found issues before merge. :: [P1] Port base-drift handling to the active review runtime

@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.

@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Jul 29, 2026
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Jul 30, 2026
@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. 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. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 30, 2026
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. P1 Urgent regression or broken agent/channel workflow affecting real users now. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. labels Aug 1, 2026
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. P1 Urgent regression or broken agent/channel workflow affecting real users now. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants