Skip to content

fix(review): stop ClawSweeper's own check runs from expiring the review cache - #971

Closed
masatohoshino wants to merge 1 commit into
openclaw:mainfrom
masatohoshino:fix/review-digest-check-churn
Closed

fix(review): stop ClawSweeper's own check runs from expiring the review cache#971
masatohoshino wants to merge 1 commit into
openclaw:mainfrom
masatohoshino:fix/review-digest-check-churn

Conversation

@masatohoshino

Copy link
Copy Markdown
Contributor

Fixes #967.

What this fixes

itemContentDigest fed context.pullChecks into the review content digest
verbatim, and pullChecksContext fills that from
commits/{headSha}/check-runs?per_page=100 with no author or app filter — so it
includes ClawSweeper's own runs. That closes a loop:

  1. a review completes and writes its labels and durable comment
  2. clawsweeper-dispatch.yml (job dispatch) and github-activity.yml (job
    notify) both trigger on labeled / unlabeled / issue_comment
  3. they add check runs to the PR head
  4. pullChecks changes, so the digest changes
  5. reviewContentCacheHit misses
  6. the unchanged head is re-reviewed — which writes labels again, back to step 1

On #948's head 144236be, 26 of 38 check runs were those two jobs: 13 notify
and 13 dispatch. After compactCheckRun reduces 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 — timeline via
CLAWSWEEPER_BOT_AUTHORS, labels via isIgnorableSourceRevisionLabel, PR review
comments via isClawSweeperComment, each with its own test in
test/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 conclusion and keeps its own entry.

The change is confined to the content-cache key. pullChecksContext is
untouched, so the check list the reviewer is shown through reviewContextLedger is
unchanged.

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 pullChecks raw
(src/review-semantic-cache.ts:785, src/clawsweeper.ts:9168), so an unchanged
head 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, base origin/main 3dc70e67. Both captures drive the exported
itemContentDigestForTest and reviewContentCacheHit with the check-run list
actually observed on 144236be (38 runs, 26 of them dispatch/notify).

Before this change:

=== A. Does ClawSweeper's own reaction churn change the digest? ===

  OK    one more bot reaction run changes the digest               1092d87e96d999aa -> fd84f307765acbe6
  OK    the added entry is a duplicate of an existing one          13 identical {notify,completed,success} entries already present

=== B. The same bot is filtered out of every sibling digest input ===

  OK    bot timeline entry does NOT change the digest              filtered by CLAWSWEEPER_BOT_AUTHORS
  OK    bot advisory label churn does NOT change the digest        filtered by isIgnorableSourceRevisionLabel
  OK    bot PR review comment does NOT change the digest           filtered by isClawSweeperComment
  OK    bot check run DOES change the digest                       pullChecks has no bot filter  <-- the asymmetry

=== E. Effect on the production cache predicate ===

  OK    cache MISSES after only bot reaction churn -> full re-review reviewContentCacheHit = false

After:

=== 1. The loop is broken: bot reaction churn no longer expires the cache ===

  OK    one more bot reaction run does NOT change the digest         ffbb57f8c75c29b8 == ffbb57f8c75c29b8
  OK    ten more bot reaction runs do NOT change the digest          ffbb57f8c75c29b8 == ffbb57f8c75c29b8

=== 2. Genuine CI signal is preserved ===

  OK    pnpm check success -> failure still changes the digest       ffbb57f8c75c29b8 -> 211e2afa4f4fb026
  OK    a bot run that newly fails still changes the digest          distinct conclusion keeps its own entry
  OK    a newly added distinct CI check still changes the digest     distinct name keeps its own entry
  OK    the same check name from a different app still changes the digest distinct app keeps its own entry

=== 3. Non-list check payloads are untouched ===

  OK    payload without a checkRuns array is passed through unchanged truncation state still distinguishes

=== 4. Production cache predicate ===

  OK    cache HITS after only bot reaction churn (no re-review)      reviewContentCacheHit = true
  OK    cache still MISSES when a real check newly fails             reviewContentCacheHit = false

The discriminator is section 4: over the same inputs, reviewContentCacheHit goes
falsetrue for pure bot churn while staying false for a real failing check.

Scale

The durable review comments record prior cycles as
- reviewed <ts> sha <sha> :: <verdict>, so the rate is public:

repo=openclaw/clawsweeper
for n in $(gh pr list --repo "$repo" --state all --limit 400 --json number --jq '.[].number'); do
  gh api "repos/$repo/issues/$n/comments" --paginate \
    --jq '.[] | select(.user.login=="clawsweeper[bot]") | .body' 2>/dev/null |
    grep '^- reviewed ' | sed "s|^|$n |"
done | python3 -c '
import sys, collections, datetime as dt, statistics
by=collections.defaultdict(list)
for line in sys.stdin:
    head, _, rest = line.rstrip("\n").partition(" :: ")
    verdict = rest.split(" :: ")[0]
    f = head.split()
    if len(f) < 6: continue
    by[f[0]].append((f[3], f[5], verdict))
gaps=[]; pairs=0; flips=0
for pr, rows in by.items():
    rows.sort()
    for (t1,s1,v1),(t2,s2,v2) in zip(rows, rows[1:]):
        if s1 != s2: continue            # head changed -> a re-review is expected
        pairs += 1
        if v1 != v2: flips += 1
        a=dt.datetime.fromisoformat(t1.replace("Z","+00:00"))
        b=dt.datetime.fromisoformat(t2.replace("Z","+00:00"))
        gaps.append((b-a).total_seconds()/60)
print(f"consecutive re-reviews of an unchanged head: {pairs}")
print(f"median gap: {statistics.median(gaps):.0f} min")
print(f"under 90 min: {sum(1 for g in gaps if g<90)}")
print(f"recorded verdict changed: {flips} ({100*flips/pairs:.0f}%)")
'
consecutive re-reviews of an unchanged head: 109
median gap: 42 min
under 90 min: 79
recorded verdict changed: 16 (15%)

reviewCadenceMs never returns less than HOURLY_REVIEW_MS, so most of those are
below the intended floor. The same command against openclaw/openclaw gives 145
pairs, 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 existing
bot-exclusion tests:

  • repeated ClawSweeper automation runs no longer change the digest
  • a repeated run that newly fails still changes it
  • runs differing only in name or in app stay distinct (no over-collapsing)
  • a checks payload without a checkRuns array is passed through untouched

Reverting the one-line call site in itemContentDigest fails exactly the first of
those and nothing else. The test imports ../dist/clawsweeper.js, so each source
edit is followed by a rebuild:

### WITH the fix
$ grep -n "checks: isPull" src/clawsweeper.ts
3207:      checks: isPull ? reviewCheckDigestParts(context.pullChecks) : null,
$ pnpm run build && node test/review-content-cache.test.ts
ℹ tests 40
ℹ pass 40
ℹ fail 0

### revert ONLY the itemContentDigest call site, then rebuild
$ grep -n "checks: isPull" src/clawsweeper.ts
3207:      checks: isPull ? (context.pullChecks ?? null) : null,
$ pnpm run build && node test/review-content-cache.test.ts
✖ content digest ignores repeated ClawSweeper automation check runs (0.823594ms)
ℹ tests 40
ℹ pass 39
ℹ fail 1

### restore, then rebuild
$ pnpm run build && node test/review-content-cache.test.ts
ℹ tests 40
ℹ pass 40
ℹ fail 0

The existing content digest busts when bounded PR check state changes passes
unchanged.

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 itemContentDigest and
    reviewContentCacheHit, driven with the real check-run list from a real head;
    plus the public review-history measurement above.

  • Exact commands run after this patch:

    pnpm run build
    node test/review-content-cache.test.ts
    node digest-loop-after-fix.mjs
  • Observed result after fix: pass 40 / fail 0, and reviewContentCacheHit
    returns true for pure bot churn and false for 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 lint exits 0. pnpm run check runs 2,748 tests with 6 failures, all
    host-caused and all in test/repair/target-validation.test.ts, which this patch
    does not touch: three need git worktree list -z (this host has Git 2.34.1, -z
    arrived in 2.36) and three cannot resolve the bun/npm/pnpm containment fixtures.

Not in scope

  • The semantic and structural digests still read pullChecks raw
    (src/review-semantic-cache.ts:785, src/clawsweeper.ts:9168). The same
    de-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.
  • Changing the digest invalidates existing content-cache entries once. Every
    item takes one extra review on first encounter after deploy, then settles.
  • Matrix jobs whose compacted tuples collide will collapse. compactCheckRun
    keeps only {name,status,conclusion,app}, and some names are the unexpanded
    template (containment smoke (sample ${{ matrix.sample }})), so shards that
    agree 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.
  • pullChecksContext is unchanged. De-duplicating there would also shrink the
    noise in the review prompt, but it would change reviewer input rather than only
    the cache key, so it belongs in its own change.
  • Filtering by bot identity was considered and rejected: app.slug is
    github-actions for both the dispatch/notify runs and genuine CI, so it
    would need a workflow-name allowlist that drifts as workflows are renamed.
  • checkRunsTruncated above 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.
  • 30 same-head re-review pairs on this repo are under 15 minutes, faster than this
    loop alone explains. That looks like a separate lane or lease issue and is not
    addressed here.
  • Whether two reviews over identical input agree is a separate question from how
    often they are asked to. This change reduces how often.

Note on #589

#589 adds pullBaseDriftDigestParts for the same reason on a different digest
input ("Excluding raw age avoids daily cache churn"). Both PRs add a helper next to
reviewTimelineDigestParts and a field to itemContentDigest, so whichever lands
second 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.

// After-fix proof: does de-duplicating compacted check runs break the re-review loop
// without losing genuine CI signal?
//
// Copy into a clawsweeper checkout and run after `pnpm run build`:
//   node digest-loop-after-fix.mjs
//
// Drives the production digest function (itemContentDigestForTest) and the production
// cache predicate (reviewContentCacheHit). The check-run list is the one actually
// observed on openclaw/clawsweeper#948 head 144236be: 38 runs, of which 26 are the
// dispatch/notify runs ClawSweeper's own review writes retrigger.

import { itemContentDigestForTest } from "./dist/clawsweeper.js";
import { reviewContentCacheHit } from "./dist/scheduler-policy.js";

const pull = {
  repo: "openclaw/clawsweeper",
  number: 948,
  kind: "pull_request",
  title: "fix(ci): build the main bundle in the jobs that publish the action ledger",
  url: "https://github.com/openclaw/clawsweeper/pull/948",
  createdAt: "2026-07-30T00:21:54Z",
  updatedAt: "2026-07-30T12:24:06Z",
  author: "masatohoshino",
  authorAssociation: "NONE",
  labels: [],
};

// Exactly the shape compactCheckRun() produces.
const ck = (name, conclusion, app = "github-actions") => ({
  name,
  status: "completed",
  conclusion,
  app,
});

const REAL_CI = [
  ck("[code]smith", "skipped", "blacksmith-sh"),
  ck("containment smoke (sample ${{ matrix.sample }})", "skipped"),
  ck("production automerge flow", "skipped"),
  ck("Analyze (actions)", "success"),
  ck("Analyze (javascript-typescript)", "success"),
  ck("Windows Codex launcher", "success"),
  ck("crawl-remote toolchain integration", "success"),
  ck("pnpm check", "success"),
  ck("sparse repair build smoke", "success"),
  ck("CodeQL", "success", "github-advanced-security"),
  ck("Socket Security: Project Report", "success", "socket-security"),
  ck("Socket Security: Pull Request Alerts", "success", "socket-security"),
];

const botRuns = (notify, dispatch) => [
  ...Array.from({ length: notify }, () => ck("notify", "success")),
  ...Array.from({ length: dispatch }, () => ck("dispatch", "skipped")),
  ck("dispatch", "success"),
];

const checks = (runs) => ({
  complete: true,
  checkRuns: [...REAL_CI, ...runs].sort((a, b) =>
    JSON.stringify(a).localeCompare(JSON.stringify(b)),
  ),
  checkRunsTruncated: false,
  statuses: [],
  statusesTruncated: false,
});

const ctx = (overrides = {}) => ({
  sourceRevision: "source-rev-fixed",
  timeline: [],
  counts: { comments: 0, timeline: 0 },
  pullRequest: {
    head: { sha: "144236be58ce023f2dd92d3da3530b186ce0c39b" },
    base: { sha: "8365a79af804737982bc0fca4465069ead86e481" },
    draft: false,
    mergeable: true,
    mergeableState: "blocked",
    additions: 128,
    deletions: 4,
    changedFiles: 5,
  },
  pullFiles: [{ filename: ".github/workflows/repair-comment-router.yml", status: "modified" }],
  pullReviewComments: [],
  pullChecks: checks(botRuns(13, 12)),
  ...overrides,
});

const digest = (o) => itemContentDigestForTest(pull, ctx(o));
const short = (d) => d.slice(0, 16);

let failures = 0;
const expect = (label, cond, detail) => {
  if (!cond) failures += 1;
  console.log(`  ${cond ? "OK  " : "FAIL"}  ${label.padEnd(60)} ${detail}`);
};

console.log("\n=== 1. The loop is broken: bot reaction churn no longer expires the cache ===\n");

// The head as observed after three review cycles.
const observed = digest({ pullChecks: checks(botRuns(13, 12)) });
// One more review cycle fires notify + dispatch again. Nothing else changes.
const afterOneMoreReview = digest({ pullChecks: checks(botRuns(14, 13)) });
// Ten more cycles.
const afterTenMore = digest({ pullChecks: checks(botRuns(23, 22)) });

expect(
  "one more bot reaction run does NOT change the digest",
  observed === afterOneMoreReview,
  `${short(observed)} == ${short(afterOneMoreReview)}`,
);
expect(
  "ten more bot reaction runs do NOT change the digest",
  observed === afterTenMore,
  `${short(observed)} == ${short(afterTenMore)}`,
);

console.log("\n=== 2. Genuine CI signal is preserved ===\n");

const flip = (name, conclusion) => {
  const c = checks(botRuns(13, 12));
  return digest({
    pullChecks: { ...c, checkRuns: c.checkRuns.map((r) => (r.name === name ? { ...r, conclusion } : r)) },
  });
};
expect(
  "pnpm check success -> failure still changes the digest",
  observed !== flip("pnpm check", "failure"),
  `${short(observed)} -> ${short(flip("pnpm check", "failure"))}`,
);
expect(
  "a bot run that newly fails still changes the digest",
  observed !== digest({ pullChecks: checks([...botRuns(12, 12), ck("notify", "failure")]) }),
  "distinct conclusion keeps its own entry",
);
expect(
  "a newly added distinct CI check still changes the digest",
  observed !== digest({ pullChecks: checks([...botRuns(13, 12), ck("new required gate", "success")]) }),
  "distinct name keeps its own entry",
);
expect(
  "the same check name from a different app still changes the digest",
  observed !== digest({ pullChecks: checks([...botRuns(13, 12), ck("pnpm check", "success", "other-app")]) }),
  "distinct app keeps its own entry",
);

console.log("\n=== 3. Non-list check payloads are untouched ===\n");

const truncatedA = digest({ pullChecks: { complete: false, checkRunsTruncated: true } });
const truncatedB = digest({ pullChecks: { complete: false, checkRunsTruncated: false } });
expect(
  "payload without a checkRuns array is passed through unchanged",
  truncatedA !== truncatedB,
  "truncation state still distinguishes",
);

console.log("\n=== 4. Production cache predicate ===\n");

const review = {
  reviewStatus: "complete",
  reviewPolicy: "policy-1",
  decision: "keep_open",
  lastFullReviewDecision: "keep_open",
  contentDigest: observed,
  lastFullReviewAt: "2026-07-30T11:11:49Z",
};
const hitAfterBotChurn = reviewContentCacheHit({
  review,
  reviewPolicy: "policy-1",
  contentDigest: afterOneMoreReview,
  now: Date.parse("2026-07-30T12:22:25Z"),
  explicitDispatch: false,
  maintainerRequest: false,
});
const hitAfterRealChange = reviewContentCacheHit({
  review,
  reviewPolicy: "policy-1",
  contentDigest: flip("pnpm check", "failure"),
  now: Date.parse("2026-07-30T12:22:25Z"),
  explicitDispatch: false,
  maintainerRequest: false,
});
expect(
  "cache HITS after only bot reaction churn (no re-review)",
  hitAfterBotChurn === true,
  `reviewContentCacheHit = ${hitAfterBotChurn}`,
);
expect(
  "cache still MISSES when a real check newly fails",
  hitAfterRealChange === false,
  `reviewContentCacheHit = ${hitAfterRealChange}`,
);

console.log(`\n${failures === 0 ? "ALL EXPECTATIONS HELD" : `${failures} EXPECTATION(S) FAILED`}\n`);
process.exit(failures === 0 ? 0 : 1);

…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.
@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. 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 30, 2026
@clawsweeper

clawsweeper Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 30, 2026, 6:21 PM ET / 22:21 UTC.

ClawSweeper review

What this changes

The 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 readiness

⚠️ Ready for maintainer review - 1 item remains

This 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 main; no discrete patch defect is established from the supplied diff, but maintainers should review the intentional loss of identical check-run multiplicity before merge.

Priority: P2
Reviewed head: 7065c5799309aa545c6b346928d57e1fd5b36b8a

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A narrow and well-supported automation repair with focused after-fix evidence; remaining review is the intentional cache-key trade-off rather than a demonstrated patch defect.
Proof confidence 🐚 platinum hermit (4/6) Sufficient (live_output): The PR body includes after-fix terminal output from the production digest and cache predicate, driven with observed check-run-shaped data, showing duplicate churn now hits the cache while a newly failing check still misses.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (live_output): The PR body includes after-fix terminal output from the production digest and cache predicate, driven with observed check-run-shaped data, showing duplicate churn now hits the cache while a newly failing check still misses.
Evidence reviewed 3 items Narrow cache-key change: The submitted patch replaces raw PR check input in itemContentDigest with a duplicate-free representation, limiting the behavioral change to the content-cache key.
Focused regression coverage: The submitted tests cover repeated automation runs, a repeated run whose conclusion changes to failure, distinct names or apps, and check payloads without an array.
Linked canonical report: The provided PR context explicitly identifies the open canonical issue as the behavior this branch fixes, so the issue should remain open until a viable fix lands.
Findings None None.
Security None None.

How this fits together

ClawSweeper 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]
Loading

Before merge

  • Resolve merge risk (P1) - Identical compact check-run tuples are intentionally treated as non-semantic, so adding or removing indistinguishable matrix shards will no longer invalidate the content cache; changed conclusions, names, and apps remain distinct.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Patch scope 2 files affected; 101 added, 1 removed The branch is limited to one cache-key helper and focused regression coverage.
Regression coverage 4 tests added The tests exercise duplicate churn, failure changes, tuple distinctions, and malformed/non-list check payloads.

Merge-risk options

Maintainer options:

  1. Merge with focused cache coverage (recommended)
    Accept duplicate compact check-run tuples as non-semantic cache input while retaining the tests that preserve distinct conclusions, names, apps, and non-list payloads.
  2. Pause for wider cache normalization
    Keep this PR open if maintainers want semantic and structural cache keys to receive the same normalization before accepting a content-cache-only change.

Technical review

Best 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.

Labels

Label changes:

  • add P2: This is a bounded automation-cache defect that causes redundant reviews but has no evidence of an immediate user-facing outage.
  • add merge-risk: 🚨 automation: The patch changes the cache key that controls whether ClawSweeper performs a full review.
  • add proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-fix terminal output from the production digest and cache predicate, driven with observed check-run-shaped data, showing duplicate churn now hits the cache while a newly failing check still misses.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body includes after-fix terminal output from the production digest and cache predicate, driven with observed check-run-shaped data, showing duplicate churn now hits the cache while a newly failing check still misses.

Label justifications:

  • P2: This is a bounded automation-cache defect that causes redundant reviews but has no evidence of an immediate user-facing outage.
  • merge-risk: 🚨 automation: The patch changes the cache key that controls whether ClawSweeper performs a full review.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body includes after-fix terminal output from the production digest and cache predicate, driven with observed check-run-shaped data, showing duplicate churn now hits the cache while a newly failing check still misses.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-fix terminal output from the production digest and cache predicate, driven with observed check-run-shaped data, showing duplicate churn now hits the cache while a newly failing check still misses.

Evidence

What I checked:

  • Narrow cache-key change: The submitted patch replaces raw PR check input in itemContentDigest with a duplicate-free representation, limiting the behavioral change to the content-cache key. (src/clawsweeper.ts:3207, 7065c5799309)
  • Focused regression coverage: The submitted tests cover repeated automation runs, a repeated run whose conclusion changes to failure, distinct names or apps, and check payloads without an array. (test/review-content-cache.test.ts:297, 7065c5799309)
  • Linked canonical report: The provided PR context explicitly identifies the open canonical issue as the behavior this branch fixes, so the issue should remain open until a viable fix lands.

Likely related people:

  • masatohoshino: The supplied context attributes the current focused cache-change commit and accompanying reproduction harness to this contributor; current-main ownership history could not be independently read during this review. (role: recent area contributor; confidence: low; commits: 7065c5799309; files: src/clawsweeper.ts, test/review-content-cache.test.ts)

Rank-up moves

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

  • Obtain a fresh ClawSweeper review if the PR body or head changes after the current in-progress checks finish.

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.

@masatohoshino

Copy link
Copy Markdown
Contributor Author

Superseded by #970, which lands the same normalization across all three cache identities rather than only the content digest. That is the better boundary — I kept mine to the content digest to avoid relocating the helper, and #970 solves that by extracting a shared module. Closing in favour of it.

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. 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ClawSweeper's own review writes expire the review content cache, so unchanged heads are re-reviewed in a loop

1 participant