Skip to content

feat: opt-in cadence to skip re-reviewing unchanged pull requests#370

Open
saariuslystoned wants to merge 2 commits into
openclaw:mainfrom
saari-co:feat/skip-unchanged-pr-rereview
Open

feat: opt-in cadence to skip re-reviewing unchanged pull requests#370
saariuslystoned wants to merge 2 commits into
openclaw:mainfrom
saari-co:feat/skip-unchanged-pr-rereview

Conversation

@saariuslystoned

Copy link
Copy Markdown

We run ClawSweeper's review brain on a metered subscription and noticed open pull requests get re-reviewed about once a day even when nothing changed since the last review, reproducing an identical verdict and re-spending review budget each time.

reviewCadenceMs already drops to hourly when there is activity since the last review and uses the daily hot-window cadence for new pull requests, but a pull request past its hot window with no activity still falls to the daily branch. This adds an optional CLAWSWEEPER_STALE_PULL_REQUEST_REVIEW_DAYS that extends only that branch (for example 7 for weekly). Unset, non-numeric, or sub-daily values keep the current daily behavior, and the hourly-activity and daily hot-window branches are untouched, so changed and brand-new pull requests are still reviewed promptly. Behavior is unchanged unless an operator opts in.

  • src: stalePullRequestReviewCadenceMs() helper, read in reviewCadenceMs
  • test: scheduler-policy cadence cases (default daily, extended weekly, invalid/sub-daily fallback) through the exported shouldReviewItem
  • docs: scheduler.md cadence section

Release note (CHANGELOG.md left untouched -- it is release-owned per AGENTS.md): adds optional CLAWSWEEPER_STALE_PULL_REQUEST_REVIEW_DAYS to extend the re-review cadence for open PRs past their hot window with no activity since last review.

We run ClawSweeper's review brain on a metered subscription and noticed open
pull requests get re-reviewed about once a day even when nothing changed since
the last review, reproducing an identical verdict and re-spending review budget
each time.

reviewCadenceMs already drops to hourly when there is activity since the last
review and uses the daily hot-window cadence for new pull requests, but a pull
request past its hot window with no activity still falls to the daily branch.
This adds an optional CLAWSWEEPER_STALE_PULL_REQUEST_REVIEW_DAYS that extends
only that branch (for example 7 for weekly). Unset, non-numeric, or sub-daily
values keep the current daily behavior, and the hourly-activity and daily
hot-window branches are untouched, so changed and brand-new pull requests are
still reviewed promptly. Behavior is unchanged unless an operator opts in.

- src: stalePullRequestReviewCadenceMs() helper, read in reviewCadenceMs
- test: scheduler-policy cadence cases (default daily, extended weekly,
  invalid/sub-daily fallback) through the exported shouldReviewItem
- docs: scheduler.md cadence section

Release note (CHANGELOG.md left untouched -- it is release-owned per AGENTS.md):
adds optional CLAWSWEEPER_STALE_PULL_REQUEST_REVIEW_DAYS to extend the re-review
cadence for open PRs past their hot window with no activity since last review.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@clawsweeper

clawsweeper Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 25, 2026, 1:07 PM ET / 17:07 UTC.

Summary
Adds an optional CLAWSWEEPER_STALE_PULL_REQUEST_REVIEW_DAYS stale inactive PR review cadence with scheduler code, workflow env wiring, docs, and scheduler-policy tests.

Reproducibility: yes. Source inspection shows that with a weekly stale-PR cadence, shouldReviewItem can return false for a quiet PR reviewed yesterday, while reviewBackfillCandidate can still return that same not-due item once it is older than the six-hour backfill age.

Review metrics: 1 noteworthy metric.

  • Scheduler selection paths: 2 paths affected. Due-cadence selection and active-floor backfill can both schedule unchanged stale PR reviews, but the PR only proves the due path.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🦪 silver shellfish
Patch quality: 🦪 silver shellfish
Result: blocked until stronger real behavior proof is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Update the active-floor backfill behavior or explicitly document and test the due-only scope.
  • Post redacted terminal output, copied live output, logs, or a linked artifact showing the fixed planner/backfill behavior with the env set and with default fallback.

Proof guidance:

  • [P1] Needs stronger real behavior proof before merge: The comment provides copied live output for tests and the shouldReviewItem cadence flip, but it does not prove the normal planner active-floor behavior that still affects the changed scheduler feature. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P1] Operators setting CLAWSWEEPER_STALE_PULL_REQUEST_REVIEW_DAYS=7 may still pay for quiet stale PR re-reviews when normal active-floor backfill needs items after the six-hour backfill age.
  • [P1] The PR adds scheduler configuration surface, so maintainers need a clear decision on whether active-floor backfill must honor the new cadence or remain an explicit exception.
  • [P1] The posted proof is useful but partial: it shows the due helper behavior, not the production planner/backfill path that still creates the merge risk.

Maintainer options:

  1. Gate floor backfill by the configured cadence (recommended)
    Exclude inactive stale PRs from active-floor backfill until CLAWSWEEPER_STALE_PULL_REQUEST_REVIEW_DAYS has elapsed, and add scheduler-policy coverage for that path.
  2. Document the active-floor exception
    Maintainers could keep the six-hour active-floor behavior, but the docs and tests should say the setting only controls due cadence and does not suppress floor backfill.

Next step before merge

  • [P1] Contributor or maintainer follow-up is needed to settle and prove the active-floor behavior; ClawSweeper repair should not be queued while the external-contributor proof is still insufficient.

Security
Cleared: The diff adds scheduler env parsing, workflow env passing, docs, and tests without new dependencies, secret exposure, or third-party code execution.

Review findings

  • [P2] Apply the stale-PR cadence before floor backfill — src/clawsweeper.ts:5233
Review details

Best possible solution:

Make the configured stale-PR cadence apply consistently to due selection, active-floor current-review backfill, dashboard cadence math, and docs/tests, unless maintainers explicitly choose and document a narrower due-only setting.

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

Yes. Source inspection shows that with a weekly stale-PR cadence, shouldReviewItem can return false for a quiet PR reviewed yesterday, while reviewBackfillCandidate can still return that same not-due item once it is older than the six-hour backfill age.

Is this the best way to solve the issue?

No. The helper and workflow wiring are the right narrow shape, but the implementation should either apply the same cadence to active-floor backfill or explicitly narrow the documented behavior with tests.

Full review comments:

  • [P2] Apply the stale-PR cadence before floor backfill — src/clawsweeper.ts:5233
    This line only affects the due check. Normal planning still calls reviewBackfillCandidate for not-due items and appends them through active-floor backfill once the latest review is older than MIN_BACKFILL_REVIEW_AGE_MINUTES. With the env set to 7, a quiet stale PR reviewed yesterday is no longer due here, but it can still be selected as floor backfill after six hours, so scheduled runs can re-review it before the configured cadence. Gate the backfill path on the same cadence, or document and test the narrower behavior.
    Confidence: 0.89

Overall correctness: patch is incorrect
Overall confidence: 0.89

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This is a bounded scheduler automation improvement with a normal-priority correctness gap in an opt-in production path.
  • merge-risk: 🚨 automation: The PR changes scheduled review cadence behavior, and active-floor backfill can still re-review unchanged stale PRs earlier than the configured cadence.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🦪 silver shellfish and patch quality is 🦪 silver shellfish.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The comment provides copied live output for tests and the shouldReviewItem cadence flip, but it does not prove the normal planner active-floor behavior that still affects the changed scheduler feature. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

What I checked:

  • Repository policy applied: AGENTS.md was read fully; its scheduler tuning guidance applies because this PR changes both src/clawsweeper.ts and .github/workflows/sweep.yml. (AGENTS.md:86, 323f7bdf7236)
  • Current main does not already implement this: Current main still returns the daily cadence for pull requests outside the hot window, so the requested opt-in cadence is not already present on main. (src/clawsweeper.ts:5215, 323f7bdf7236)
  • Branch due-cadence implementation: The PR branch adds stalePullRequestReviewCadenceMs() and uses it only in the stale pull-request branch of reviewCadenceMs. (src/clawsweeper.ts:5233, a71d082e97c6)
  • Workflow env wiring is now present: The updated branch passes CLAWSWEEPER_STALE_PULL_REQUEST_REVIEW_DAYS into both the scheduled planner env and dashboard audit env. (.github/workflows/sweep.yml:1095, a71d082e97c6)
  • Active-floor backfill bypass remains: On the PR branch, reviewBackfillCandidate returns not-due current reviews after the minimum backfill age, and planCandidates appends those fallback candidates to fill the active floor. (src/clawsweeper.ts:5333, a71d082e97c6)
  • Current docs confirm the active-floor behavior: The scheduler docs say normal planning fills up to 9 nonempty shards with eligible current-review items whose latest complete review is at least 6 hours old. (docs/scheduler.md:281, 323f7bdf7236)

Likely related people:

  • brokemac79: Blame shows the current review cadence, due candidate logic, active-floor backfill planning, and workflow planner env allowlist all date to the scheduler requeue work in bb22ac97a417. (role: introduced scheduler behavior; confidence: high; commits: bb22ac97a417; files: src/clawsweeper.ts, .github/workflows/sweep.yml)
  • Dallin Romney: Blame shows the active-floor scheduler-policy test was added during the recent scheduler/review-comment test split. (role: recent test area contributor; confidence: medium; commits: 929e605da559; files: test/scheduler-policy.test.ts)
  • Vincent Koc: Recent workflow history includes review fanout, worker burst, and API-pressure changes in the same scheduled sweep area. (role: recent workflow contributor; confidence: medium; commits: f69d9debf487, c66b2dedc6ec, 1566ad6a0a5b; files: .github/workflows/sweep.yml, docs/scheduler.md)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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.

How this review workflow works
  • 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.

@clawsweeper clawsweeper Bot added 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. merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. labels Jun 25, 2026
Addresses ClawSweeper's review of openclaw#370. The new
CLAWSWEEPER_STALE_PULL_REQUEST_REVIEW_DAYS was read in reviewCadenceMs, but the
scheduled planner in .github/workflows/sweep.yml passes an explicit env
allowlist to `pnpm run plan`, so setting the documented repository variable
would not reach production scheduled runs -- stale PRs would keep the daily
cadence after opt-in. Per AGENTS.md, scheduler tuning must update both
src/clawsweeper.ts and .github/workflows/sweep.yml.

- sweep.yml: pass vars.CLAWSWEEPER_STALE_PULL_REQUEST_REVIEW_DAYS into the
  planner `select` step (the only cadence-selection path -- review shards run
  exact --item-numbers from the plan matrix) and into the dashboard
  `audit --update-dashboard` step.
- clawsweeper.ts: cadenceBucketForReview reuses stalePullRequestReviewCadenceMs
  so the dashboard "current within cadence" math matches what the planner
  schedules.
- docs/scheduler.md: document the repository-variable wiring.

Unset/non-numeric/<= daily keeps the daily default everywhere, so behavior is
unchanged unless an operator opts in.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@saariuslystoned saariuslystoned requested a review from a team as a code owner June 25, 2026 16:48
@saariuslystoned

Copy link
Copy Markdown
Author

Thanks for the review! Addressed the workflow-wiring finding: CLAWSWEEPER_STALE_PULL_REQUEST_REVIEW_DAYS is now wired into .github/workflows/sweep.yml (the scheduled planner select step + the
dashboard audit step), and cadenceBucketForReview reuses the same helper so the dashboard matches the planner. Docs updated; per AGENTS.md both src/clawsweeper.ts and sweep.yml are updated. Default
behavior unchanged unless the repo variable is set.

Real behavior proof (built dist/, Node 24):

node --test test/scheduler-policy.test.ts → 13 tests, pass 13, fail 0.

Cadence flip via shouldReviewItem from dist/, stale PR with no activity:
unset (default daily) reviewed ~26h ago -> due: true # unchanged
=7 (weekly) reviewed ~26h ago -> due: false # skipped, not re-reviewed
=7 (weekly) reviewed ~8d ago -> due: true # due after the window
pnpm run check passes locally.

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

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: 🧂 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant