From e97efce048f4fa710d397b09d2f03aaa9a17dc49 Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Fri, 31 Jul 2026 21:41:57 +0200 Subject: [PATCH 1/2] fix(review): classify complete pull request files --- src/clawsweeper-types.ts | 2 + src/clawsweeper.ts | 78 ++++++++++++++++++++++++++++-- test/context.test.ts | 46 ++++++++++++++++++ test/pr-surface-policy.test.ts | 44 +++++++++++++++++ test/review-prompt-context.test.ts | 41 ++++++++++++++++ 5 files changed, 206 insertions(+), 5 deletions(-) diff --git a/src/clawsweeper-types.ts b/src/clawsweeper-types.ts index 06bddaf223..f922faa113 100644 --- a/src/clawsweeper-types.ts +++ b/src/clawsweeper-types.ts @@ -8,6 +8,7 @@ import type { SchedulerDueCandidate } from "./scheduler-policy.js"; /** Shared ClawSweeper domain, review, scheduling, and dashboard shapes. */ export const completeActivityContextSymbol = Symbol("completeActivityContext"); +export const completePullFilesSymbol = Symbol("completePullFiles"); export type ItemKind = "issue" | "pull_request"; export type ApplyKind = ItemKind | "all"; @@ -535,6 +536,7 @@ export interface CompleteActivityContext { export interface ItemContext { [completeActivityContextSymbol]?: CompleteActivityContext; + [completePullFilesSymbol]?: readonly unknown[]; issue: unknown; comments: unknown[]; timeline: unknown[]; diff --git a/src/clawsweeper.ts b/src/clawsweeper.ts index f017650a67..8f17332c8a 100644 --- a/src/clawsweeper.ts +++ b/src/clawsweeper.ts @@ -234,7 +234,7 @@ import { themedRatingName, } from "./clawsweeper-rating.js"; -import { completeActivityContextSymbol } from "./clawsweeper-types.js"; +import { completeActivityContextSymbol, completePullFilesSymbol } from "./clawsweeper-types.js"; import type { AcquiredReviewStartLease, Action, @@ -4473,6 +4473,40 @@ function compactSemanticPullFile(value: unknown): unknown { }; } +function completePullFilesForDeterministicReview(options: { + repo: string; + total: number; + truncated: boolean; + fetchAll: () => unknown[]; +}): unknown[] | null { + if (options.repo !== "openclaw/openclaw" || !options.truncated) return null; + try { + const files = options.fetchAll(); + return files.length === options.total ? files.map(compactSemanticPullFile) : null; + } catch { + // Keep the bounded window usable; deterministic classifiers retain the unknown marker. + return null; + } +} + +export function completePullFilesForDeterministicReviewForTest(options: { + repo?: string; + total: number; + truncated: boolean; + files: unknown[]; + fetchError?: Error; +}): unknown[] | null { + return completePullFilesForDeterministicReview({ + repo: options.repo ?? "openclaw/openclaw", + total: options.total, + truncated: options.truncated, + fetchAll: () => { + if (options.fetchError) throw options.fetchError; + return options.files; + }, + }); +} + function normalizedPullFileStatus(value: unknown): string { const status = typeof value === "string" ? value.trim().toLowerCase() : ""; if (status === "m" || status === "modified" || status === "changed") return "modified"; @@ -6569,6 +6603,12 @@ function collectItemContext( 80, ); const pullFiles = pullFilesWindow.items; + const completePullFiles = completePullFilesForDeterministicReview({ + repo: targetRepo(), + total: pullFilesWindow.total, + truncated: pullFilesWindow.truncated, + fetchAll: () => ghPaged(`repos/${targetRepo()}/pulls/${item.number}/files`), + }); const pullCommitsWindow = ghPagedContextWindow( `repos/${targetRepo()}/pulls/${item.number}/commits`, pullRecord.commits, @@ -6599,6 +6639,12 @@ function collectItemContext( fullPullReviewComments.length >= pullReviewCommentsWindow.total; context.pullRequest = compactPullRequest(pullRequest); context.pullFiles = compactMappedWindow(pullFiles, pullFilesWindow.total, 80, compactPullFile); + if (completePullFiles) { + Object.defineProperty(context, completePullFilesSymbol, { + value: completePullFiles, + enumerable: false, + }); + } context.semanticPullFiles = options.reviewCacheDigest && options.reviewCacheGitDir && @@ -10724,13 +10770,28 @@ function pullRequestFilePathsFromReport(markdown: string): string[] { return frontMatterStringArray(markdown, "pull_files"); } +function deterministicPullFilesFromContext(context: ItemContext): { + complete: boolean; + files: readonly unknown[]; +} { + const completePullFiles = context[completePullFilesSymbol]; + if (context.counts?.pullFilesTruncated && completePullFiles) { + return { complete: true, files: completePullFiles }; + } + return { + complete: context.counts?.pullFilesTruncated !== true, + files: context.pullFiles ?? [], + }; +} + function configSurfaceChangeFromContext(repo: string, context: ItemContext): ConfigSurfaceChange { if (repo !== "openclaw/openclaw") { return { change: false, keys: [] }; } const keys = new Set(); - for (const entry of context.pullFiles ?? []) { + const deterministicPullFiles = deterministicPullFilesFromContext(context); + for (const entry of deterministicPullFiles.files) { const file = asRecord(entry); const path = typeof file.filename === "string" ? file.filename.trim() : ""; const previousPath = @@ -10760,7 +10821,7 @@ function configSurfaceChangeFromContext(repo: string, context: ItemContext): Con } } - if (context.counts?.pullFilesTruncated) { + if (!deterministicPullFiles.complete) { keys.add("unknown-truncated-pull-files"); } @@ -10768,6 +10829,7 @@ function configSurfaceChangeFromContext(repo: string, context: ItemContext): Con } export function configSurfaceChangeFromPullFilesForTest(options: { + completePullFiles?: readonly unknown[]; repo?: string; pullFiles?: unknown[]; pullFilesTruncated?: boolean; @@ -10782,6 +10844,8 @@ export function configSurfaceChangeFromPullFilesForTest(options: { counts, }; if (options.pullFiles !== undefined) context.pullFiles = options.pullFiles; + if (options.completePullFiles !== undefined) + context[completePullFilesSymbol] = options.completePullFiles; return configSurfaceChangeFromContext(options.repo ?? "openclaw/openclaw", context); } @@ -10791,7 +10855,8 @@ function dataModelChangeFromContext(repo: string, context: ItemContext): DataMod } const surfaces = new Set(); - for (const entry of context.pullFiles ?? []) { + const deterministicPullFiles = deterministicPullFilesFromContext(context); + for (const entry of deterministicPullFiles.files) { const file = asRecord(entry); const path = typeof file.filename === "string" ? file.filename.trim() : ""; const previousPath = @@ -10823,7 +10888,7 @@ function dataModelChangeFromContext(repo: string, context: ItemContext): DataMod } } - if (context.counts?.pullFilesTruncated) { + if (!deterministicPullFiles.complete) { surfaces.add("unknown-truncated-pull-files"); } @@ -10831,6 +10896,7 @@ function dataModelChangeFromContext(repo: string, context: ItemContext): DataMod } export function dataModelChangeFromPullFilesForTest(options: { + completePullFiles?: readonly unknown[]; repo?: string; pullFiles?: unknown[]; pullFilesTruncated?: boolean; @@ -10845,6 +10911,8 @@ export function dataModelChangeFromPullFilesForTest(options: { counts, }; if (options.pullFiles !== undefined) context.pullFiles = options.pullFiles; + if (options.completePullFiles !== undefined) + context[completePullFilesSymbol] = options.completePullFiles; return dataModelChangeFromContext(options.repo ?? "openclaw/openclaw", context); } diff --git a/test/context.test.ts b/test/context.test.ts index 719faa3a65..108ec5f8e1 100644 --- a/test/context.test.ts +++ b/test/context.test.ts @@ -6,6 +6,7 @@ import { assistPromptContextForTest, compactMappedSlice, compactMappedWindow, + completePullFilesForDeterministicReviewForTest, extractLatestClawSweeperReviewForTest, extractLatestClawSweeperReviewFromHydrationForTest, filterReviewContextCommentsForTest, @@ -109,6 +110,51 @@ test("compactMappedWindow keeps bounded hydrated context when total is larger th assert.deepEqual(mapped, [1, 2, 99, 100]); }); +test("complete pull-file hydration requires the exact reported count", () => { + const files = Array.from({ length: 118 }, (_, index) => ({ + filename: `src/file-${index}.ts`, + patch: `+export const value${index} = ${index};`, + })); + + assert.equal( + completePullFilesForDeterministicReviewForTest({ + total: 118, + truncated: true, + files: files.slice(0, 117), + }), + null, + ); + assert.equal( + completePullFilesForDeterministicReviewForTest({ + repo: "openclaw/clawhub", + total: 118, + truncated: true, + files, + }), + null, + ); + assert.equal( + completePullFilesForDeterministicReviewForTest({ + total: 118, + truncated: true, + files, + })?.length, + 118, + ); +}); + +test("complete pull-file hydration fails closed when the full fetch throws", () => { + assert.equal( + completePullFilesForDeterministicReviewForTest({ + total: 118, + truncated: true, + files: [], + fetchError: new Error("GitHub pagination failed"), + }), + null, + ); +}); + function issueComment( id: number, body: string, diff --git a/test/pr-surface-policy.test.ts b/test/pr-surface-policy.test.ts index bb6ec0ebe8..44fce02350 100644 --- a/test/pr-surface-policy.test.ts +++ b/test/pr-surface-policy.test.ts @@ -238,6 +238,29 @@ test("config surface detector fails closed for truncated pull files", () => { }); }); +test("config surface detector uses complete files without a truncation marker", () => { + const completePullFiles = Array.from({ length: 118 }, (_, index) => ({ + filename: index === 59 ? "src/config/schema.ts" : `src/agents/generated/file-${index}.test.ts`, + patch: index === 59 ? "@@\n+ codeMode: z.boolean().optional()," : "@@\n+ const value = true;", + })); + const compactPullFiles = [ + ...completePullFiles.slice(0, 40), + { omitted: 38, note: "middle entries omitted from prompt context" }, + ...completePullFiles.slice(-40), + ]; + + const detection = configSurfaceChangeFromPullFilesForTest({ + completePullFiles, + pullFiles: compactPullFiles, + pullFilesTruncated: true, + }); + + assert.deepEqual(detection, { + change: true, + keys: ["codeMode"], + }); +}); + test("data model detector finds persistent schema and embedding metadata changes", () => { const detection = dataModelChangeFromPullFilesForTest({ pullFiles: [ @@ -347,6 +370,27 @@ test("data model detector fails closed for missing and truncated likely-surface }); }); +test("data model detector uses complete files without a truncation marker", () => { + const completePullFiles = Array.from({ length: 118 }, (_, index) => ({ + filename: index === 58 ? "packages/database/schema.sql" : `src/runtime/file-${index}.ts`, + patch: + index === 58 + ? "@@\n+ALTER TABLE sessions ADD COLUMN last_model TEXT;" + : "@@\n+export const value = true;", + })); + + const detection = dataModelChangeFromPullFilesForTest({ + completePullFiles, + pullFiles: [...completePullFiles.slice(0, 40), ...completePullFiles.slice(-40)], + pullFilesTruncated: true, + }); + + assert.deepEqual(detection, { + change: true, + surfaces: ["database schema: packages/database/schema.sql"], + }); +}); + test("data model reports force human review without migration proof", () => { const report = `${reportFrontMatter({ repository: "openclaw/openclaw", diff --git a/test/review-prompt-context.test.ts b/test/review-prompt-context.test.ts index 297912343b..854c387659 100644 --- a/test/review-prompt-context.test.ts +++ b/test/review-prompt-context.test.ts @@ -4,6 +4,7 @@ import test from "node:test"; import { compactPullRequestForTest, + itemContentDigestForTest, renderReviewContextBudgetForTest, reviewContextLedgerForTest, reviewDecisionSchemaText, @@ -11,6 +12,7 @@ import { reviewPromptTelemetryForTest, reviewPromptTemplate, } from "../dist/clawsweeper.js"; +import { completePullFilesSymbol } from "../dist/clawsweeper-types.js"; import { parseArgs as parseClawsweeperArgs } from "../dist/clawsweeper-args.js"; import { git, item } from "./helpers.ts"; @@ -211,3 +213,42 @@ test("review context ledger records ordered section budgets", () => { assert.match(renderReviewContextBudgetForTest(context), /- timeline events: 1\/1 hydrated/); assert.match(renderReviewContextBudgetForTest(context), /- previous ClawSweeper review: 1 entry/); }); + +test("review prompt, ledger, and content digest exclude complete deterministic pull files", () => { + const context = { + issue: { number: 123, title: "Large PR" }, + comments: [], + timeline: [], + pullRequest: { number: 123, additions: 120 }, + pullFiles: [{ filename: "src/first.ts", patch: "+first" }], + pullCommits: [], + pullReviewComments: [], + counts: { + comments: 0, + timeline: 0, + pullFiles: 118, + pullFilesHydrated: 80, + pullFilesTruncated: true, + }, + }; + const digestWithoutSidecar = itemContentDigestForTest( + item({ kind: "pull_request", number: 123 }), + context, + git, + ); + Object.defineProperty(context, completePullFilesSymbol, { + value: [{ filename: "src/private-middle.ts", patch: "+middle" }], + }); + + const prompt = reviewPromptForTest(item({ kind: "pull_request", number: 123 }), context, git); + const ledger = reviewContextLedgerForTest(context); + const digestWithSidecar = itemContentDigestForTest( + item({ kind: "pull_request", number: 123 }), + context, + git, + ); + + assert.doesNotMatch(prompt, /private-middle/); + assert.equal(ledger.find((entry) => entry.section === "pullFiles")?.entries, 1); + assert.equal(digestWithSidecar, digestWithoutSidecar); +}); From 9cb2073090e34c80e95d1d0659bf2bd2aced033e Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Fri, 31 Jul 2026 22:55:31 +0200 Subject: [PATCH 2/2] fix(review): stabilize complete pull file hydration --- CHANGELOG.md | 1 + src/clawsweeper.ts | 148 +++++++++++++++++++++++++++++++++++++++---- test/context.test.ts | 64 +++++++++++++++++++ 3 files changed, 201 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index edd353ad1b..822de70766 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ checkpoint, and status-only commits are intentionally omitted. ### Changed +- Large OpenClaw pull requests now reuse each paginated file-list page for bounded context and deterministic classification, accepting the complete sidecar only when filenames are unique and the PR base/head snapshot remains stable. - Event-review artifact publication completes as a superseded no-op when the reviewed branch vanished upstream (force-push or deletion) instead of failing the run. - Worker record requests now retry transient blank/invalid 2xx bodies from the edge within the bounded budget instead of failing hydration on the first occurrence. - Exact reviews of items that closed after enqueue now complete as superseded no-ops, and GitHub-throttled reservations defer as held retries — neither spends the item's review-failure budget. diff --git a/src/clawsweeper.ts b/src/clawsweeper.ts index 8f17332c8a..cafc55faa8 100644 --- a/src/clawsweeper.ts +++ b/src/clawsweeper.ts @@ -4473,30 +4473,118 @@ function compactSemanticPullFile(value: unknown): unknown { }; } +type PullFileSnapshot = { + baseSha: string; + headSha: string; +}; + +const GITHUB_PULL_FILES_MAX = 3000; + +function pullFileSnapshot(value: unknown): PullFileSnapshot | null { + const pull = asRecord(value); + const baseSha = stringOrUndefined(asRecord(pull.base).sha)?.toLowerCase() ?? ""; + const headSha = stringOrUndefined(asRecord(pull.head).sha)?.toLowerCase() ?? ""; + return /^[0-9a-f]{40}$/.test(baseSha) && /^[0-9a-f]{40}$/.test(headSha) + ? { baseSha, headSha } + : null; +} + function completePullFilesForDeterministicReview(options: { + initialPullRequest: unknown; repo: string; total: number; truncated: boolean; fetchAll: () => unknown[]; + fetchPullRequest: () => unknown; }): unknown[] | null { - if (options.repo !== "openclaw/openclaw" || !options.truncated) return null; + if ( + options.repo !== "openclaw/openclaw" || + !options.truncated || + options.total > GITHUB_PULL_FILES_MAX + ) { + return null; + } try { + const initialSnapshot = pullFileSnapshot(options.initialPullRequest); + if (!initialSnapshot) return null; const files = options.fetchAll(); - return files.length === options.total ? files.map(compactSemanticPullFile) : null; + if (files.length !== options.total) return null; + const filenames = files.map((file) => stringOrUndefined(asRecord(file).filename)?.trim() ?? ""); + if (filenames.some((filename) => !filename) || new Set(filenames).size !== filenames.length) { + return null; + } + const refreshedSnapshot = pullFileSnapshot(options.fetchPullRequest()); + if ( + !refreshedSnapshot || + refreshedSnapshot.baseSha !== initialSnapshot.baseSha || + refreshedSnapshot.headSha !== initialSnapshot.headSha + ) { + return null; + } + return files.map(compactSemanticPullFile); } catch { // Keep the bounded window usable; deterministic classifiers retain the unknown marker. return null; } } +function pullFileReviewHydration(options: { + repo: string; + path: string; + total: unknown; + pullRequest: unknown; + fetchPage: (path: string, page: number) => unknown[]; + fetchPullRequest: () => unknown; +}): { window: ContextHydration; completeFiles: unknown[] | null } { + const pages = new Map(); + const readPage = (path: string, page: number): unknown[] => { + const cached = pages.get(page); + if (cached) return cached; + const files = options.fetchPage(path, page); + pages.set(page, files); + return files; + }; + const window = ghPagedContextWindow(options.path, options.total, 80, { + page: readPage, + }); + const completeFiles = completePullFilesForDeterministicReview({ + initialPullRequest: options.pullRequest, + repo: options.repo, + total: window.total, + truncated: window.truncated, + fetchAll: () => { + const files: unknown[] = []; + const pageCount = Math.ceil(window.total / 100); + for (let page = 1; page <= pageCount; page += 1) { + const pageFiles = readPage(options.path, page); + files.push(...pageFiles); + if (pageFiles.length < 100) break; + } + return files; + }, + fetchPullRequest: options.fetchPullRequest, + }); + return { window, completeFiles }; +} + export function completePullFilesForDeterministicReviewForTest(options: { + initialBaseSha?: string; + initialHeadSha?: string; + refreshedBaseSha?: string; + refreshedHeadSha?: string; repo?: string; total: number; truncated: boolean; files: unknown[]; fetchError?: Error; }): unknown[] | null { + const initialBaseSha = options.initialBaseSha ?? "a".repeat(40); + const initialHeadSha = options.initialHeadSha ?? "b".repeat(40); return completePullFilesForDeterministicReview({ + initialPullRequest: { + base: { sha: initialBaseSha }, + head: { sha: initialHeadSha }, + }, repo: options.repo ?? "openclaw/openclaw", total: options.total, truncated: options.truncated, @@ -4504,9 +4592,44 @@ export function completePullFilesForDeterministicReviewForTest(options: { if (options.fetchError) throw options.fetchError; return options.files; }, + fetchPullRequest: () => ({ + base: { sha: options.refreshedBaseSha ?? initialBaseSha }, + head: { sha: options.refreshedHeadSha ?? initialHeadSha }, + }), }); } +export function pullFileReviewHydrationForTest(options: { + total: number; + pages: readonly unknown[][]; +}): { + completeFiles: unknown[] | null; + hydrated: number; + pageCalls: number[]; +} { + const pageCalls: number[] = []; + const pullRequest = { + base: { sha: "a".repeat(40) }, + head: { sha: "b".repeat(40) }, + }; + const result = pullFileReviewHydration({ + repo: "openclaw/openclaw", + path: "repos/openclaw/openclaw/pulls/123/files", + total: options.total, + pullRequest, + fetchPage: (_path, page) => { + pageCalls.push(page); + return [...(options.pages[page - 1] ?? [])]; + }, + fetchPullRequest: () => pullRequest, + }); + return { + completeFiles: result.completeFiles, + hydrated: result.window.hydrated, + pageCalls, + }; +} + function normalizedPullFileStatus(value: unknown): string { const status = typeof value === "string" ? value.trim().toLowerCase() : ""; if (status === "m" || status === "modified" || status === "changed") return "modified"; @@ -6597,18 +6720,19 @@ function collectItemContext( if (item.kind === "pull_request") { pullRequest = ghJson(["api", `repos/${targetRepo()}/pulls/${item.number}`]); const pullRecord = asRecord(pullRequest); - const pullFilesWindow = ghPagedContextWindow( - `repos/${targetRepo()}/pulls/${item.number}/files`, - pullRecord.changed_files, - 80, - ); - const pullFiles = pullFilesWindow.items; - const completePullFiles = completePullFilesForDeterministicReview({ + const pullFilesPath = `repos/${targetRepo()}/pulls/${item.number}/files`; + const pullFileHydration = pullFileReviewHydration({ repo: targetRepo(), - total: pullFilesWindow.total, - truncated: pullFilesWindow.truncated, - fetchAll: () => ghPaged(`repos/${targetRepo()}/pulls/${item.number}/files`), + path: pullFilesPath, + total: pullRecord.changed_files, + pullRequest, + fetchPage: ghPage, + fetchPullRequest: () => + ghJson(["api", `repos/${targetRepo()}/pulls/${item.number}`]), }); + const pullFilesWindow = pullFileHydration.window; + const pullFiles = pullFilesWindow.items; + const completePullFiles = pullFileHydration.completeFiles; const pullCommitsWindow = ghPagedContextWindow( `repos/${targetRepo()}/pulls/${item.number}/commits`, pullRecord.commits, diff --git a/test/context.test.ts b/test/context.test.ts index 108ec5f8e1..6148a40356 100644 --- a/test/context.test.ts +++ b/test/context.test.ts @@ -15,6 +15,7 @@ import { githubContextWindowPlan, githubLinkLastPageNumber, githubPaginatedPath, + pullFileReviewHydrationForTest, stripEmptyMaintainerRulingFieldsForTest, } from "../dist/clawsweeper.js"; @@ -143,6 +144,69 @@ test("complete pull-file hydration requires the exact reported count", () => { ); }); +test("complete pull-file hydration requires unique filenames and a stable pull snapshot", () => { + const files = Array.from({ length: 118 }, (_, index) => ({ + filename: `src/file-${index}.ts`, + patch: `+export const value${index} = ${index};`, + })); + + assert.equal( + completePullFilesForDeterministicReviewForTest({ + total: 118, + truncated: true, + files: [...files.slice(0, 117), files[0]], + }), + null, + ); + assert.equal( + completePullFilesForDeterministicReviewForTest({ + total: 118, + truncated: true, + files, + refreshedHeadSha: "c".repeat(40), + }), + null, + ); + assert.equal( + completePullFilesForDeterministicReviewForTest({ + total: 118, + truncated: true, + files, + refreshedBaseSha: "d".repeat(40), + }), + null, + ); +}); + +test("complete pull-file hydration fetches each page at most once", () => { + const files = Array.from({ length: 118 }, (_, index) => ({ + filename: `src/file-${index}.ts`, + patch: `+export const value${index} = ${index};`, + })); + const result = pullFileReviewHydrationForTest({ + total: files.length, + pages: [files.slice(0, 100), files.slice(100)], + }); + + assert.equal(result.hydrated, 80); + assert.equal(result.completeFiles?.length, 118); + assert.deepEqual(result.pageCalls, [1, 2]); +}); + +test("complete pull-file hydration fails closed above GitHub's file-list limit", () => { + const files = Array.from({ length: 100 }, (_, index) => ({ + filename: `src/file-${index}.ts`, + patch: `+export const value${index} = ${index};`, + })); + const result = pullFileReviewHydrationForTest({ + total: 5000, + pages: [files], + }); + + assert.equal(result.completeFiles, null); + assert.deepEqual(result.pageCalls, [1, 50]); +}); + test("complete pull-file hydration fails closed when the full fetch throws", () => { assert.equal( completePullFilesForDeterministicReviewForTest({