-
Notifications
You must be signed in to change notification settings - Fork 134
fix: [#1052] bundled cleanup — 5 deferred items + 3 review-fixes #1085
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 10 commits
59cae82
7b945a3
4f92ed7
78e8287
b73fdc9
3b947f5
c237795
977e207
630accd
5b30a30
d2c2cd6
cafdcf4
dca7360
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,3 +18,7 @@ if (process.versions.bun !== expectedBunVersion) { | |
| } | ||
| ' | ||
| bun typecheck | ||
| # altimate_change — #1052 D8: scan pushed content for internal-tracker refs | ||
| # (see RULES in the script for the specific patterns). Silent on clean; | ||
| # exits 1 on hit. Bypass with SKIP_TRACKER_CHECK=1 for genuine emergencies. | ||
| bun script/check-tracker-leaks.ts | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: The guard silently disables itself when the local base ref is missing: check-tracker-leaks.ts computes base from a hardcoded local Prompt for AI agents |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,8 @@ import path from "path" | |
| import { fileURLToPath } from "url" | ||
| import { createRequire } from "node:module" | ||
| import solidPlugin from "@opentui/solid/bun-plugin" | ||
| // altimate_change — #1052 D10: sha256 for the per-target build-inputs stamp. | ||
| import { createHash } from "node:crypto" | ||
|
|
||
| const __filename = fileURLToPath(import.meta.url) | ||
| const __dirname = path.dirname(__filename) | ||
|
|
@@ -523,6 +525,122 @@ for (const item of targets) { | |
| 2, | ||
| ), | ||
| ) | ||
|
|
||
| // altimate_change start — #1052 D10: emit a build-inputs stamp so the | ||
| // smoke-test staleness guard can compare against ALL binary-embedded inputs, | ||
| // not just src/ + script/ mtimes. | ||
| // | ||
| // The previous guard (m5) walked src/ + script/ for the newest mtime — good | ||
| // for the common case but blind to changes in CHANGELOG.md, migrations, | ||
| // bundled skills, the models.dev snapshot, the parser worker, and the | ||
| // per-platform altimate-core prebuild. Editing any of those without touching | ||
| // a .ts file would leave the guard silent and the binary silently stale. | ||
| // | ||
| // Stamp format: JSON with one entry per input, sha256 of file content. Read | ||
| // side rehashes each listed path and compares; any mismatch → stale. Paths | ||
| // are REPO_ROOT-relative so entries under packages/tui, packages/core, the | ||
| // workspace-root package.json, bun.lock, etc. resolve without munging. | ||
| const REPO_ROOT = path.resolve(dir, "../..") | ||
| const stampInputs: Array<{ path: string; sha256: string }> = [] | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| const addFile = (absPath: string) => { | ||
| try { | ||
| const buf = fs.readFileSync(absPath) | ||
| const rel = path.relative(REPO_ROOT, absPath) | ||
| const hash = createHash("sha256").update(buf).digest("hex") | ||
| stampInputs.push({ path: rel, sha256: hash }) | ||
| } catch { | ||
| // Missing file: silently skip. The stamp only covers what actually | ||
| // shipped; a file the build didn't need doesn't invalidate the guard. | ||
| } | ||
| } | ||
| // CHANGELOG.md | ||
| addFile(changelogPath) | ||
| // Migrations | ||
| for (const m of migrationDirs) addFile(path.join(dir, "migration", m, "migration.sql")) | ||
| // Skills bundled via .opencode/skills/ | ||
| for (const entry of skillEntries) addFile(path.join(skillsRoot, entry.name, "SKILL.md")) | ||
| // Generated models snapshot (build.ts rewrote it before we got here) | ||
| addFile(path.join(dir, "src/provider/models-snapshot.ts")) | ||
| // opentui parser worker | ||
| addFile(parserWorker) | ||
| // Per-target altimate-core NAPI prebuild | ||
| addFile(platformNodeSrc) | ||
| // altimate_change — #1052 D10 review-fix (M2): package.json + bun.lock cover | ||
| // dependency-version bumps that change what Bun.build embeds. Without these, | ||
| // `bun install` bumping a bundled dep would leave the stamp reporting fresh. | ||
| // Include per-package package.json in the workspace walk below. | ||
| addFile(path.join(REPO_ROOT, "package.json")) | ||
| addFile(path.join(REPO_ROOT, "bun.lock")) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [WARNING]:
Reply with |
||
| // Also include tsconfig files that affect compiled output shape | ||
| // (bot review: tsconfig changes can flip target/moduleResolution). | ||
| addFile(path.join(dir, "tsconfig.json")) | ||
| // src/ + script/ TypeScript tree — hash every file the compiler actually saw | ||
| // (same extension filter build.ts globs for embedding). | ||
| const IGNORED = new Set(["node_modules", ".turbo", ".cache", "dist", "target"]) | ||
| const walk = (root: string): void => { | ||
| let entries: fs.Dirent[] | ||
| try { | ||
| entries = fs.readdirSync(root, { withFileTypes: true }) | ||
| } catch { | ||
| return | ||
| } | ||
| for (const entry of entries) { | ||
| if (entry.name.startsWith(".")) continue | ||
| if (IGNORED.has(entry.name)) continue | ||
| const full = path.join(root, entry.name) | ||
| if (entry.isDirectory()) { | ||
| walk(full) | ||
| continue | ||
| } | ||
| if (!/\.(tsx?|json|txt|md)$/.test(entry.name)) continue | ||
| addFile(full) | ||
| } | ||
| } | ||
| walk(path.join(dir, "src")) | ||
| walk(path.join(dir, "script")) | ||
|
cubic-dev-ai[bot] marked this conversation as resolved.
|
||
| // altimate_change — #1052 D10 review-fix (M2): also hash every workspace | ||
| // package's src/ tree. `packages/opencode/src` imports from | ||
| // `@opencode-ai/{core,tui,util,plugin,sdk,server,cli,...}` and | ||
| // `@altimateai/{dbt-tools,drivers}` — Bun.build follows these imports and | ||
| // bundles them into the binary transitively. The original stamp walked only | ||
| // packages/opencode, so edits under any sibling workspace package would leave | ||
| // the binary silently stale. Enumerate `packages/*/src` at build time (rather | ||
| // than hard-coding names) so new packages get covered automatically. | ||
| const packagesRoot = path.resolve(REPO_ROOT, "packages") | ||
| try { | ||
| for (const pkg of fs.readdirSync(packagesRoot, { withFileTypes: true })) { | ||
| if (!pkg.isDirectory() || pkg.name.startsWith(".")) continue | ||
| // Skip packages/opencode — already covered by the walks above. | ||
| if (pkg.name === "opencode") continue | ||
|
cubic-dev-ai[bot] marked this conversation as resolved.
|
||
| const pkgSrc = path.join(packagesRoot, pkg.name, "src") | ||
| if (fs.existsSync(pkgSrc)) walk(pkgSrc) | ||
| // Each workspace package.json influences its resolution/exports and could | ||
| // change what ends up in the binary even when its src/ files are unchanged. | ||
| const pkgJson = path.join(packagesRoot, pkg.name, "package.json") | ||
| if (fs.existsSync(pkgJson)) addFile(pkgJson) | ||
| } | ||
| } catch { | ||
| // packages/ missing (unlikely at build time) — skip; addFile() ignores non-existent paths anyway. | ||
| } | ||
| // Deterministic order so the aggregate hash is stable across build runs. | ||
| stampInputs.sort((a, b) => a.path.localeCompare(b.path)) | ||
| const aggregate = createHash("sha256") | ||
| .update(stampInputs.map((i) => `${i.path}\t${i.sha256}`).join("\n")) | ||
| .digest("hex") | ||
| await Bun.file(`dist/${name}/bin/build-inputs.json`).write( | ||
| JSON.stringify( | ||
| { | ||
| target: name, | ||
| version: Script.version, | ||
| aggregate, | ||
| inputs: stampInputs, | ||
| }, | ||
| null, | ||
| 2, | ||
| ), | ||
| ) | ||
| // altimate_change end | ||
|
|
||
| binaries[name] = Script.version | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -121,12 +121,35 @@ export namespace ModelsDev { | |
| const result = await Filesystem.readJson(Flag.OPENCODE_MODELS_PATH ?? filepath).catch(() => {}) | ||
| if (result) return result | ||
| const result2 = await fetchApi() | ||
| if (result2.ok) { | ||
| await Filesystem.write(filepath, result2.text).catch((e) => { | ||
| log.error("Failed to write models cache", { error: e }) | ||
| // altimate_change — #1052 D14 review-fix (M3): fetchApi returning a non-2xx | ||
| // (e.g. 5xx with an HTML error body) previously fell through to | ||
| // `JSON.parse(<HTML>)` and crashed with SyntaxError. Return an empty | ||
| // catalog instead — callers already tolerate empty results (Provider.state | ||
| // just yields no models.dev-derived providers, which is the same UX as | ||
| // running with OPENCODE_DISABLE_MODELS_FETCH=1). Pre-D14 this rarely | ||
| // fired because the eager refresh usually warmed the disk cache; post-D14 | ||
| // more first-calls fall through to fetch, so more chances to hit the crash. | ||
| // | ||
| // Bot-review follow-up: a 2xx can still carry HTML or truncated JSON | ||
| // (proxies, load-balancer error pages that respond 200, mid-stream | ||
| // truncation). Try to parse first; only cache + return on success. On | ||
| // parse failure, log and return empty — same graceful-degradation path | ||
| // as the non-2xx branch, and we don't poison the disk cache with junk. | ||
| if (!result2.ok) return {} | ||
| let parsed: Record<string, unknown> | ||
| try { | ||
| parsed = JSON.parse(result2.text) | ||
|
cubic-dev-ai[bot] marked this conversation as resolved.
Outdated
|
||
| } catch (e) { | ||
| log.error("models.dev returned non-JSON body; not caching", { | ||
| error: e, | ||
| firstBytes: result2.text.slice(0, 120), | ||
| }) | ||
| return {} | ||
| } | ||
| return JSON.parse(result2.text) | ||
| await Filesystem.write(filepath, result2.text).catch((e) => { | ||
| log.error("Failed to write models cache", { error: e }) | ||
| }) | ||
| return parsed | ||
| }) | ||
| }) | ||
|
|
||
|
|
@@ -152,18 +175,38 @@ export namespace ModelsDev { | |
| } | ||
|
|
||
| if (!Flag.OPENCODE_DISABLE_MODELS_FETCH && !process.argv.includes("--get-yargs-completions")) { | ||
| // altimate_change start — upstream_fix: bridge merge removed the setTimeout(...,0) | ||
| // wrapper. Defer the initial refresh past the current microtask so that | ||
| // Installation.USER_AGENT (used inside refresh()) is fully initialized — we hit | ||
| // a circular-dep issue on cold start without this. See altimate commit 980efaab64. | ||
| setTimeout(() => { | ||
| ModelsDev.refresh() | ||
| setInterval( | ||
| async () => { | ||
| await ModelsDev.refresh() | ||
| }, | ||
| 60 * 1000 * 60, | ||
| ).unref() | ||
| }, 0) | ||
| // altimate_change start — #1052 D14: drop the eager import-time ModelsDev.refresh(). | ||
| // | ||
| // The previous `setTimeout(() => ModelsDev.refresh(), 0)` fired a fetch to | ||
| // https://models.dev/api.json at module import. Its `AbortSignal.timeout(10000)` | ||
| // cannot cancel a synchronous `getaddrinfo()` — under Linux `unshare --net` | ||
| // (Verdaccio sanity Phase 3 [10/10] on Ubuntu CI runners) the DNS call blocked | ||
| // long enough that the pending fetch held the event loop past command | ||
| // completion and SIGTERM landed before any bytes flushed. That blocked the | ||
| // v0.9.4 release. | ||
| // | ||
| // Callers that need model data use `ModelsDev.Data()`, which resolves in this | ||
| // priority order: (1) local disk cache, (2) bundled snapshot at | ||
| // `models-snapshot.ts` (embedded in release binaries — regenerated at each | ||
| // build; dev-mode builds without the snapshot fall through to fetch), (3) | ||
| // `Flock.withLock(...) → fetchApi()` only when both are absent. Release | ||
| // binaries therefore have release-time model metadata even on a cold-start | ||
| // with no network. Long-running processes (TUI, serve) still receive updates | ||
| // via the hourly `setInterval` below (`.unref()`'d so it never blocks exit). | ||
| // | ||
| // Trade-off: without an eager fetch, models added to models.dev between | ||
| // releases would not appear until the hourly interval below fires. The | ||
| // fire-and-forget refresh() below narrows that window without holding the | ||
| // event loop — a microtask can't itself keep Bun alive, and if the fetch it | ||
| // schedules is still in flight at process-exit, the snapshot covers callers | ||
| // on the next run. The load-bearing part of the D14 fix (removing the | ||
| // setTimeout(...,0)-wrapped fetch that kept the loop alive) is preserved. | ||
| // | ||
| // If this reintroduces the unshare-net hang on CI, drop the Promise.then | ||
| // line — the snapshot alone still keeps release binaries functional offline. | ||
| Promise.resolve().then(() => ModelsDev.refresh().catch(() => {})) | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WARNING: Boot-time
Since Bun's Reply with
cubic-dev-ai[bot] marked this conversation as resolved.
Outdated
|
||
| setInterval(async () => { | ||
| await ModelsDev.refresh() | ||
| }, 60 * 60 * 1000).unref() | ||
| // altimate_change end | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P1: The pre-push hook can approve one ref while pushing another because it never passes the hook's pushed ref/update data to the scanner; the scanner always examines the current
HEAD. A command such asgit push origin featurefrom a cleanmaincan therefore publish an unchecked branch. Reading the pre-push stdin tuples and scanning each pushed local OID (or restricting the hook to the checked-out ref) would align the check with the actual push.Prompt for AI agents