Skip to content

Show stale review status during refresh#645

Closed
hxy91819 wants to merge 1 commit into
mainfrom
feature/durable-review-status
Closed

Show stale review status during refresh#645
hxy91819 wants to merge 1 commit into
mainfrom
feature/durable-review-status

Conversation

@hxy91819

Copy link
Copy Markdown
Member

Summary

  • project an existing durable ClawSweeper review into a visible stale/in-progress state after the exact-head review lease is confirmed
  • collapse the previous review while stripping actionable verdict, action, security, version, and identity markers
  • replace abandoned or expired refreshes with a tuple-bound interrupted state
  • preserve prior-review digest, context, and review-history continuity for cache revalidation and the next publication
  • ignore canonical refresh projections in the repair router

Root cause

ClawSweeper coordinates a new review with a separate transient lease comment, but the older durable review comment remains unchanged higher in the timeline. That makes the previous verdict look current while a new review is already running and forces maintainers to inspect comment history or find the transient status manually.

Safety

  • the dedicated lease comment remains the authoritative coordination record; the durable projection is presentation-only and best effort
  • projection writes happen only after lease election and use the live comment ETag with If-Match, so a stale read cannot overwrite a concurrently published verdict or newer projection
  • publication remains fenced to the matching refreshing lease tuple; interrupted tuples cannot publish
  • old automation markers are removed, including for comments near GitHub's size limit, and the repair router explicitly ignores validated projections
  • malformed projections and displaced-body digest mismatches fail closed

Validation

  • pnpm run check
  • focused review status, history, rendering, and repair-router tests (180 passing)
  • gitleaks protect --staged --redact --no-banner (no leaks found)
  • autoreview --mode local (clean after addressing three actionable findings)

@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 17, 2026
@clawsweeper

clawsweeper Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 17, 2026, 1:32 AM ET / 05:32 UTC.

Summary
The PR projects existing durable reviews into non-actionable refreshing or interrupted states, preserves displaced review history, and prevents those projections from entering repair routing.

Reproducibility: not applicable. as a bug reproduction: this is a maintainer-facing status feature, although the existing separate-lease and unchanged-durable-comment behavior clearly establishes the UX gap.

Review metrics: 2 noteworthy metrics.

  • Changed surface: 9 files; 698 additions, 24 deletions. The change spans central review coordination, a new status parser/renderer, repair routing, documentation, and focused tests.
  • Coverage surface: 4 test files changed. Projection rendering, history continuity, source integration, and repair-router suppression each receive targeted coverage.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🌊 off-meta tidepool
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

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

Rank-up moves:

  • Run one controlled test-PR refresh that captures successful replacement, lease interruption, and a rejected stale conditional write.

Risk before merge

  • [P1] Merging changes live review-comment mutation and routing behavior; a controlled test-repository run should confirm that concurrent publication wins over stale projection writes and that expired leases cannot publish or trigger repair.

Maintainer options:

  1. Validate live transitions (recommended)
    Exercise the branch on a test PR and retain redacted comments and workflow logs showing successful, interrupted, and concurrent-publication paths before merge.
  2. Accept test-backed risk
    Merge after required checks if maintainers consider the tuple, digest, and conditional-write coverage sufficient without a controlled live run.

Next step before merge

  • [P2] This is a normal member-authored draft with no concrete repair finding; continue through ordinary maintainer review and validation rather than opening an automated fix lane.

Security
Cleared: No dependency, workflow-permission, secret-access, or supply-chain change is present, and the comment markers are treated as non-authoritative presentation data with fail-closed validation.

Review details

Best possible solution:

Keep the transient lease authoritative and the durable projection presentation-only, then land the change after a controlled GitHub run demonstrates completed-to-refreshing, refreshing-to-interrupted, and refreshing-to-new-review transitions without stale writes or repair routing.

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

Not applicable as a bug reproduction: this is a maintainer-facing status feature, although the existing separate-lease and unchanged-durable-comment behavior clearly establishes the UX gap.

Is this the best way to solve the issue?

Yes, provisionally. Reusing the elected lease tuple and conditionally projecting into the existing durable comment avoids creating another coordination authority, while digest checks and marker removal constrain stale automation behavior.

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add P2: The PR addresses misleading maintainer-facing review state and stale automation markers with a bounded operational blast radius.
  • add merge-risk: 🚨 automation: The diff changes review leases, conditional GitHub comment mutation, verdict-marker interpretation, history extraction, and repair routing.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The contributor proof gate does not apply because the supplied GitHub context identifies the author as a repository member; the reported checks and focused tests remain useful supplemental validation.

Label justifications:

  • P2: The PR addresses misleading maintainer-facing review state and stale automation markers with a bounded operational blast radius.
  • merge-risk: 🚨 automation: The diff changes review leases, conditional GitHub comment mutation, verdict-marker interpretation, history extraction, and repair routing.
  • 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: The contributor proof gate does not apply because the supplied GitHub context identifies the author as a repository member; the reported checks and focused tests remain useful supplemental validation.
Evidence reviewed

What I checked:

  • Protected author status: The supplied GitHub context identifies the author as a repository MEMBER, so repository policy requires this PR to remain open for explicit maintainer handling. (f58a3278394c)
  • Current behavior: Current-main documentation describes a separate transient start-status comment while the durable review remains unchanged until final synchronization, matching the presentation gap described by the PR. (docs/pr-review-comments.md:27, 0a77e4f25c21)
  • Projection implementation: The new module defines tuple-bound refreshing/interrupted projections, displaced-body digests, identity validation, marker sanitization, and comment-size handling. (src/review-comment-status.ts:1, f58a3278394c)
  • History continuity: Latest-review extraction unwraps validated projections and restores the displaced review timestamp, SHA, digest, and history rather than treating the temporary projection as a completed verdict. (src/clawsweeper.ts:5688, f58a3278394c)
  • Repair isolation: Trusted automation parsing explicitly ignores durable refresh projections so displaced action markers cannot route repair work while stale. (src/repair/comment-router-core.ts:1668, f58a3278394c)
  • Focused coverage: The branch adds dedicated projection tests and extends review-history, rendering, and repair-router coverage; the PR reports 180 focused tests plus the repository check passing. (test/review-comment-status.test.ts:1, f58a3278394c)

Likely related people:

  • hxy91819: The supplied context identifies them as a repository member, and their branch integrates the change across durable review rendering, lease coordination, history extraction, and repair routing. Prior merged history on these exact paths was not available, so ownership confidence remains medium. (role: automation-area contributor and current implementation owner; confidence: medium; commits: f58a3278394c; files: src/clawsweeper.ts, src/review-comment-status.ts, src/review-history.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.

@hxy91819

Copy link
Copy Markdown
Member Author

Live validation result: partial pass, blocking failure

I exercised this branch against disposable test PR #646 using workflow_dispatch with --ref feature/durable-review-status.

What passed

What failed

  • The durable comment never entered clawsweeper-review-status:refreshing while lease comment 5002510417 was active.

  • The review-shard log shows the fresh projection write was rejected:

    [review] could not project durable review refresh for #646:
    gh api ... --method PATCH -H If-Match: W/"53562..." ...
    gh: Bad Request (HTTP 400)
    
  • A controlled 30-second Codex timeout created lease comment 5002756860, then removed it when the review failed, but the durable comment entered neither refreshing nor interrupted.

  • Interruption evidence run: https://github.com/openclaw/clawsweeper/actions/runs/29576709251. Its log repeats the same HTTP 400 on the fresh projection write before the expected Codex failure.

  • I also tested a temporary user-owned probe comment directly. Both GitHub's weak ETag (W/"...") and the same value without W/ produced HTTP 400 on issue-comment PATCH. The probe comment was deleted afterward.

Conclusion

GitHub issue-comment PATCH does not accept this If-Match usage. The current implementation therefore rejects fresh projection writes, not only stale writes. Final publication still replaces the durable comment, but the maintainer-facing refreshing/interrupted states do not work, and the requested stale-conditional-write proof cannot pass.

This live proof should be treated as blocking for the current implementation. The concurrency design needs to stop relying on HTTP If-Match for issue-comment PATCH, then the same three-path validation should be rerun on PR 646.

@hxy91819

Copy link
Copy Markdown
Member Author

Superseded by #674, which includes this durable review-status behavior and its tests together with the per-item shard architecture. Closing this draft to keep one active implementation path.

@hxy91819 hxy91819 closed this Jul 19, 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. 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.

1 participant