From c1accfa58873bd7945d03969bf634b178217199b Mon Sep 17 00:00:00 2001 From: Mark IJbema Date: Tue, 5 May 2026 14:17:33 +0200 Subject: [PATCH] fix(upstream): stop finder hanging on big assets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the subprocess-based line diff in classifyDrift with an in-process multiset diff (approxDiff) so concurrent classifications don't deadlock on big text files like sprite.svg via Bun $.quiet() pipe-buffer stalls. Also filter non-code assets (SVG, PNG, fonts, archives, lock files, etc.) before classification so they don't bloat the report or stress git subprocesses. Switch kilo-only path excludes to glob pathspecs so every packages/kilo-*/ and **/kilocode/** dir is excluded without maintaining a hand list. Rename the 'whitespace-only' bucket to 'cosmetic-only' — with the multiset diff it also catches line reordering, so the old label was misleading. --- script/upstream/README.md | 14 +-- script/upstream/find-reset-candidates.ts | 114 ++++++++++++++++++----- script/upstream/utils/markers.ts | 25 +++++ script/upstream/utils/reset.ts | 16 ++-- 4 files changed, 131 insertions(+), 38 deletions(-) diff --git a/script/upstream/README.md b/script/upstream/README.md index d8c9371247a..cc4ccbee1d7 100644 --- a/script/upstream/README.md +++ b/script/upstream/README.md @@ -274,23 +274,25 @@ Options: --concurrency Parallel classifications (default: 8). ``` -The command pre-filters with `git diff --name-only ..HEAD`, excluding kilo-only paths (`packages/kilo-*`, `packages/opencode/src/kilocode/`, `packages/opencode/test/kilocode/`, `script/upstream/`), then classifies each file against the transformed upstream baseline: +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: | Bucket | Meaning | Action | |---|---|---| | `identical` | Local bytes already match transformed upstream (branding-only drift in raw git diff) | none | | `markers-only` | Stripping `kilocode_change` markers makes local match upstream | reset | -| `whitespace-only` | Only non-marker diff is whitespace | reset | -| `small-diff` | ≤ `--review-limit` non-marker, non-whitespace diff lines | reset | -| `large-diff` | > `--review-limit` non-marker, non-whitespace diff lines | skipped | +| `cosmetic-only` | Non-marker diff is only whitespace or reordered lines (the line multiset is identical) | reset | +| `small-diff` | ≤ `--review-limit` non-marker, non-cosmetic diff lines | reset | +| `large-diff` | > `--review-limit` non-marker, non-cosmetic diff lines | skipped | | `upstream-missing` | File does not exist upstream (kilo-only, intentional) | skipped | | `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 | -`markers-only`, `whitespace-only`, and `small-diff` buckets are auto-reset unless `--dry-run` is passed. A markdown summary is printed to stdout so you can review what happened and spot-check the resulting `git diff`. All resets land as uncommitted working-tree changes; `git diff` / `git checkout` is your safety net. +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". -Tighten the blast radius with `--review-limit 0` (only `markers-only` and `whitespace-only`) or by scoping with a `path` argument (e.g. `packages/opencode/src/mcp`). +`markers-only`, `cosmetic-only`, and `small-diff` buckets are auto-reset unless `--dry-run` is passed. A markdown summary is printed to stdout so you can review what happened and spot-check the resulting `git diff`. All resets land as uncommitted working-tree changes; `git diff` / `git checkout` is your safety net. + +Tighten the blast radius with `--review-limit 0` (only `markers-only` and `cosmetic-only`) or by scoping with a `path` argument (e.g. `packages/opencode/src/mcp`). ## Using Custom Base Branches diff --git a/script/upstream/find-reset-candidates.ts b/script/upstream/find-reset-candidates.ts index 90d3dd2c9b0..85932b6aa6a 100644 --- a/script/upstream/find-reset-candidates.ts +++ b/script/upstream/find-reset-candidates.ts @@ -46,25 +46,52 @@ interface Entry extends ClassifyResult { reset?: boolean } -const KILO_ONLY_PATHS = [ - "packages/kilo-docs", - "packages/kilo-gateway", - "packages/kilo-i18n", - "packages/kilo-indexing", - "packages/kilo-jetbrains", - "packages/kilo-telemetry", - "packages/kilo-ui", - "packages/kilo-vscode", - "packages/opencode/src/kilocode", - "packages/opencode/test/kilocode", - "script/upstream", +const KILO_ONLY_PATHSPECS = [ + ":(exclude,glob)packages/kilo-*/**", + ":(exclude,glob)**/kilocode/**", + ":(exclude)script/upstream", ] -const RESET_BUCKETS = new Set(["markers-only", "whitespace-only", "small-diff"]) +// Non-code assets never make sense to bulk-reset. Big binary-ish files (large +// SVG sprites, icons, fonts, archives) also stress concurrent git subprocesses +// and hide real drift in the report. Use reset-to-upstream.ts per file if you +// really want to restore one of these. +const SKIP_EXTENSIONS = new Set([ + ".svg", + ".png", + ".jpg", + ".jpeg", + ".gif", + ".webp", + ".avif", + ".ico", + ".bmp", + ".woff", + ".woff2", + ".ttf", + ".otf", + ".eot", + ".zip", + ".tar", + ".gz", + ".br", + ".wasm", + ".bin", + ".db", + ".sqlite", + ".mp3", + ".mp4", + ".mov", + ".pdf", +]) + +const SKIP_FILENAMES = new Set(["bun.lock", "package-lock.json", "yarn.lock", "pnpm-lock.yaml", "Cargo.lock"]) + +const RESET_BUCKETS = new Set(["markers-only", "cosmetic-only", "small-diff"]) const BUCKET_ORDER: Bucket[] = [ "markers-only", - "whitespace-only", + "cosmetic-only", "small-diff", "large-diff", "identical", @@ -127,17 +154,37 @@ function args(): Args { } } -async function candidates(commit: string, scope: string | undefined, top: string): Promise { - const pathspecs = [scope ?? ".", ...KILO_ONLY_PATHS.map((p) => `:(exclude)${p}`)] +async function candidates( + commit: string, + scope: string | undefined, + top: string, +): Promise<{ files: string[]; skippedAssets: string[] }> { + const pathspecs = [scope ?? ".", ...KILO_ONLY_PATHSPECS] const result = await $`git diff --name-only ${commit}..HEAD -- ${pathspecs}`.cwd(top).quiet().nothrow() if (result.exitCode !== 0) { throw new Error(`Failed to list candidate files: ${result.stderr.toString()}`) } - return result.stdout + const all = result.stdout .toString() .split("\n") .map((line) => line.trim()) .filter((line) => line.length > 0) + + const files: string[] = [] + const skippedAssets: string[] = [] + for (const file of all) { + if (asset(file)) skippedAssets.push(file) + else files.push(file) + } + return { files, skippedAssets } +} + +function asset(file: string): boolean { + const base = file.slice(file.lastIndexOf("/") + 1) + if (SKIP_FILENAMES.has(base)) return true + const dot = base.lastIndexOf(".") + if (dot === -1) return false + return SKIP_EXTENSIONS.has(base.slice(dot).toLowerCase()) } async function concurrent(items: T[], limit: number, fn: (item: T, index: number) => Promise): Promise { @@ -167,8 +214,8 @@ function group(entries: Entry[]): Map { 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 === "whitespace-only") - return { label: `whitespace-only (${count})`, action: dryRun ? "would reset" : "reset" } + if (bucket === "cosmetic-only") + return { label: `cosmetic-only (${count})`, action: dryRun ? "would reset" : "reset" } if (bucket === "small-diff") return { label: `small-diff (${count})`, action: dryRun ? "would reset" : "reset" } if (bucket === "large-diff") return { label: `large-diff (${count})`, action: "skipped" } if (bucket === "identical") return { label: `identical (${count})`, action: "nothing to do" } @@ -178,7 +225,15 @@ function describe(bucket: Bucket, count: number, dryRun: boolean): { label: stri return { label: `local-missing (${count})`, action: "skipped" } } -function report(entries: Entry[], dryRun: boolean, tag: string, commit: string, scope: string, limit: number) { +function report( + entries: Entry[], + skippedAssets: string[], + dryRun: boolean, + tag: string, + commit: string, + scope: string, + limit: number, +) { const grouped = group(entries) const lines: string[] = [] @@ -189,6 +244,7 @@ function report(entries: Entry[], dryRun: boolean, tag: string, commit: string, lines.push(`- Review limit: ${limit} non-marker diff line(s)`) 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}`) lines.push("") lines.push(`## Summary`) @@ -201,6 +257,7 @@ function report(entries: Entry[], dryRun: boolean, tag: string, commit: string, const info = describe(bucket, items.length, dryRun) lines.push(`| ${bucket} | ${items.length} | ${info.action} |`) } + if (skippedAssets.length > 0) lines.push(`| non-code-asset | ${skippedAssets.length} | skipped |`) lines.push("") for (const bucket of BUCKET_ORDER) { @@ -241,9 +298,10 @@ async function main() { info(`Review limit: ${opts.reviewLimit} non-marker diff line(s)`) info(`Mode: ${opts.dryRun ? "dry-run" : "auto-apply"}`) - const files = await candidates(version.commit, scope, top) + const { files, skippedAssets } = await candidates(version.commit, scope, top) + if (skippedAssets.length > 0) info(`Skipping ${skippedAssets.length} non-code asset(s)`) if (files.length === 0) { - success("No files differ from upstream in scope. Nothing to do.") + success("No code files differ from upstream in scope. Nothing to do.") return } info(`Candidate files: ${files.length}`) @@ -272,7 +330,17 @@ async function main() { } console.log("") - console.log(report(entries, opts.dryRun, version.tag, version.commit, scope ?? "(all shared paths)", opts.reviewLimit)) + console.log( + report( + entries, + skippedAssets, + opts.dryRun, + version.tag, + version.commit, + scope ?? "(all shared paths)", + opts.reviewLimit, + ), + ) } main().catch((err) => { diff --git a/script/upstream/utils/markers.ts b/script/upstream/utils/markers.ts index 8234f340fd1..4f2f9dc9d86 100644 --- a/script/upstream/utils/markers.ts +++ b/script/upstream/utils/markers.ts @@ -421,3 +421,28 @@ export async function changed(base: Text, head: Text, opts?: { ignoreWhitespace? await rm(dir, { recursive: true, force: true }) } } + +/** + * Pure in-process line-diff used by bulk classifiers. Returns the number of + * non-matching lines between two texts using a multiset approach (moving a line + * around doesn't count as drift). Whitespace can optionally be ignored. + * + * Unlike `changed()`, this spawns no subprocesses so it is safe to run + * concurrently without risking pipe-buffer deadlocks on large inputs. + */ +export function approxDiff(base: string, head: string, opts?: { ignoreWhitespace?: boolean }): number { + if (base === head) return 0 + const norm = opts?.ignoreWhitespace ? (line: string) => line.replace(/\s+/g, " ").trim() : (line: string) => line + const counts = new Map() + for (const line of base.split(/\r?\n/)) { + const key = norm(line) + counts.set(key, (counts.get(key) ?? 0) + 1) + } + for (const line of head.split(/\r?\n/)) { + const key = norm(line) + counts.set(key, (counts.get(key) ?? 0) - 1) + } + let total = 0 + for (const v of counts.values()) total += Math.abs(v) + return total +} diff --git a/script/upstream/utils/reset.ts b/script/upstream/utils/reset.ts index 4a028b6b274..2e8f078e363 100644 --- a/script/upstream/utils/reset.ts +++ b/script/upstream/utils/reset.ts @@ -9,7 +9,7 @@ import { rm } from "node:fs/promises" import path from "node:path" -import { binary, changed, clean, join } from "./markers" +import { approxDiff, binary, clean, join } from "./markers" import { translate, upstreamData } from "./upstream" export type ResetAction = "identical" | "deleted" | "written" | "skipped" @@ -72,7 +72,7 @@ export async function resetFile(opts: ResetOptions): Promise { export type Bucket = | "identical" | "markers-only" - | "whitespace-only" + | "cosmetic-only" | "small-diff" | "large-diff" | "upstream-missing" @@ -120,14 +120,12 @@ export async function classifyDrift(opts: ClassifyOptions): Promise