fix(review): stop re-reviewing unchanged heads after duplicate checks - #970
Conversation
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
clawsweeper
Bot
added
proof: sufficient
|
Codex review: needs maintainer review before merge. Reviewed July 31, 2026, 6:01 AM ET / 10:01 UTC. ClawSweeper reviewWhat this changesThe PR de-duplicates byte-identical compacted pull-request check runs before structural, semantic, and content cache identities are calculated, preventing repeated equivalent checks from forcing another full review. Merge readinessKeep open for maintainer review. This is a focused repair for a source-reproducible cache-churn bug, and the updated branch applies the shared normalization at all three review-cache boundaries while preserving distinct check-run outcomes; no concrete patch defect was found. Priority: P2 Review scores
Verification
How this fits togetherClawSweeper collects GitHub pull-request check state alongside source, discussion, and review metadata to decide whether an existing review can be reused. This change normalizes only the check-state input to the three cache keys; the reviewer-visible check context and collection completeness rules continue downstream unchanged. flowchart LR
A[GitHub pull-request check runs] --> B[Compacted check state]
B --> C[Duplicate-run normalization]
C --> D[Structural cache key]
C --> E[Semantic cache key]
C --> F[Content cache key]
D --> G[Reuse or refresh review]
E --> G
F --> G
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Land the narrow shared cache-key normalization with its identity guardrails, then observe one post-deploy cache refresh and confirm that equivalent repeated check runs no longer schedule full re-reviews while changed conclusions still do. Do we have a high-confidence way to reproduce the issue? Yes, source-reproducible with medium confidence: the documented built-package harness compares equivalent duplicate check-run inputs against the production digest and cache predicate, and the pre-fix raw cache-key paths shown by the PR explain the miss on current main. This read-only review did not execute the harness itself. Is this the best way to solve the issue? Yes. Normalizing only the cache identity is the narrowest maintainable repair because it preserves full check context for reviewers, keeps truncation/completeness behavior intact, and retains distinct check names, apps, statuses, and conclusions as invalidating signals. 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:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (4 earlier review cycles)
|
|
I filed #967 and had a narrower version of this in flight; yours is the better boundary, so I closed mine. Two tests I had that yours doesn't cover, if they're useful — both guard the identity key rather than the de-duplication itself. I checked them against your branch at To confirm they earn their place, I narrowed your identity key to name-only: - const identity = stableJsonCodeUnit(checkRun);
+ const identity = stableJsonCodeUnit((checkRun as Record<string, unknown>)?.name);Your three current tests still pass under that change — a regression that widens what counts as "the same run" lands silently without these two. test("content digest keeps check runs that differ only in name or app", () => {
const pull = item({ kind: "pull_request", number: 200 });
const check = { name: "smoke", status: "completed", conclusion: "success", app: "github-actions" };
const digest = (checkRuns: unknown[]) =>
itemContentDigestForTest(
pull,
pullContext({
pullChecks: {
complete: true,
checkRuns,
checkRunsTruncated: false,
statuses: [],
statusesTruncated: false,
},
}),
);
const alone = digest([check]);
const byName = digest([check, { ...check, name: "smoke (windows)" }]);
const byApp = digest([check, { ...check, app: "blacksmith-sh" }]);
assert.notEqual(alone, byName);
assert.notEqual(alone, byApp);
assert.notEqual(byName, byApp);
});
test("content digest busts when one of several repeated check runs newly fails", () => {
const pull = item({ kind: "pull_request", number: 200 });
const notify = {
name: "notify",
status: "completed",
conclusion: "success",
app: "github-actions",
};
const digest = (checkRuns: unknown[]) =>
itemContentDigestForTest(
pull,
pullContext({
pullChecks: {
complete: true,
checkRuns,
checkRunsTruncated: false,
statuses: [],
statusesTruncated: false,
},
}),
);
const allPassing = digest([notify, notify, notify]);
const oneFailing = digest([notify, notify, { ...notify, conclusion: "failure" }]);
assert.notEqual(allPassing, oneFailing);
});The second one matters for the case that started this: the head I measured carried 13 identical I also wrote a third, asserting a checks payload with no One thing your version handles that mine didn't: extracting the helper into a shared module so all three cache identities can use it. I kept mine to the content digest specifically to avoid that relocation, which left the semantic and structural digests still churning. That was the wrong call. Take or ignore these freely — no need to credit. |
clawsweeper
Bot
added
rating: 🦞 diamond lobster
db44c8a to
0a3d5c5
Compare
|
Thank you — both tests are excellent guardrails, and I added them in I repeated your mutation proof locally: narrowing the identity to name-only makes exactly those two tests fail (38/40), while restoring the full-object identity returns the content-cache suite to 40/40. I agree the non-list pass-through case did not earn a test without a mutation it could catch. I also rebased the branch onto current |
clawsweeper
Bot
added
rating: 🦞 diamond lobster
3 participants
Closes #967
What Problem This Solves
Fixes an issue where contributors with an unchanged pull-request head could receive repeated full ClawSweeper reviews after equivalent GitHub Actions check runs accumulated on that head. Those unnecessary cycles consume review capacity and can expose contributors to different model verdicts despite no change to their code or discussion.
Why This Change Was Made
Cache-key construction now de-duplicates equivalent compacted check-run tuples across the structural, semantic, and content review caches. The full check context supplied to the reviewer is deliberately unchanged; distinct names, apps, statuses, or conclusions still invalidate the relevant caches, and existing completeness/truncation safeguards remain intact.
This keeps the repair at the cache identity boundary requested in #967 rather than introducing a workflow-name allowlist or hiding repeated runs from review context.
User Impact
An unchanged PR head no longer receives another full review solely because automation appended another equivalent check run. Meaningful CI changes—such as a previously successful check failing—continue to trigger fresh evaluation.
Evidence
{name,status,conclusion,app}duplicate leaves the content digest unchanged.mainbefore the fix: the two equivalent contexts produced different content digests.mainatc8f44886fde43c44187a2f239dc0623fdaa279de.pnpm run check:static,pnpm run build:all, andpnpm run lintpass on Node 26.5.0 / pnpm 11.10.0.pnpm run checkcompletes all changed-surface validation; its five remaining failures are in repair files unchanged fromorigin/mainand are Linux Landlock/capability tests running on macOS (capsetis absent from Darwin libc). Hosted Linux CI is the authoritative full-suite result.Real Behavior Proof
Claim: Adding another equivalent compacted check run to an unchanged PR head preserves the review cache identity, while a distinct failure still invalidates it.
Exercised surface: The production
itemContentDigestForTestexport andreviewContentCacheHitpredicate from the built package.Scenario: The fixture keeps the PR source revision, head, base, files, timeline, and check outcome fixed. It compares one successful
notifyrun with two byte-equivalent successfulnotifyruns, then changes the second run's conclusion tofailure.Command and environment: A Node 26.5.0 ESM harness against
pnpm run buildoutput on macOS 15.Observed result:
{ "priorDigest": "aadae6eeaf1ce3ecfc7c0ef8eeeb72b8d1c18c6c693ff9fd38268154e260cbf5", "repeatedDigest": "aadae6eeaf1ce3ecfc7c0ef8eeeb72b8d1c18c6c693ff9fd38268154e260cbf5", "duplicateStable": true, "distinctFailureInvalidates": true, "cacheHit": true }Trace: The executable fixture is captured as the focused regression in
test/review-content-cache.test.ts; semantic identity is independently covered intest/review-semantic-cache.test.ts.Limits: This does not alter check-run collection, the reviewer-visible context, the 100-run completeness boundary, scheduler cadence, explicit re-review commands, or maintainer-request behavior.
AI Assistance
This PR was implemented and reviewed with OpenAI Codex assistance. The author ran and inspected the reported validation and remains responsible for the change.