fix(review): stop ClawSweeper's own check runs from expiring the review cache - #971
fix(review): stop ClawSweeper's own check runs from expiring the review cache#971masatohoshino wants to merge 1 commit into
Conversation
…ew cache
itemContentDigest fed context.pullChecks into the digest verbatim, and
pullChecksContext fills that from commits/{headSha}/check-runs with no author or
app filter. A completed review writes its labels and durable comment, which
retriggers clawsweeper-dispatch and github-activity; those add check runs to the
head, the digest changes, reviewContentCacheHit misses, and the unchanged head is
reviewed again -- which writes labels again. On openclaw#948's head 144236b, 26 of 38
check runs were those two jobs, and 13 of them were byte-identical after
compactCheckRun.
Across every review comment on this repo the durable comments record 154
consecutive re-reviews of an unchanged head, median gap 33 minutes, below the
HOURLY_REVIEW_MS floor that reviewCadenceMs can return.
De-duplicate the compacted runs inside the digest. Once a run is reduced to
{name,status,conclusion,app} a repeat reports nothing the first one did not,
while a run that newly fails differs in conclusion and keeps its own entry. This
touches the cache key only: pullChecksContext is unchanged, so the check list the
reviewer sees through reviewContextLedger is unchanged.
The same bot was already excluded from timeline, labels, and PR review comments;
checks were the remaining input.
|
Codex review: needs maintainer review before merge. Reviewed July 30, 2026, 6:21 PM ET / 22:21 UTC. ClawSweeper reviewWhat this changesThe branch de-duplicates identical compact pull-request check-run records in the content-cache digest and adds focused regression tests so repeated ClawSweeper automation runs do not trigger another full review. Merge readinessThis draft PR addresses a concrete review-cache churn bug with a narrow implementation and focused proof. It remains necessary because the linked canonical issue is open and the proposed fix is not on current Priority: P2 Review scores
Verification
How this fits togetherClawSweeper gathers pull-request state, including check runs, then derives cache keys before deciding whether an unchanged item can skip a full Codex review. This patch changes only content-cache normalization; the check context supplied to the reviewer remains unchanged. flowchart LR
A[Pull request activity] --> B[Collected check runs]
B --> C[Content-cache normalization]
C --> D[Content-cache key]
D --> E{Cache hit?}
E -->|Yes| F[Skip full Codex review]
E -->|No| G[Run Codex review]
G --> H[Review labels and comment]
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Accept this as a narrowly scoped content-cache repair only if maintainers agree that duplicate compact check-run multiplicity is not meaningful review input, while preserving the focused tests for changed outcomes and distinct checks. Do we have a high-confidence way to reproduce the issue? Yes, at source level: the PR provides a focused harness using the production digest and cache predicate with repeated real-shaped check-run records. The submitted after-fix output shows a cache hit for duplicate churn and a miss when a check newly fails. Is this the best way to solve the issue? Yes, provisionally: changing only the content-cache representation is the narrowest way to stop duplicate check-run churn without changing reviewer-visible check context. The remaining question is whether collapsing identical matrix entries is an accepted cache-key trade-off. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against c8f44886fde4. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
Fixes #967.
What this fixes
itemContentDigestfedcontext.pullChecksinto the review content digestverbatim, and
pullChecksContextfills that fromcommits/{headSha}/check-runs?per_page=100with no author or app filter — so itincludes ClawSweeper's own runs. That closes a loop:
clawsweeper-dispatch.yml(jobdispatch) andgithub-activity.yml(jobnotify) both trigger onlabeled/unlabeled/issue_commentpullCheckschanges, so the digest changesreviewContentCacheHitmissesOn #948's head
144236be, 26 of 38 check runs were those two jobs: 13notifyand 13
dispatch. AftercompactCheckRunreduces a run to{name,status,conclusion,app}, 13 of them are byte-identical to each other.The same bot was already excluded from every sibling digest input —
timelineviaCLAWSWEEPER_BOT_AUTHORS, labels viaisIgnorableSourceRevisionLabel, PR reviewcomments via
isClawSweeperComment, each with its own test intest/review-content-cache.test.ts. Checks were the remaining input.This de-duplicates the compacted runs inside the digest. Once a run is reduced to
that tuple a repeat reports nothing the first one did not, while a run that newly
fails differs in
conclusionand keeps its own entry.The change is confined to the content-cache key.
pullChecksContextisuntouched, so the check list the reviewer is shown through
reviewContextLedgerisunchanged.
To be precise about what this does and does not stop: the semantic cache is
consulted first and the content cache second, as independent skips. Duplicate runs
still churn the semantic and structural digests, which also read
pullChecksraw(
src/review-semantic-cache.ts:785,src/clawsweeper.ts:9168), so an unchangedhead still pays context hydration and revalidation. What it stops is the expensive
part — falling through both gates into a full Codex review of an item whose
content did not change.
Evidence
Head
7065c579, baseorigin/main3dc70e67. Both captures drive the exporteditemContentDigestForTestandreviewContentCacheHitwith the check-run listactually observed on
144236be(38 runs, 26 of themdispatch/notify).Before this change:
After:
The discriminator is section 4: over the same inputs,
reviewContentCacheHitgoesfalse→truefor pure bot churn while stayingfalsefor a real failing check.Scale
The durable review comments record prior cycles as
- reviewed <ts> sha <sha> :: <verdict>, so the rate is public:reviewCadenceMsnever returns less thanHOURLY_REVIEW_MS, so most of those arebelow the intended floor. The same command against
openclaw/openclawgives 145pairs, a 10-minute median, 134 under 90 minutes, and 22 verdict changes (15%).
Two caveats on those numbers. They come only from the history block, which records
prior cycles — the in-progress verdict marker is not counted, so they understate
the total. And the counts drift upward as new reviews land, so re-running gives
slightly different totals than the snapshot above.
The 15% verdict-change figure is a correlation I can measure but not attribute:
this loop explains why the extra cycles happen, not why two runs over identical
input disagree. Reducing the cycles should reduce the churn either way.
Tests
Four added to
test/review-content-cache.test.ts, beside the three existingbot-exclusion tests:
nameor inappstay distinct (no over-collapsing)checkRunsarray is passed through untouchedReverting the one-line call site in
itemContentDigestfails exactly the first ofthose and nothing else. The test imports
../dist/clawsweeper.js, so each sourceedit is followed by a rebuild:
The existing
content digest busts when bounded PR check state changespassesunchanged.
Proof summary
Behavior addressed: a completed review invalidates its own content-cache
entry, so an unchanged PR head is re-reviewed repeatedly.
Real environment tested: the production
itemContentDigestandreviewContentCacheHit, driven with the real check-run list from a real head;plus the public review-history measurement above.
Exact commands run after this patch:
Observed result after fix:
pass 40 / fail 0, andreviewContentCacheHitreturns
truefor pure bot churn andfalsefor a newly failing check.What was not tested: this is not deployed, so there is no production run of
the scheduler with the fix in place. The loop is demonstrated at the digest and
cache-predicate level, not end to end — I cannot run the review scheduler.
pnpm run lintexits 0.pnpm run checkruns 2,748 tests with 6 failures, allhost-caused and all in
test/repair/target-validation.test.ts, which this patchdoes not touch: three need
git worktree list -z(this host has Git 2.34.1,-zarrived in 2.36) and three cannot resolve the bun/npm/pnpm containment fixtures.
Not in scope
pullChecksraw(
src/review-semantic-cache.ts:785,src/clawsweeper.ts:9168). The samede-duplication applies there, but the helper would have to move to a module both
sides can import, and changing three cache keys at once invalidates three stored
cache families in one deploy. Happy to do it here if you would rather have it in
one change — I kept it out because the model review is the expensive gate and
this alone closes it.
item takes one extra review on first encounter after deploy, then settles.
compactCheckRunkeeps only
{name,status,conclusion,app}, and some names are the unexpandedtemplate (
containment smoke (sample ${{ matrix.sample }})), so shards thatagree on all four fields become one entry. Adding or removing such a shard no
longer busts the content cache. A shard that differs — including one that
fails — keeps its own entry, so "at least one failed" is preserved; "how many
passed" is not.
pullChecksContextis unchanged. De-duplicating there would also shrink thenoise in the review prompt, but it would change reviewer input rather than only
the cache key, so it belongs in its own change.
app.slugisgithub-actionsfor both thedispatch/notifyruns and genuine CI, so itwould need a workflow-name allowlist that drifts as workflows are renamed.
checkRunsTruncatedabove 100 runs. These runs accumulate toward that ceiling,but nothing has crossed it: across the 60 most recent PRs on this repo the
largest head carried 37 check runs (38 counting fix(ci): build the main bundle in the jobs that publish the action ledger #948). Worth watching, not a
live failure.
loop alone explains. That looks like a separate lane or lease issue and is not
addressed here.
often they are asked to. This change reduces how often.
Note on #589
#589 adds
pullBaseDriftDigestPartsfor the same reason on a different digestinput ("Excluding raw age avoids daily cache churn"). Both PRs add a helper next to
reviewTimelineDigestPartsand a field toitemContentDigest, so whichever landssecond needs a trivial merge. No behavioral overlap.
Harness used for the after-fix capture
Drop into a checkout and run after
pnpm run build. No credentials needed.