Skip to content

fix(review): bind exact reviews to the leased PR head - #981

Draft
vincentkoc wants to merge 3 commits into
mainfrom
fix/exact-review-materialization
Draft

fix(review): bind exact reviews to the leased PR head#981
vincentkoc wants to merge 3 commits into
mainfrom
fix/exact-review-materialization

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Jul 31, 2026

Copy link
Copy Markdown
Member

What Problem This Solves

Fixes an issue where exact pull request reviews could claim one immutable head SHA while reviewing the target branch checkout instead, and OpenClaw reviews could run without the required sibling Codex source.

Why This Change Was Made

Exact PR runs now delegate leased-head fetch, validation, and detached checkout to a testable TypeScript command. A moved head returns a structured source_drift result that skips review, artifact creation, and lease cleanup while the durable queue requeues the latest source. OpenClaw review workers also materialize openai/codex beside the target checkout.

User Impact

Maintainers can trust that an exact ClawSweeper verdict applies to the SHA named by its queue lease and that Codex-backed OpenClaw contract checks have the required dependency source. There is no new configuration.

Evidence

  • Exact head: 302b879791f77c94305190d1e8ce730421209497 (signed).
  • pnpm run build:repair passed.
  • node --test test/repair/exact-review-source.test.ts test/sweep-workflow.test.ts: 107/107 passed, including matching head, source drift, unfetchable ref, and a shallow divergent-history fixture with a 75-commit PR plus 60 newer base commits.
  • git diff --check: clean.
  • Fresh uncommitted and committed-branch autoreview: 0 accepted/actionable findings; committed review confidence 0.90.
  • Full local pnpm run check: build, format, lint, coverage thresholds, and 2,847 tests passed; five unchanged Linux Landlock fixture tests cannot import on macOS because Apple libc has no capset. Exact-head Linux CI is the authoritative remaining gate.

Real Behavior Proof

  • Claim: exact PR review cannot proceed on a checkout that differs from the leased source SHA.
  • Exercised surface: exact-review target materialization, structured source-drift output, downstream review/artifact/lease gates, and OpenClaw Codex materialization.
  • Scenario: real temporary bare Git remotes cover a matching PR head, a force-moved PR head, and a missing PR ref.
  • Observed result: matching heads detach-checkout the exact leased SHA; moved heads return source_drift without changing the target checkout; missing refs fail closed. A shallow target whose base advanced 60 commits and whose PR contains 75 commits is fully unshallowed, preserves the original merge base, and exposes all 75 PR commits to origin/main..HEAD. Parsed workflow coverage proves source drift skips reservation, review, ledger finalization, artifact bundling, and unsuccessful lease release while exporting requeue_latest=true.
  • Limits: production exact-review confirmation requires this draft to pass Linux CI and later be deployed.

ClawSweeper Finding Disposition - July 31, 2026

  • Move leased-head handling out of workflow shell: addressed by repair:exact-review-source; workflow YAML now only clones the target, invokes the command, and branches on structured outputs.
  • Replace command-text assertions: addressed with real Git behavior tests for matching, moved, and unfetchable heads; workflow tests retain only wiring and side-effect-fence assertions.
  • Prevent every review-result side effect on drift: explicit guards now cover reservation, review, ledger finalization, artifact creation, and lease cleanup. Command-status reporting intentionally remains enabled so maintainers see that the latest source is waiting for review.
  • Requeue newest source reliably: source drift completes the generation as success with requeue_latest=true; fetch failure remains a hard failure rather than a false requeue.
  • Preserve PR ancestry beyond the fixed fetch depth: addressed by removing the PR-side depth cap, passing the canonical base branch into the source guard, and calling the shared merge-base materializer after checkout. The new divergent-history fixture starts from a depth-1 base clone and proves both sides are complete before review.
  • Remove the release-owned changelog entry: not applied. Current origin/main AGENTS.md reserves the OpenClaw target repository's CHANGELOG.md during foreign PR work; CONTRIBUTING.md explicitly says ClawSweeper follows its own release-note policy. This PR changes ClawSweeper itself, and the operationally meaningful exact-review change therefore keeps its ClawSweeper changelog entry.
  • Exact-head CI is now complete: pnpm check, CodeQL, containment smoke, sparse repair build, Windows launcher, and production automerge E2E are green.

@vincentkoc
vincentkoc force-pushed the fix/exact-review-materialization branch from 85284ab to 12b5e4b Compare July 31, 2026 18:28
@clawsweeper clawsweeper Bot added 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. 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 31, 2026
@clawsweeper

clawsweeper Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed August 2, 2026, 5:05 PM ET / 21:05 UTC.

ClawSweeper review

What this changes

The PR adds a typed Git source guard that verifies an exact-review lease still matches the pull request head, skips review side effects when it moved, and materializes Codex source for OpenClaw contract review.

Merge readiness

⚠️ Ready for maintainer review - 3 items remain

Keep open. This is a MEMBER-authored, concrete automation-correctness fix with a credible narrow design, but its head is 31 current-main commits behind its merge base and GitHub reports a dirty merge, so the workflow integration must be reconciled before it can safely land.

Priority: P2
Reviewed head: 302b879791f77c94305190d1e8ce730421209497

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The source-guard design and focused coverage are useful, but the dirty, substantially stale branch has not yet demonstrated safe integration with current exact-review publication and lifecycle behavior.
Proof confidence 🌊 off-meta tidepool Not applicable: This PR is authored by a repository MEMBER, so the external-contributor proof gate does not apply; its body nevertheless documents concrete temporary-Git-remote behavior for the proposed guard.
Patch quality 🦐 gold shrimp (3/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: This PR is authored by a repository MEMBER, so the external-contributor proof gate does not apply; its body nevertheless documents concrete temporary-Git-remote behavior for the proposed guard.
Evidence reviewed 5 items Leased-head guard: The proposed helper fetches the pull-request head, compares it to the required leased SHA, returns a structured source-drift result on mismatch, and only detaches/checks merge-base ancestry after an exact match.
Workflow side-effect fencing: The proposed workflow wires source-drift outputs into the checkout/review sequence and its focused workflow test asserts that review, reservation, ledger finalization, artifact creation, and unsuccessful lease release are skipped on drift.
Current-main integration drift: The branch diverged from merge base 3637a94: current main has 31 additional commits while the PR has 3, including newer exact-review publication and lifecycle work that the branch predates.
Findings None None.
Security None None.

How this fits together

Exact event reviews lease a particular pull-request revision, check out the target repository, then produce review artifacts and publish queue/lifecycle results. The new source guard sits after checkout and before review work so a moved pull-request head is requeued instead of receiving a verdict for the wrong revision.

flowchart LR
  Queue[Exact review queue] --> Lease[Leased PR head]
  Lease --> Checkout[Target repository checkout]
  Checkout --> Guard[Verify fetched PR head]
  Guard -->|Head matches| Review[Review and artifact creation]
  Guard -->|Head moved| Requeue[Requeue latest source]
  Review --> Publish[Queue and lifecycle publication]
Loading

Before merge

  • Resolve merge risk (P1) - GitHub reports this draft as dirty, and its current merge base is 31 main commits behind; merging requires a conflict-resolving rebase rather than treating the branch as an isolated workflow edit.
  • Resolve merge risk (P1) - The branch predates current direct-publication and lifecycle-finalization paths, so the source-drift no-side-effect guarantee must be revalidated across those current paths after rebase.
  • Complete next step (P2) - The concrete branch needs an integration-aware rebase and current-head proof; its dirty merge and post-base workflow changes make this unsuitable for automatic repair routing.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Patch scope 7 files affected; 432 added, 14 removed The change spans workflow orchestration, a new repair command, focused tests, package scripts, and release notes.
Base drift 31 main commits ahead; 3 branch commits The PR predates substantive exact-review publication and lifecycle changes, making rebase integration material to merge safety.

Merge-risk options

Maintainer options:

  1. Reconcile the current workflow before merge (recommended)
    Rebase onto current main, resolve the dirty merge around exact-review publication and lifecycle steps, then rerun focused source-drift proof against the rebased workflow.

Technical review

Best possible solution:

Rebase the typed source guard onto current main, preserve the current direct-publication and lifecycle flow, and prove that a moved leased head produces only the intended requeue result with no review, artifact, ledger, or lease-cleanup side effects.

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

Yes, from source. The focused fixture creates real temporary Git remotes and models matching, moved, missing, and shallow divergent pull-request heads; this read-only review did not execute that fixture.

Is this the best way to solve the issue?

Unclear. A typed pre-review source guard is the right narrow boundary, but this branch must first be integrated with the newer direct-publication and lifecycle workflow paths on current main.

AGENTS.md: found and applied where relevant.

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

Labels

Label changes:

  • remove proof: sufficient: Current real behavior proof status is not_applicable, not sufficient.

Label justifications:

  • P2: A wrong-head review can misstate exact-review results, but the affected automation has a bounded recovery path through queue reprocessing.
  • merge-risk: 🚨 automation: The diff changes exact-review queue, checkout, artifact, ledger, and lease orchestration where an incomplete rebase can alter post-review automation behavior.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🌊 off-meta tidepool and patch quality is 🦐 gold shrimp.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: This PR is authored by a repository MEMBER, so the external-contributor proof gate does not apply; its body nevertheless documents concrete temporary-Git-remote behavior for the proposed guard.

Evidence

What I checked:

  • Leased-head guard: The proposed helper fetches the pull-request head, compares it to the required leased SHA, returns a structured source-drift result on mismatch, and only detaches/checks merge-base ancestry after an exact match. (src/repair/exact-review-source.ts:42, 302b879791f7)
  • Workflow side-effect fencing: The proposed workflow wires source-drift outputs into the checkout/review sequence and its focused workflow test asserts that review, reservation, ledger finalization, artifact creation, and unsuccessful lease release are skipped on drift. (.github/workflows/sweep.yml:883, 302b879791f7)
  • Current-main integration drift: The branch diverged from merge base 3637a94: current main has 31 additional commits while the PR has 3, including newer exact-review publication and lifecycle work that the branch predates. (.github/workflows/sweep.yml:1258, de31c9959070)
  • Recent exact-review ownership: Current main's direct publication bound was introduced by Vincent Koc in commit 0925eca, and the lifecycle projection was introduced by Martin Cleary in commit b07ab75; both postdate this branch's merge base and are relevant to a safe rebase. (.github/workflows/sweep.yml:1352, 0925ecae3787)
  • Patch hygiene: The proposed three commits have no whitespace errors against their merge base. (302b879791f7)

Likely related people:

  • vincentkoc: Authored the current-main exact-review publication bound and the proposed guard, so has direct context on queue/publication integration. (role: recent exact-review contributor; confidence: high; commits: 0925ecae3787, 302b879791f7; files: .github/workflows/sweep.yml, src/repair/exact-review-source.ts)
  • Martin Cleary: Introduced the current-main exact-review lifecycle projection that the rebase must preserve. (role: lifecycle projection contributor; confidence: high; commits: b07ab751813c; files: .github/workflows/sweep.yml)
  • Peter Steinberger: Recent current-main commits continue to maintain the sweep workflow and exact-review finalization paths after this PR's merge base. (role: recent workflow contributor; confidence: medium; commits: de31c9959070, 7e4d8cb7cee4; files: .github/workflows/sweep.yml)

Rank-up moves

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

  • Rebase and resolve the current workflow integration.
  • Run the exact-review source and workflow tests against the rebased head and refresh the PR-body proof.

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 (15 earlier review cycles; latest 8 shown)
  • reviewed 2026-08-01T16:18:58.622Z sha 302b879 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-01T17:33:42.183Z sha 302b879 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-01T21:23:20.390Z sha 302b879 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-01T22:37:58.621Z sha 302b879 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-02T08:55:06.575Z sha 302b879 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-02T12:47:35.836Z sha 302b879 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-02T15:26:11.506Z sha 302b879 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-02T17:35:29.303Z sha 302b879 :: needs maintainer review before merge. :: none

@vincentkoc

Copy link
Copy Markdown
Member Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 31, 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: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

@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: 🦐 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. labels Jul 31, 2026
@vincentkoc

Copy link
Copy Markdown
Member Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 31, 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: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

@vincentkoc

Copy link
Copy Markdown
Member Author

@clawsweeper re-review

@clawsweeper clawsweeper Bot removed the proof: sufficient Contributor real behavior proof is sufficient. label Jul 31, 2026
@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. 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. 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant