Skip to content

fix(review): suppress owned activity churn - #1007

Open
PollyBot13 wants to merge 1 commit into
openclaw:mainfrom
PollyBot13:fix/review-storm-self-activity
Open

fix(review): suppress owned activity churn#1007
PollyBot13 wants to merge 1 commit into
openclaw:mainfrom
PollyBot13:fix/review-storm-self-activity

Conversation

@PollyBot13

@PollyBot13 PollyBot13 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

Scheduled hot intake can repeatedly run a full review on an unchanged issue or PR after ClawSweeper updates its own durable review comment or labels. GitHub may publish the item updated_at one second after the local sync timestamp, so the existing one-hour owned-activity guard eventually classifies that bot-owned timestamp as fresh target activity.

Fixes #1006.

Why This Change Was Made

Production run https://github.com/openclaw/clawsweeper/actions/runs/30704608131 reviewed openclaw/clickclack#117 at unchanged head d26c11abba773132c51defb6e647db3a69610003. The full review completed at 14:52:41Z, the durable review comment synced at 14:53:28Z, and GitHub advanced the PR updated_at to 14:53:29Z. Scheduled hot intake then saw that later server timestamp as uncaptured activity and ran the full review again with zero cache hits.

The apply lane now reads back and stores the authoritative GitHub item timestamp after each owned label or durable-comment mutation. Scheduler cadence, exact hot-intake planning, and the structural cache accept only an exact match to that stored timestamp as owned activity. Missing metadata and any later timestamp continue to fail open into review.

The new operational timestamp is excluded from PR-close proof prompt binding so persisting it cannot invalidate an otherwise exact precomputed proof. Semantic report changes remain bound.

User Impact

  • Stops ClawSweeper from amplifying its own comment and label writes into repeated hourly full reviews.
  • Preserves daily or weekly coverage cadence.
  • Preserves immediate review eligibility for any later target-side activity.
  • Existing records without the new field retain the prior conservative behavior.

Evidence

  • Old-code regression proof: the new scheduler test failed against the previous implementation because the unchanged item was considered due.
  • Focused validation: 88 scheduler, structural-cache, and apply tests passed.
  • Proof-boundary validation: 39 PR-close proof and apply tests passed, including semantic-change invalidation.
  • Full repository gate: pnpm run check passed, including static checks, builds, lint, 217 test files, and both coverage thresholds.
  • git diff --check passed.
  • Fresh Codex review before commit: no actionable findings.
  • Fresh Codex committed-range review against origin/main: no discrete regression found.

AI-assisted

Implemented and reviewed with Codex. The production timestamp chain and GitHub state were inspected directly; tests and repository gates provide the after-fix proof.

Exact-head live GitHub proof

A disposable PR in the PollyBot13 fork exercised the exact upstream head ca43c93fe837c0b008c8a4054512b40961cda77a against base dccfa528f0387431807818ba43823310b5e08f51: PollyBot13#1 (comment).

  1. The PR was created at 18:00:00Z.
  2. An automation-owned label mutation advanced GitHub updatedAt to 18:00:16Z.
  3. The branch scheduler evaluated the live item beyond the one-hour hot-intake cadence with automation_item_updated_at: 18:00:16Z and returned scheduledReviewDue: false.
  4. A later target-side body edit advanced GitHub updatedAt to 18:00:44Z; using the same persisted automation timestamp, the branch scheduler returned scheduledReviewDue: true.

The proof PR is closed and disposable; its exact-head transcript and GitHub timeline remain inspectable. No production records or workflows were mutated.

@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. 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 Aug 1, 2026
@clawsweeper

clawsweeper Bot commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Codex review: found issues before merge. Reviewed August 2, 2026, 4:55 AM ET / 08:55 UTC.

ClawSweeper review

What this changes

Records the GitHub item timestamp observed after ClawSweeper changes its own labels or durable review comment, then treats that exact timestamp as automation-owned in scheduler, hot-intake, and structural-cache decisions.

Merge readiness

⚠️ Needs maintainer review before merge - 5 items remain

Keep open: the PR has credible live proof for the reported review storm, but an equal updated_at value is not by itself an operation-specific ownership receipt, so a target-side update reported with that same value can be deferred. Likely related people: Peter Steinberger (high confidence) and Martin Cleary (high confidence).

Priority: P2
Reviewed head: ca43c93fe837c0b008c8a4054512b40961cda77a
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The PR has strong targeted proof and focused tests, but the timestamp-only ownership inference leaves a high-priority correctness boundary unresolved.
Proof confidence 🦞 diamond lobster (5/6) ✨ media proof bonus Sufficient (linked_artifact): The linked disposable exact-head GitHub run demonstrates the post-mutation suppression and a later distinct target-side update becoming due; it should be extended if the timestamp-collision repair proceeds, with private details redacted.
Patch quality 🦐 gold shrimp (3/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Verified Sufficient (linked_artifact): The linked disposable exact-head GitHub run demonstrates the post-mutation suppression and a later distinct target-side update becoming due; it should be extended if the timestamp-collision repair proceeds, with private details redacted.
Evidence reviewed 6 items Current main still has the reported ambiguity: Current main compares item activity against the stored review timestamp and local synchronization timestamps; it has no post-mutation authoritative item receipt, so the central behavior is not already implemented.
Proposed equality suppression is the unsafe boundary: The branch returns no new activity whenever the item timestamp equals the persisted automation timestamp, without separately proving that the matching activity came from the recorded mutation.
The same receipt is reused across review-admission paths: The PR adds the same exact-equality condition to scheduled hot intake and the structural cache, so an ambiguous equality can suppress more than the hourly scheduler path.
Findings 1 actionable finding [P1] Do not treat an equal timestamp as an ownership receipt
Security None None.

How this fits together

ClawSweeper publishes labels and durable review comments to GitHub, then uses the resulting item activity timestamp to decide whether scheduled review work is new or already covered. The scheduler and structural cache feed that decision into review admission, so an ownership misclassification either causes redundant reviews or defers genuine follow-up activity.

flowchart LR
  A[GitHub item activity] --> B[Apply lane writes labels or review comment]
  B --> C[Post-mutation item timestamp]
  C --> D[Durable review record]
  D --> E[Scheduler activity decision]
  D --> F[Hot-intake snapshot guard]
  D --> G[Structural cache]
  E --> H[Review admission]
  F --> H
  G --> H
Loading

Decision needed

Question Recommendation
Should ClawSweeper accept an exact GitHub updated_at string as sufficient proof that later matching activity is automation-owned, despite an independently generated target update potentially sharing that value? Require an unambiguous receipt before merge: Preserve fail-open behavior for timestamp equality unless the stored receipt identifies the specific automation mutation or a monotonic activity boundary.

Why: The branch corrects a demonstrated automation storm, but choosing whether same-value activity may be deferred is a cross-lane review-policy tradeoff rather than a mechanical implementation detail.

Before merge

  • Do not treat an equal timestamp as an ownership receipt (P1) - updated_at is persisted only as a timestamp string. If a target-side update is reported with that same value after the automation mutation, this return suppresses it as owned activity; the same condition also feeds hot intake and the structural cache. Preserve fail-open behavior unless the receipt can distinguish the specific mutation. This was visible on the unchanged head reviewed in the prior cycle.
  • Resolve merge risk (P1) - Merging the exact timestamp equality rule can treat a target-side update reported with the same persisted updated_at value as bot-owned, deferring its hot or scheduled review until a later cadence rather than preserving the current fail-open behavior.
  • Resolve merge risk (P1) - The receipt is consumed by the scheduler, hot-intake guard, and structural cache, so an attribution mistake affects several review-admission paths at once.
  • Complete next step (P2) - A maintainer must choose the acceptable ownership-proof boundary before this cross-lane automation change can be repaired or merged.

Findings

  • [P1] Do not treat an equal timestamp as an ownership receipt — src/scheduler-policy.ts:92
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Changed surface 12 files affected: 8 implementation files and 4 focused tests The patch propagates one ownership signal through apply, record metadata, planning, cache, scheduler, and proof-binding boundaries.
Admission paths 3 review-admission decisions changed Scheduler, hot-intake, and structural-cache equality checks all depend on the new timestamp receipt.

Root-cause cluster

Relationship: fixed_by_candidate
Canonical: #1006
Summary: This PR is the active candidate fix for the open canonical report about owned comment timestamps causing repeated scheduled reviews.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge-risk options

Maintainer options:

  1. Use a non-colliding ownership receipt (recommended)
    Add a regression for a target update that shares the persisted timestamp and suppress activity only when the stored evidence can distinguish the automation mutation.
  2. Accept the deferred-review edge case
    Land the timestamp-only receipt only if maintainers explicitly accept that same-value activity may not receive immediate hot-intake review.
  3. Pause the PR
    Close or defer this implementation if an unambiguous GitHub receipt is unavailable and the current fail-open policy is preferred.

Technical review

Best possible solution:

Use a durable receipt that can distinguish the specific automation mutation from later item activity, or retain the existing fail-open rule whenever an exact timestamp alone cannot establish that distinction.

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

Yes. The supplied production trace identifies an unchanged item being re-reviewed after a ClawSweeper mutation, and current-main source still uses the timestamp comparison that permits it; the branch’s own live proof reproduces the intended post-mutation behavior on its exact head.

Is this the best way to solve the issue?

No. Persisting only an exact timestamp fixes the demonstrated later-second case, but it cannot safely distinguish a later target update that GitHub reports with the same value; the safer design needs a discriminating receipt or must fail open.

Full review comments:

  • [P1] Do not treat an equal timestamp as an ownership receipt — src/scheduler-policy.ts:92
    updated_at is persisted only as a timestamp string. If a target-side update is reported with that same value after the automation mutation, this return suppresses it as owned activity; the same condition also feeds hot intake and the structural cache. Preserve fail-open behavior unless the receipt can distinguish the specific mutation. This was visible on the unchanged head reviewed in the prior cycle.
    Confidence: 0.97
    Late finding: first raised on code an earlier review cycle already covered.

Overall correctness: patch is incorrect
Overall confidence: 0.95

AGENTS.md: found and applied where relevant.

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

Labels

Label changes:

  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦞 diamond lobster and patch quality is 🦐 gold shrimp.
  • add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (linked_artifact): The linked disposable exact-head GitHub run demonstrates the post-mutation suppression and a later distinct target-side update becoming due; it should be extended if the timestamp-collision repair proceeds, with private details redacted.
  • remove status: 👀 ready for maintainer look: Current PR status label is status: ⏳ waiting on author.
  • remove rating: 🦞 diamond lobster: Current PR rating is rating: 🦐 gold shrimp, so this older rating label is no longer current.

Label justifications:

  • P2: This is a bounded automation correctness fix that reduces repeated reviews, but the active regression is not a current user-facing outage.
  • merge-risk: 🚨 automation: The patch changes how scheduler and cache paths suppress or admit automated reviews after GitHub mutations.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦞 diamond lobster and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (linked_artifact): The linked disposable exact-head GitHub run demonstrates the post-mutation suppression and a later distinct target-side update becoming due; it should be extended if the timestamp-collision repair proceeds, with private details redacted.
  • proof: sufficient: Contributor real behavior proof is sufficient. The linked disposable exact-head GitHub run demonstrates the post-mutation suppression and a later distinct target-side update becoming due; it should be extended if the timestamp-collision repair proceeds, with private details redacted.

Evidence

What I checked:

  • Current main still has the reported ambiguity: Current main compares item activity against the stored review timestamp and local synchronization timestamps; it has no post-mutation authoritative item receipt, so the central behavior is not already implemented. (src/scheduler-policy.ts:84, 7e4d8cb7cee4)
  • Proposed equality suppression is the unsafe boundary: The branch returns no new activity whenever the item timestamp equals the persisted automation timestamp, without separately proving that the matching activity came from the recorded mutation. (src/scheduler-policy.ts:92, ca43c93fe837)
  • The same receipt is reused across review-admission paths: The PR adds the same exact-equality condition to scheduled hot intake and the structural cache, so an ambiguous equality can suppress more than the hourly scheduler path. (src/clawsweeper-review-planning.ts:255, ca43c93fe837)
  • Live proof establishes the main storm scenario but not the collision boundary: The PR body and linked disposable exact-head proof show that a later activity timestamp becomes due again, but the supplied scenario uses different second-level timestamps and does not demonstrate a target-side update reported with the exact persisted receipt value. (test/scheduler-policy.test.ts:225, ca43c93fe837)
  • Prior policy intentionally preserved ambiguous post-sync activity: Recent scheduler history includes the merged ambiguity-preserving regression work from Martin Cleary, which is relevant context for requiring a receipt that distinguishes ownership rather than only matching a timestamp. (src/scheduler-policy.ts:84, c7b083110f14)
  • The current base has not independently absorbed this patch: The changed production paths have no intervening changes from the PR base to current main, while the PR commit is not an ancestor of current main; this remains an active candidate rather than an implemented-on-main cleanup. (src/clawsweeper-apply-decision-workflow.ts:1928, 7e4d8cb7cee4)

Likely related people:

  • Peter Steinberger: Current exact hot-intake activity handling in the review-planning subsystem dates to Peter's recent decomposition work, and he also carried the related scheduled-throughput changes. (role: recent area contributor; confidence: high; commits: 922ebc673, 6b3a2cef36f5; files: src/clawsweeper-review-planning.ts, src/scheduler-policy.ts)
  • Martin Cleary: Martin authored the recent merged scheduler work that both preserved ambiguous post-sync activity and added exact hot-intake duplicate suppression. (role: scheduler behavior contributor; confidence: high; commits: c7b083110f14, 0ced0b5e515b; files: src/scheduler-policy.ts, test/scheduler-policy.test.ts)

Rank-up moves

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

  • Add a failing regression where target-side activity is reported with the persisted timestamp value.
  • Revise the ownership receipt or retain fail-open behavior, then provide updated live proof covering both suppression and the ambiguous boundary.

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 (4 earlier review cycles)
  • reviewed 2026-08-01T17:55:09.771Z sha ca43c93 :: needs real behavior proof before merge. :: none
  • reviewed 2026-08-01T18:06:55.447Z sha ca43c93 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-01T21:23:32.360Z sha ca43c93 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-01T22:37:44.706Z sha ca43c93 :: needs maintainer review before merge. :: none

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. 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. and removed 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 Aug 1, 2026
@PollyBot13

Copy link
Copy Markdown
Contributor Author

Historical RCA: why the earlier fixes did not stop the review storm

Short version: no single recent PR introduced the whole failure. A latent timestamp-provenance ambiguity dates to the April scheduler work; later throughput changes amplified it from an occasional redundant review into a visible storm.

Timeline

  1. 21f9571 (25 April, ci: schedule reviews by activity cadence) made GitHub item updated_at part of review scheduling.
  2. 3ac50a8 (30 April, fix: harden clawsweeper scheduler fairness) added bot-owned synchronization timestamps. The safe boundary was effectively “at or before our locally recorded sync”; a later GitHub timestamp remained reviewable.
  3. #903 deliberately preserved that fail-open rule for activity 1–5 seconds after comment or label synchronization. That was the correct conservative choice without durable provenance: a nearby human update could not safely be distinguished from GitHub publishing ClawSweeper's own mutation late. The PR explicitly identified an exact-operation receipt as the proper future solution.
  4. #940, #955, #958, and #959 increased scheduled feed capacity, per-cycle candidate volume, and hot/normal cadence. They did not create the false-positive condition, but they gave it enough cadence and capacity to repeatedly select the same unchanged items.

Why #970 and #992 were insufficient

  • #970 fixed a separate duplicate path: equivalent CI check runs no longer create different cache identities for an unchanged head. That prevents duplicate CI data -> false cache miss, but it does not address ClawSweeper's own GitHub write -> false scheduler wake.
  • #992 correctly suppressed a fresh completed hot-intake review only when the persisted exact snapshot still matched the live PR. It intentionally failed open when activity timestamps differed. In the production failure, the local comment sync was recorded at 14:53:28Z, then GitHub authoritatively advanced the PR to 14:53:29Z; the snapshots therefore never matched, so the guard could not prove that the update was bot-owned.

Why this PR closes the gap

This PR supplies the missing provenance rather than widening a time window. After an owned label or durable-comment mutation, ClawSweeper reads back and persists GitHub's authoritative updated_at. Scheduler and cache logic suppress only an exact match to that receipt. Missing metadata or any genuinely later timestamp still fails open into review.

So the attribution is:

This also explains why the system appeared healthy before the July scaling work: the false-positive edge existed, but the older scheduler did not have the cadence and throughput to hammer it repeatedly.

@clawsweeper clawsweeper Bot added rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. 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. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. 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. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Scheduled hot intake re-reviews unchanged PRs after owned comment timestamps outrun sync metadata

1 participant