From 0c5ec06f6edbe604e9ee32837214afda5b4b3ce3 Mon Sep 17 00:00:00 2001 From: Mark IJbema Date: Tue, 5 May 2026 15:25:32 +0200 Subject: [PATCH] fix(upstream): pre-filter big + policy-protected files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes hang when running the finder from the repo root without a path scope. Root cause: upstream-only packages (packages/console, packages/web, packages/desktop, etc. in config.skipFiles) contain multi-megabyte media assets that stall concurrent Bun `git show` subprocesses on the pipe buffer. Changes: - Batch upstream blob sizes in one `git cat-file --batch-check` subprocess before classifying anything. Files with missing upstream land directly in `upstream-missing`; files > 256 KB land in the new `too-large` bucket. Only sane-sized survivors get per-file `git show`. - Filter out files matching the merge config's `keepOurs` and `skipFiles` globs (.github workflows, translated READMEs, upstream- only packages, etc.) — these are intentionally preserved or removed in Kilo and would otherwise pollute the report and tempt incorrect resets. Full-repo dry-run now completes in ~2s. --- script/upstream/README.md | 9 ++- script/upstream/find-reset-candidates.ts | 94 ++++++++++++++++++++---- script/upstream/utils/reset.ts | 1 + script/upstream/utils/upstream.ts | 34 +++++++++ 4 files changed, 123 insertions(+), 15 deletions(-) diff --git a/script/upstream/README.md b/script/upstream/README.md index cc4ccbee1d7..5368bf8c05f 100644 --- a/script/upstream/README.md +++ b/script/upstream/README.md @@ -274,7 +274,13 @@ Options: --concurrency Parallel classifications (default: 8). ``` -The command pre-filters with `git diff --name-only ..HEAD`, excluding kilo-only paths (anything under `packages/kilo-*/`, any `**/kilocode/**` subdir, `script/upstream/`) and non-code assets (SVG, PNG, fonts, archives, lock files, etc. — see `SKIP_EXTENSIONS` / `SKIP_FILENAMES` in the script). The remaining text files are classified against the transformed upstream baseline: +The command pre-filters with `git diff --name-only ..HEAD` and drops: + +- Kilo-only paths: anything under `packages/kilo-*/`, any `**/kilocode/**` subdir, `script/upstream/`. +- Non-code assets: SVG, PNG, fonts, archives, lock files, etc. (see `SKIP_EXTENSIONS` / `SKIP_FILENAMES` in the script). +- Files covered by the merge config's `keepOurs` or `skipFiles` lists in `utils/config.ts` — these are intentionally preserved or removed in Kilo and must not be bulk-reset. + +It then issues one `git cat-file --batch-check` for all remaining paths to grab upstream blob sizes in a single subprocess. Files absent upstream land in `upstream-missing` immediately; files above 256 KB land in `too-large` (generated manifests, giant snapshots). Only the survivors get fetched via `git show` and classified: | Bucket | Meaning | Action | |---|---|---| @@ -287,6 +293,7 @@ The command pre-filters with `git diff --name-only ..HEAD` | `local-missing` | File tracked but missing locally (deleted in Kilo) | skipped | | `binary-diff` | Binary file differs | skipped (use `reset-to-upstream.ts` per file) | | `binary-identical` | Binary file already matches | none | +| `too-large` | Upstream blob > 256 KB | skipped (use `reset-to-upstream.ts` per file) | Line counting uses an in-process multiset diff (pure JS, no subprocess) for speed and robustness against concurrent git output stalls on big files. Moved/reordered lines therefore count as zero drift, which is usually what you want for "is this file meaningfully different from upstream". diff --git a/script/upstream/find-reset-candidates.ts b/script/upstream/find-reset-candidates.ts index 85932b6aa6a..a6ba3fc5783 100644 --- a/script/upstream/find-reset-candidates.ts +++ b/script/upstream/find-reset-candidates.ts @@ -29,9 +29,11 @@ */ import { $ } from "bun" +import { defaultConfig } from "./utils/config" import { error, header, info, success, warn } from "./utils/logger" +import { matches } from "./utils/match" import { classifyDrift, resetFile, type Bucket, type ClassifyResult } from "./utils/reset" -import { last, normalize, root } from "./utils/upstream" +import { last, normalize, root, upstreamSizes } from "./utils/upstream" interface Args { scope?: string @@ -87,6 +89,12 @@ const SKIP_EXTENSIONS = new Set([ const SKIP_FILENAMES = new Set(["bun.lock", "package-lock.json", "yarn.lock", "pnpm-lock.yaml", "Cargo.lock"]) +// Cap the blob size we'll classify. Generated manifests (models-snapshot.ts, +// openapi.json), media bundled in upstream-only packages (MP4s, PNGs), etc. +// are not meaningful reset candidates and big git show outputs can stall Bun +// subprocesses under concurrency. +const MAX_SIZE = 256 * 1024 + const RESET_BUCKETS = new Set(["markers-only", "cosmetic-only", "small-diff"]) const BUCKET_ORDER: Bucket[] = [ @@ -97,6 +105,7 @@ const BUCKET_ORDER: Bucket[] = [ "identical", "binary-diff", "binary-identical", + "too-large", "upstream-missing", "local-missing", ] @@ -154,11 +163,13 @@ function args(): Args { } } -async function candidates( - commit: string, - scope: string | undefined, - top: string, -): Promise<{ files: string[]; skippedAssets: string[] }> { +interface CandidateSet { + files: string[] + skippedAssets: string[] + skippedPolicy: string[] +} + +async function candidates(commit: string, scope: string | undefined, top: string): Promise { const pathspecs = [scope ?? ".", ...KILO_ONLY_PATHSPECS] const result = await $`git diff --name-only ${commit}..HEAD -- ${pathspecs}`.cwd(top).quiet().nothrow() if (result.exitCode !== 0) { @@ -172,11 +183,31 @@ async function candidates( const files: string[] = [] const skippedAssets: string[] = [] + const skippedPolicy: string[] = [] for (const file of all) { - if (asset(file)) skippedAssets.push(file) - else files.push(file) + if (asset(file)) { + skippedAssets.push(file) + continue + } + if (policyExempt(file)) { + skippedPolicy.push(file) + continue + } + files.push(file) } - return { files, skippedAssets } + return { files, skippedAssets, skippedPolicy } +} + +/** + * Files the upstream merge config marks as "keep ours" (Kilo-specific preserved + * versions) or "skip" (upstream-only, removed in Kilo) should never be touched + * by the bulk resetter. They show up in `git diff` against raw upstream but + * resetting them would undo deliberate Kilo decisions. + */ +function policyExempt(file: string): boolean { + if (matches(file, defaultConfig.keepOurs)) return true + if (matches(file, defaultConfig.skipFiles)) return true + return false } function asset(file: string): boolean { @@ -212,6 +243,12 @@ function group(entries: Entry[]): Map { return out } +function detail(entry: Entry): string { + if (entry.lines === undefined) return "" + if (entry.bucket === "too-large") return ` (${Math.round(entry.lines / 1024)} KB)` + return ` (${entry.lines} line${entry.lines === 1 ? "" : "s"})` +} + function describe(bucket: Bucket, count: number, dryRun: boolean): { label: string; action: string } { if (bucket === "markers-only") return { label: `markers-only (${count})`, action: dryRun ? "would reset" : "reset" } if (bucket === "cosmetic-only") @@ -222,12 +259,14 @@ function describe(bucket: Bucket, count: number, dryRun: boolean): { label: stri if (bucket === "binary-diff") return { label: `binary-diff (${count})`, action: "skipped" } if (bucket === "binary-identical") return { label: `binary-identical (${count})`, action: "nothing to do" } if (bucket === "upstream-missing") return { label: `upstream-missing (${count})`, action: "skipped" } + if (bucket === "too-large") return { label: `too-large (${count})`, action: "skipped" } return { label: `local-missing (${count})`, action: "skipped" } } function report( entries: Entry[], skippedAssets: string[], + skippedPolicy: string[], dryRun: boolean, tag: string, commit: string, @@ -245,6 +284,7 @@ function report( lines.push(`- Mode: ${dryRun ? "dry-run (no writes)" : "auto-apply"}`) lines.push(`- Total candidates: ${entries.length}`) if (skippedAssets.length > 0) lines.push(`- Non-code assets skipped: ${skippedAssets.length}`) + if (skippedPolicy.length > 0) lines.push(`- Config-protected files skipped: ${skippedPolicy.length}`) lines.push("") lines.push(`## Summary`) @@ -258,6 +298,7 @@ function report( lines.push(`| ${bucket} | ${items.length} | ${info.action} |`) } if (skippedAssets.length > 0) lines.push(`| non-code-asset | ${skippedAssets.length} | skipped |`) + if (skippedPolicy.length > 0) lines.push(`| config-protected | ${skippedPolicy.length} | skipped |`) lines.push("") for (const bucket of BUCKET_ORDER) { @@ -267,7 +308,7 @@ function report( lines.push(`## ${info.label} — ${info.action}`) lines.push("") for (const entry of items) { - const suffix = entry.lines !== undefined ? ` (${entry.lines} line${entry.lines === 1 ? "" : "s"})` : "" + const suffix = detail(entry) const note = entry.reset === false ? " [reset failed]" : "" lines.push(`- \`${entry.file}\`${suffix}${note}`) } @@ -298,26 +339,50 @@ async function main() { info(`Review limit: ${opts.reviewLimit} non-marker diff line(s)`) info(`Mode: ${opts.dryRun ? "dry-run" : "auto-apply"}`) - const { files, skippedAssets } = await candidates(version.commit, scope, top) + const { files, skippedAssets, skippedPolicy } = await candidates(version.commit, scope, top) if (skippedAssets.length > 0) info(`Skipping ${skippedAssets.length} non-code asset(s)`) + if (skippedPolicy.length > 0) info(`Skipping ${skippedPolicy.length} file(s) protected by keepOurs/skipFiles config`) if (files.length === 0) { success("No code files differ from upstream in scope. Nothing to do.") return } info(`Candidate files: ${files.length}`) - const entries = await concurrent(files, opts.concurrency, async (file, i) => { + // Batch-check upstream sizes in one subprocess. Pre-bucket absent and oversized + // files so we don't spawn `git show` for a 16 MB .mp4 that would stall under + // concurrency anyway. + info(`Checking upstream blob sizes...`) + const sizes = await upstreamSizes(version.commit, files) + const classifyQueue: string[] = [] + const preBucketed: Entry[] = [] + for (const file of files) { + const size = sizes.get(file) + if (size === null || size === undefined) { + preBucketed.push({ file, bucket: "upstream-missing" }) + continue + } + if (size > MAX_SIZE) { + preBucketed.push({ file, bucket: "too-large", lines: size }) + continue + } + classifyQueue.push(file) + } + if (preBucketed.length > 0) info(`Pre-bucketed ${preBucketed.length} (missing or too-large)`) + info(`Classifying ${classifyQueue.length} file(s)...`) + + const classified = await concurrent(classifyQueue, opts.concurrency, async (file, i) => { const result = await classifyDrift({ root: top, file, commit: version.commit, reviewLimit: opts.reviewLimit, }) - if ((i + 1) % 25 === 0 || i === files.length - 1) { - info(`Classified ${i + 1}/${files.length}`) + if ((i + 1) % 50 === 0 || i === classifyQueue.length - 1) { + info(`Classified ${i + 1}/${classifyQueue.length}`) } return { file, ...result } as Entry }) + const entries = [...preBucketed, ...classified] if (!opts.dryRun) { const resets = entries.filter((e) => RESET_BUCKETS.has(e.bucket)) @@ -334,6 +399,7 @@ async function main() { report( entries, skippedAssets, + skippedPolicy, opts.dryRun, version.tag, version.commit, diff --git a/script/upstream/utils/reset.ts b/script/upstream/utils/reset.ts index 2e8f078e363..f392423d5c2 100644 --- a/script/upstream/utils/reset.ts +++ b/script/upstream/utils/reset.ts @@ -79,6 +79,7 @@ export type Bucket = | "binary-diff" | "binary-identical" | "local-missing" + | "too-large" export interface ClassifyResult { bucket: Bucket diff --git a/script/upstream/utils/upstream.ts b/script/upstream/utils/upstream.ts index 98959b2c23d..2ab0c1b3537 100644 --- a/script/upstream/utils/upstream.ts +++ b/script/upstream/utils/upstream.ts @@ -91,6 +91,40 @@ export async function upstreamData(ref: string, file: string) { throw new Error(`Failed to read ${file} from ${ref}: ${stderr}`) } +/** + * Batch-look up upstream blob sizes for many files in one subprocess. Returns + * a map keyed by the input file path. Missing files map to `null`. Avoids + * per-file `git show` spawns and keeps memory bounded when most candidates are + * missing upstream or above a size threshold. + */ +export async function upstreamSizes(ref: string, files: string[]): Promise> { + const result = new Map() + if (files.length === 0) return result + + const proc = Bun.spawn(["git", "cat-file", "--batch-check"], { + stdin: "pipe", + stdout: "pipe", + stderr: "pipe", + }) + const input = files.map((f) => `${ref}:${f}\n`).join("") + proc.stdin.write(input) + await proc.stdin.end() + + const stdout = await new Response(proc.stdout).text() + const lines = stdout.split("\n").filter((line) => line.length > 0) + for (let i = 0; i < files.length; i++) { + const line = lines[i] ?? "" + if (line.includes(" missing")) { + result.set(files[i], null) + continue + } + const parts = line.trim().split(/\s+/) + const size = Number(parts[2] ?? "") + result.set(files[i], Number.isFinite(size) ? size : null) + } + return result +} + export async function translate(file: string, text: string) { const names = applyPackageNameTransforms(text).result const script = applyScriptTransforms(names).result