fix: reviews whose repository inspection failed still publish a rating and close items - #988
Conversation
`local_checkout_access: verified` is written as a literal in the review-record template, so `hasVerifiedLocalCheckoutAccess` was true for every record. The three guards that consult it could not fire, including the apply guard whose skip reason is "review lacks verified local checkout access". `hasBlockedLocalCheckoutAccess` already detects a review that reports its read-only inspection never ran, but its only consumer was `effectiveReviewStatus`, which affects re-review scheduling and not publication or close. A review that recorded a sandbox startup failure was therefore still comment-synced and still eligible to close. Require both signals so the existing guards apply. `effectiveReviewStatus` is unchanged: it tests the blocked predicate first, so the `stale_local_checkout_unverified` branch stays unreachable for blocked records. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Codex review: needs real behavior proof before merge. Reviewed August 3, 2026, 5:05 AM ET / 09:05 UTC. ClawSweeper reviewWhat this changesThe branch changes local-checkout verification so a review record that reports blocked repository inspection cannot be comment-synced or automatically closed, and adds a focused apply dry-run test. Merge readiness⛔ Blocked until stronger real behavior proof is added - 9 items remain This PR addresses a real fail-safe gap, but its single production edit targets the pre-refactor monolith while current Priority: P1 Review scores
Verification
How this fits togetherClawSweeper’s review lane emits durable Markdown records; the apply lane reads their metadata before synchronizing a review comment or closing an eligible GitHub item. The local-checkout gate is the safety boundary that prevents automation from acting on a review whose source inspection did not run. flowchart LR
A[Codex review record] --> B[Inspection evidence]
B --> C[Record metadata gate]
C --> D[Apply decision workflow]
D --> E[Durable review comment]
D --> F[Automatic item close]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Copy recommended automerge instructionTechnical reviewBest possible solution: Rebase onto current Do we have a high-confidence way to reproduce the issue? Yes, for current Is this the best way to solve the issue? No, not as submitted: the fail-safe rule is appropriate, but the branch changes a pre-refactor monolithic helper rather than the active record-metadata implementation that feeds current apply guards. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 2d1a40bfa643. LabelsLabel justifications:
EvidenceAcceptance criteria:
What I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (16 earlier review cycles; latest 8 shown)
|
What Problem This Solves
When ClawSweeper's read-only repository inspection fails to start, the review it publishes still
carries a Patch quality tier and a merge-readiness verdict, and the item stays eligible for the
apply lane to close. The review text says the opposite in the same comment — for example:
while the scores table in the same comment reports:
Maintainers and contributors read a rating and a "no findings" rationale produced by an inspection
that provably did not run, and the item can be closed on that basis.
This is not rare. Of the ClawSweeper comment bodies on
openclaw/openclawlast updated between2026-07-27 and 07-31T12:00Z, 203 of 8,515 currently match
bwrap/RTM_NEWADDR— a steady2.4% share on each of those days. Among bodies last updated during 2026-07-31T14:00Z and
15:00Z that share was 37.2% and 49.3%. Because reviews land as in-place comment edits these
are lower bounds, and they count comment bodies rather than review runs; the exact collection method
and what it does and does not prove are in Real Behavior Proof below.
Why This Change Was Made
The repository already has the guard for this case.
hasBlockedLocalCheckoutAccessdetects a reviewthat reports its inspection never ran, and three apply-lane call sites gate on
hasVerifiedLocalCheckoutAccess: the guard whose skip reason is literallyreview lacks verified local checkout access, the comment-sync guard, and the counterpart check inthe same-author PR+issue pair close.
Those guards could not distinguish this case.
local_checkout_access: verifiedis emitted as aliteral by the review-record writer, so for every record that writer produces
hasVerifiedLocalCheckoutAccesswas true regardless of what the review reported. The blockedpredicate's only consumer was
effectiveReviewStatus, which classifies the recordstale_local_checkout_blocked— that affects re-review scheduling, not publication or close.The change requires both signals, so the existing guards apply to reviews that already report a
blocked inspection. No new configuration, no new state, no change to how ratings are rendered.
effectiveReviewStatusis unchanged for every input: it tests the blocked predicate first, soblocked records still return
stale_local_checkout_blocked, and non-blocked records fall through tothe same front-matter check as before. That is an invariant by construction in a four-line function,
not something a new test asserts.
Non-goals.
in the review runtime, not here.
src/repair/workflow-utils.tsreadslocal_checkout_accessfrom the front matter directly and isbuilt as a separate module that does not import this predicate. It is unchanged: blocked
records remain selectable there. That site produces close candidates for the apply lane rather
than closing anything itself, so the apply guard above is still what withholds the close — but the
selection is wasted work and making that module agree is left for a separate change.
separate question.
User Impact
On the ordinary apply path, a review whose report matches the existing blocked-inspection predicate
no longer syncs its comment and is not eligible to close. Such a record is already classified
stale_local_checkout_blocked, so it stays due for re-review. This change does not remove or edit ablocked comment that was already published, and it does not make the scheduled re-review succeed; it
stops the lane from publishing a new one or closing on it.
A matching record also no longer qualifies as the counterpart in a same-author PR+issue pair close,
so that pair is kept open instead. Same fail-safe direction, and no focused test covers that path.
The withholding is bounded in two ways, both deliberate:
promisor/DNS variant in Limits below is a counterexample that this change does not catch.
shouldProbeClosedStateskips the apply guard, andstaleCanonicalCommentSyncPendingskips the comment-sync guard. Both are narrow recovery states,and this change does not alter either condition.
For records that do not match the predicate, nothing changes.
Evidence
src/clawsweeper.ts+7/−1, plus one focused test in the narrowest matching file(
test/apply-blocked-local-checkout.test.ts, alongside the existingapply-*policy tests).Real Behavior Proof
Claim. A review record whose body reports that its read-only inspection never ran is admitted
past the apply lane's local-checkout guard on
main, and is withheld after this change with the skipreason the guard already defines. The claim is scoped to the ordinary apply path — the exception
branches above and
src/repair/workflow-utils.tsare not exercised.Environment. Node v24.18.0, Linux. Base
0ee212e35. Branch head69926bd38.1. Non-mocked dry-run against a live GitHub item
Real
ghat/usr/bin/gh, noGH_BIN/GH_BIN_ARGSoverride (env | grep -c '^GH_BIN'→0).--dry-runshort-circuits before the comment upsert (src/clawsweeper.ts:29492) and before the close(
src/clawsweeper.ts:29827), so every path below is read-only. Following the apply-repro guidance inAGENTS.md, one record for the real open issueopenclaw/clawsweeper#951was copied into a tempitems/dir with a temp--record-root,--closed-dir, and--plans-dir. The record declareslocal_checkout_access: verifiedandlabels: [], and its body carries the sandbox-failure sentence.With this change (head
69926bd38):Same record, same command, unmodified base
0ee212e35:On base the record is admitted past the local-checkout guard and evaluated further, reaching the live
protected-label check. That check reports labels the record never declared, so the run genuinely
queried GitHub —
ghwas not mocked. With the change the same record stops at the guard.This particular item carries protected labels, so on base it stops there rather than at
would close; the admitted-through-to-close path is what the harness test below shows.2. Focused harness test (before / after)
Exercised surface. The same apply entry point,
node dist/clawsweeper.js apply-decisions, drivenend to end through
runApplyDecisionsForTest, withghmocked at the process boundary viawithMockGhso the close proposal can be carried to completion deterministically.Scenario / fixture. One review record in a temp
items/dir, carrying the front-matter value thereview writer emits (
local_checkout_access: verified, asserted in the test) and, in the body, thesandbox-failure sentence quoted above. Decision
close/implemented_on_main, with the durablereview comment already synced.
Command.
Observed result — on
main(fix reverted, test kept):The item is comment-synced and closed. The guard never fires.
Observed result — with this change:
Reverting only
src/clawsweeper.tsto0ee212e35and rebuilding reproduces the failure above, so thetest has teeth.
3. Public measurement
Reproducible from the GitHub API. For every
clawsweeper[bot]issue comment onopenclaw/openclawwhose body was last updated in the window, the current body was matched against
/bwrap|RTM_NEWADDR/and bucketed by
updated_at.What this is and is not:
matchingcounts bodies matching/bwrap|RTM_NEWADDR/, which is anarrower query than
hasBlockedLocalCheckoutAccess— the predicate also recognises five othersandbox phrasings, so the true share is at least this. Buckets are comment bodies last updated in
that bucket, not publication events and not per-day review incidence. Reviews land as in-place
edits, so a body written during an outage and re-reviewed afterwards now reads clean and is counted
clean. A match shows the published text reports a failed inspection; it is not by itself evidence
that any apply action ran on that item.
4. Suite and static checks
node scripts/run-node-tests.mjs all→ 2851 tests, 2845 pass, 6 fail. The same 6 fail onunmodified
0ee212e35in this environment (test/repair/target-validation.test.ts— detached-processreaping and git branch plumbing, both needing a real pnpm install and worktree setup this checkout does
not have).
oxfmt --check,oxlint(src / scripts+test),check:active-surface,check:limits,check:dashboard-queue-boundaryall pass.Limits — what this proof does not cover
pair-close counterpart check, the
shouldProbeClosedStateandstaleCanonicalCommentSyncPendingbypasses, and the close-promotion selector in
src/repair/workflow-utils.tsare untested here.The
effectiveReviewStatusinvariant is argued by construction, not asserted by a new test.openclaw/clawsweeper#951carries protected labels, so on base it reaches
skipped_protected_labelrather than a close. Thenon-mocked run therefore demonstrates that the checkout guard does not fire on base and does fire
after the change; the close consequence itself is shown only under the mocked harness.
clawsweeper[bot]closed 10openclaw/openclawitems in the last 7 days — but none of the itemsclosed during the sampled window carried a matching review. The 39 matching items closed between
14:00Z and 16:51Z were all closed by humans. The close behaviour above is demonstrated on the real
apply path, not observed in the wild; the published-rating harm is what is observed.
note above it.
hasVerifiedLocalCheckoutAccesswas true forevery record the current review-record writer produces, because that writer emits the literal. It says
nothing about hand-edited, malformed, or pre-existing records that lack the key; those already
resolved to unverified and still do.
replaced the
bwrapfailure withpromisor objects; Git attempted to fetch them and failed because github.com could not resolve— plausibly the same root, buthasBlockedLocalCheckoutAccessdoes notmatch that phrasing, so such a review is not even classified stale. Widening the predicate from one
observed sample seemed worse than leaving it to a decision about how the reviewer should report
inspection failure structurally; the review prompt currently asks for prose only.
legitimately discusses
bwrap: loopbackwould be treated as blocked. That already causes permanentstaleness on
main; with this change it would also withhold the comment on the ordinary path. Thedirection is fail-safe — the lane declines to act rather than acting on a review it cannot trust — but
it is a real behaviour change for that case.