Skip to content

Review each selected PR in its own item shard#674

Closed
hxy91819 wants to merge 19 commits into
mainfrom
feature/per-pr-item-shard-review
Closed

Review each selected PR in its own item shard#674
hxy91819 wants to merge 19 commits into
mainfrom
feature/per-pr-item-shard-review

Conversation

@hxy91819

Copy link
Copy Markdown
Member

Summary

  • replace multi-item sequential review shards with one matrix job per selected item
  • use per-item compute concurrency and generation-bound queue claims so newer exact reviews cancel only stale compute for the same item
  • serialize durable publication, apply, and comment sync through a non-cancelling per-item mutation lane
  • add protocol v3 rolling compatibility, terminal reconciliation, cancellation cleanup, and item-level cost/concurrency telemetry

This draft supersedes #645. It stays draft until the isolated canary and the two-item live validation complete.

Why

The previous shard model reviewed several PRs sequentially in one long-running job. A new generation for one PR could not cancel only that PR's stale Codex work without also affecting unrelated items. The new execution unit is one item job, while the durable queue remains the cross-workflow admission and generation authority.

Safety and compatibility

  • batch_size is the total planner selection budget; shard_count maps to matrix max-parallel
  • queue protocol v1/v2 claims remain non-preemptible during rollout and requeue onto the current v3 generation
  • publication checks generation before GitHub mutation and acquires a generation-bound mutation permit
  • mutation leases are never reclaimed by TTL; terminal queue reconciliation or a verified terminal GitHub Actions attempt releases abandoned ownership
  • no live apply or close command is part of this validation

Validation

  • Node v24.15.0
  • pnpm run check
  • focused item-shard, queue, workflow, and bundle tests (291/291)
  • /data/code/openclaw/openclaw/.agents/skills/autoreview/scripts/autoreview --mode local --stream-engine-output
    • final result: clean, no accepted/actionable findings

Draft exit criteria

  • deploy an isolated canary Worker and Durable Object namespace with no production binding or data migration
  • run a two-item batch using this PR and Test durable review status transitions #646
  • verify same-item generation cancellation, unaffected sibling work, durable handoff/comment identity, stale publication rejection, refreshing-to-interrupted behavior, terminal reconciliation, and telemetry evidence

@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. 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 18, 2026
@clawsweeper

clawsweeper Bot commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 19, 2026, 5:48 AM ET / 09:48 UTC.

Summary
The branch replaces sequential multi-item review shards with one cancellable job per selected item, adding protocol-v3 generation fencing, per-item mutation permits, durable status projections, and review telemetry.

Reproducibility: not applicable. This PR proposes review-automation architecture rather than addressing a reproducible user bug.

Review metrics: 2 noteworthy metrics.

  • Automation surface: 23 files; 3,348 added, 228 removed. The change spans workflow scheduling, Durable Object queue state, CLI orchestration, comment routing, telemetry, documentation, and focused tests.
  • Protocol migration: v1/v2 compatibility plus v3 generation fields. Live and queued records may span protocol versions during the proposed rollout.

Root-cause cluster
Relationship: canonical
Canonical: #674
Summary: This PR is the active implementation for per-item shard execution; related PRs are a superseded proposal, validation context, or merged prerequisites.

Members:

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

Merge readiness
Overall: 🦐 gold shrimp
Proof: 🦐 gold shrimp
Patch quality: 🐚 platinum hermit
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:

  • Provide redacted two-item canary evidence covering same-item cancellation, sibling isolation, and claimed mutation/comment synchronization.
  • Confirm the canary runs against the exact current PR head after merged observability prerequisites.

Proof guidance:

  • [P1] Needs stronger real behavior proof before merge: The contributor supplied partial canary logs, but the PR explicitly says a claimed production event is still needed to exercise final mutation/comment sync and the planned two-item validation remains incomplete; add redacted runtime evidence, then update the PR body or ask a maintainer to comment @clawsweeper re-review.

Risk before merge

  • [P1] This 23-file rollout changes queue protocol, workflow concurrency, publication ordering, and durable comment synchronization; maintainers need exact-head proof that cancellation remains item-scoped while claimed mutation publication stays serialized.
  • [P1] The supplied canary intentionally did not exercise the final claimed mutation/comment-sync path, and the stated two-item validation remains incomplete.

Maintainer options:

  1. Run the exact-head rollout validation (recommended)
    Keep the draft open until a verified exact-head canary proves same-item cancellation, unaffected sibling work, and claimed mutation/comment publication.
  2. Split the protocol migration
    Land smaller independently validated pieces if maintainers do not want the queue, workflow, and publication changes to roll out together.
  3. Pause the combined rollout
    Defer the branch if the repository does not want to accept this combined review-automation protocol migration now.

Next step before merge

  • [P2] This member-authored draft needs a maintainer rollout decision and proof review; no narrow mechanical repair is established.

Maintainer decision needed

  • Question: After reviewing exact-head canary evidence, is the protocol-v3 per-item execution model ready to replace the existing sequential shard model?
  • Rationale: This is a broad workflow and durable-queue behavior change whose operational compatibility and rollout acceptance require repository-owner judgment beyond a mechanical repair.
  • Likely owner: hxy91819 — The supplied history identifies this member as the active contributor for the affected review queue and workflow surfaces; repository leadership should make the final acceptance choice.
  • Options:
    • Require the planned rollout proof (recommended): Keep the PR draft until the exact head has passed the stated isolated and two-item validation, including a claimed mutation publication path.
    • Split the migration: Separate matrix execution from protocol-v3 mutation and generation fencing if maintainers want smaller rollout and rollback boundaries.
    • Accept the operational risk: Approve based on existing checks and partial canary evidence despite the unexercised claimed-mutation boundary.

Security
Cleared: The supplied diff context shows no new dependency source, secret exposure, or permission broadening; this remains an operational automation-risk review rather than a concrete security finding.

Review details

Best possible solution:

Validate the exact head in an isolated and then two-item proposal-only run, demonstrating same-item cancellation, sibling isolation, generation fencing, final mutation/comment synchronization, terminal reconciliation, and telemetry before the PR leaves draft status.

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

Not applicable: this PR proposes review-automation architecture rather than addressing a reproducible user bug.

Is this the best way to solve the issue?

Unclear: per-item execution directly addresses the stated cancellation-isolation problem, but exact-head integration proof is still required to establish that this is the safest landing path.

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add merge-risk: 🚨 compatibility: The branch introduces protocol-v3 generation and mutation-permit behavior while retaining v1/v2 records during rollout.
  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The contributor supplied partial canary logs, but the PR explicitly says a claimed production event is still needed to exercise final mutation/comment sync and the planned two-item validation remains incomplete; add redacted runtime evidence, then update the PR body or ask a maintainer to comment @clawsweeper re-review.
  • remove rating: 🐚 platinum hermit: Current PR rating is rating: 🦐 gold shrimp, so this older rating label is no longer current.
  • remove status: 👀 ready for maintainer look: Current PR status label is status: 📣 needs proof.

Label justifications:

  • P2: This is a material but non-emergency automation improvement with bounded repository-wide operational impact.
  • merge-risk: 🚨 automation: The branch changes the review matrix, queue admission, cancellation, publication, telemetry, and action-ledger workflow behavior.
  • merge-risk: 🚨 compatibility: The branch introduces protocol-v3 generation and mutation-permit behavior while retaining v1/v2 records during rollout.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The contributor supplied partial canary logs, but the PR explicitly says a claimed production event is still needed to exercise final mutation/comment sync and the planned two-item validation remains incomplete; add redacted runtime evidence, then update the PR body or ask a maintainer to comment @clawsweeper re-review.
Evidence reviewed

What I checked:

  • Draft rollout boundary: The PR body explicitly keeps the branch draft until an isolated canary and a two-item validation exercise same-item cancellation, unaffected sibling work, durable handoff, stale-publication rejection, terminal reconciliation, and telemetry. (6a039cfd0720)
  • Merged prerequisite context: The supplied related-item context identifies review observability prerequisites merged in Add PR review observability foundation #684 and Add PR 674 review reliability observability #693; this PR is the remaining generation-aware producer and execution change. (bc29bc8bed98)
  • Repository policy: The target AGENTS.md requires conservative automation review and directs changes to review throughput/default behavior to update both the primary CLI and sweep workflow, which this branch does. (AGENTS.md:29, 1b9630b4911b)
  • Current review continuity: The prior completed ClawSweeper review found no discrete patch defect but required maintainer evaluation of the canary evidence and this combined protocol/workflow rollout; the latest head was subsequently force-pushed on July 19, 2026. (6a039cfd0720)

Likely related people:

  • hxy91819: The supplied branch history attributes the queue, workflow, telemetry, status-projection, and cancellation work to this contributor; the author association is MEMBER, so automated cleanup closure is not appropriate. (role: recent area contributor; confidence: high; commits: 87c25e2d1080, f439f55445cc, 6a039cfd0720; files: .github/workflows/sweep.yml, dashboard/exact-review-queue.ts, src/clawsweeper.ts)
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.
Review history (2 earlier review cycles)
  • reviewed 2026-07-18T17:15:57.964Z sha 7eecc0d :: needs maintainer review before merge. :: none
  • reviewed 2026-07-18T17:47:10.390Z sha f57a43b :: needs maintainer review before merge. :: none

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Jul 18, 2026
@hxy91819

Copy link
Copy Markdown
Member Author

Canary follow-up for the durable stale-review status included in this PR:

No live apply/close command was run. This PR remains draft.

@hxy91819
hxy91819 force-pushed the feature/per-pr-item-shard-review branch from f57a43b to 6a039cf Compare July 19, 2026 09:45
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. and removed 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. labels Jul 19, 2026
@hxy91819

Copy link
Copy Markdown
Member Author

Landing update for #674 at head 6a039cfd0720c2c061e05d8db74c933bf41bc616 (rebased onto 1b9630b4911b2f6179c43a72f389d5e53e120dc4):

Validation completed:

The branch now also includes PR 693 telemetry-v2 producer integration, immutable terminal handling, retryable-publication heartbeats, mutation fencing/reconciliation, canonical repository identity, and durable publication ordering without duplicating the observer/dashboard implementation.

Landing is intentionally blocked and the PR remains draft. Final autoreview found one unresolved generation-ordering flaw in GitHub native concurrency: cancellation happens before the arriving job validates its durable claim, so a delayed duplicate or older same-item repository_dispatch can cancel the current newer generation and then lose its own stale claim. Fixing this safely requires moving cancellation authority into the durable generation protocol (for example, an authoritative claim response that identifies the exact superseded run to cancel), rather than another concurrency-key tweak. That is a new protocol/design dependency, so it is outside the safe local-fix boundary for this landing attempt.

The requested pre-merge production-like canary cannot safely bypass this blocker. Ordinary branch workflow_dispatch still has no production queue claim (as already shown by https://github.com/openclaw/clawsweeper/actions/runs/29654503026), while queue-originated repository_dispatch loads the workflow from default-branch main. I did not change the default branch, deploy a branch Worker into production, create synthetic live failures, run live apply/close, or pause the sweep.

Current read-only observability remains passive at https://clawsweeper.openclaw.ai/api/review-observability?range=24h&repo=all: expected attempts 614, terminal attempts 0, coverage 0%, unexpected cancelled 0, unresolved failures 0, refreshing/slow/orphan 0. Exact event, Hot intake, and Normal backfill all currently show attributed successful source freshness, but these are prerequisite/passive observations, not PR 674 canary proof.

Required next design decision: add a queue-authoritative, generation-aware cancellation handoff that cannot let stale dispatches cancel newer work; then rerun autoreview and the requested two-generation/two-item canary using #674 and #646 before removing draft status.

@brokemac79

Copy link
Copy Markdown
Contributor

@hxy91819 - when you land this PR, can you take a look at #706 as it is sequenced to land after. I have kept it draft for now. Feel free to review that PR and if you are happy with it, land it, after you land this one. Thanks - Martin

@hxy91819

Copy link
Copy Markdown
Member Author

Maintainer direction update (2026-07-21): stop the current combined rollout and do not land this head.

The experiment exposed a flaw in the central same-item cancellation assumption. The exact-event job currently gives every generation of an item the same GitHub concurrency key with cancel-in-progress: true. GitHub performs that cancellation before the arriving job executes its durable queue claim. A delayed older dispatch can therefore cancel the valid newer generation, then have its own claim correctly rejected as stale. The mutation fence still protects against stale publication, but the current generation can be stranded or delayed until reconciliation/lease expiry.

Neither simple concurrency-key alternative preserves the intended behavior:

  • Adding generation to the key prevents stale-to-new cancellation, but also prevents a new generation from cancelling old compute.
  • Disabling cancel-in-progress removes the unsafe race, but means this branch no longer implements or proves eager same-item generation cancellation.

A safe eager-cancellation design would require cancellation authority to move behind the durable claim: the queue would need to identify the exact older claimed run, persist and retry that cancellation instruction, carry it across multiple superseding generations, and reconcile one immutable superseded/generation_superseded terminal outcome. It would also need a protected claimed-canary path because branch workflow_dispatch has no production claim while queue-originated repository_dispatch loads the workflow from default-branch main. That is a new control-plane protocol, not a narrow correction to per-item matrix sharding, and we should not add it merely to preserve this diff.

Scope is another reason to stop rather than patch forward. The original goal is “Review each selected PR in its own item shard,” but the current branch spans 23 files, 19 commits, 3,348 additions, and 228 deletions across matrix planning, queue protocol v3, mutation permits, publication ordering, status projection, and telemetry. The combined rollout is no longer an appropriate landing unit for that requirement.

Decision:

  • Keep Review each selected PR in its own item shard #674 draft and unmerged; do not expand it with another cancellation protocol.
  • Preserve this PR and its existing CI/canary comments as experiment evidence.
  • Do not enable required observability enforcement from this branch.
  • Do not pause the live sweep or run live apply/close as part of this decision.
  • Do not assume draft: coalesce matching exact-review ingress #706 can remain sequenced behind this rollout; it needs an independent compatibility and landing decision. This update does not review or modify that PR.

Potential follow-up directions, in preferred order:

  1. Minimal per-item matrix without eager cancellation (recommended). Start from current main; emit one item per matrix entry; use batch_size for selection and shard_count for max-parallel; preserve current-main exact-event queue, publication, freshness, and mutation behavior; do not change queue schema/API/dashboard/telemetry.
  2. Prevent duplicate work before dispatch. Coalesce same-item pending work in the existing durable planning/intake owner if measurements show duplicate selection is material.
  3. Cooperative supersession. Let a claimed worker check durable generation at safe checkpoints and exit as superseded. Treat supervised interruption of an opaque Codex process as a separate worker-lifecycle feature.
  4. Queue-authoritative targeted Actions cancellation. Consider only if production measurements prove that completing superseded compute is too costly; define it as a separate protocol and PR with its own threat model, tests, rollout, and claimed canary.
  5. Per-item budgets without cross-run cancellation. Independent timeouts/retries/artifacts may deliver most of the desired failure and latency isolation with much less protocol risk.

A replacement narrow PR should prove only the original invariant: one item per job, sibling isolation, correct batch_size/max-parallel, unchanged durable publication safety, no item-scoped native cancellation, Node 24 checks, autoreview, and a proposal-only two-owned-item canary. It should not require a same-item two-generation cancellation canary.

Useful retained evidence:

@hxy91819 hxy91819 closed this Jul 21, 2026
@hxy91819

Copy link
Copy Markdown
Member Author

Clarification after closure: this PR carried a second, independent requirement that remains valid and should not be discarded with the sharding/cancellation experiment.

The current behavior gap is in the durable ClawSweeper review comment. When a fresh review wins its exact-revision lease, current main posts a separate transient clawsweeper-review-status:started comment, but the older durable Codex review: comment remains unchanged higher in the timeline until final publication. Its previous verdict can still look current, and the durable comment itself has no visible refreshing/interrupted marker. Head-mismatch stale handling does not cover a normal same-head or issue refresh interval.

The required invariant is still:

  • after lease election, the existing durable comment visibly becomes refreshing;
  • the prior review remains available as collapsed history but its verdict/action/security/version/identity markers are inactive;
  • successful publication replaces the matching projection in place;
  • timeout or abandoned refresh changes only the matching tuple to interrupted;
  • stale writers cannot overwrite a newer final review or newer projection;
  • presentation-write failure warns and does not block review compute.

This requirement originally lived independently in #645. That PR was closed only because #674 claimed to supersede it. Its first live implementation had a real blocker: GitHub issue-comment PATCH rejected the attempted If-Match conditional write, documented at #645 (comment) with runs https://github.com/openclaw/clawsweeper/actions/runs/29575130444 and https://github.com/openclaw/clawsweeper/actions/runs/29576709251.

The version later carried in this PR removed the unsupported If-Match dependency: after confirming the elected review lease it re-read the durable comment, compared the exact body, and treated a changed body as a no-op. Proposal-only evidence at https://github.com/openclaw/clawsweeper/actions/runs/29652620083 showed the durable comment at #646 (comment) transition from refreshing to interrupted with old automation markers removed. The follow-up https://github.com/openclaw/clawsweeper/actions/runs/29654503026 retained useful timeout/publication evidence, although ordinary branch workflow_dispatch still could not exercise a production claimed mutation.

Updated disposition:

  1. Keep Review each selected PR in its own item shard #674 closed; its combined generation/cancellation rollout remains rejected.
  2. Preserve the durable stale-comment requirement as a separate status-focused work item.
  3. Do not reopen the old Show stale review status during refresh #645 head unchanged because it contains the failed If-Match approach and predates later main changes such as fix(review): clean up superseded review-started placeholder comments #710.
  4. Port/reimplement the corrected projection against current main in a narrow PR with no per-item matrix, generation protocol, native cancellation, queue schema, dashboard, or telemetry changes.
  5. Reuse Test durable review status transitions #646 only as the proposal-only validation fixture, covering refreshing, interrupted, successful replacement, and stale-write rejection against the exact candidate head.

So the closure decision separates two concerns; it does not declare the stale durable-comment problem solved or unnecessary.

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. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. 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.

2 participants