Repository navigation
refactor(nix): fetch dependencies from bun.lock - #59403
jerome-benoit wants to merge 33 commits into
Conversation
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
The approach looks sound, but I could not verify it: this environment has no nix, so I could not evaluate or build anything on the base or this branch. I checked by reading the code. Reading per-package URLs and integrity from bun.lock and installing with bun install --frozen-lockfile --offline removes the hash that only matched one Bun version. The CLI channel change prod → latest keeps the same database file (opencode.db) and moves the service config to service.json on the shared port, which bundled CLI discovery in Desktop needs. Desktop stays on prod, so its behaviour is unchanged. The desktop → opencode-desktop rename in check-nix.yml also fixes an attribute name the flake does not define.
Public API: only Nix packaging changes. The node_modules_updater flake output and the hash argument of nix/node_modules.nix are removed, a checks.<system>.runtime output is added, and the stable Nix CLI now reports channel latest instead of prod. The new bun2nix flake input needs no changes in downstream flakes. Anyone overriding hash on node_modules.nix would break, which is expected since there is no aggregate hash any more.
Concerns worth addressing (details inline):
- Every package entry in
bun.lockis fetched, including about 305 of 2,876 that are for other platforms (win32, darwin, musl and so on). Before,bun install --os/--cpuonly fetched packages for the host. - The GitHub dependency is downloaded as a GitHub-generated archive and checked against the integrity in
bun.lock. GitHub does not guarantee those archives stay byte-identical over time. - The new CI step builds the CLI and Desktop in full on every PR that touches any
package.json,bun.lock,nix/orpatches/, with the timeout raised from 15 to 60 minutes. That is a large CI cost for most dependency bumps, and it covers only x86_64-linux.
| { | ||
| key = "github:${builtins.elemAt entry 2}"; | ||
| value = { | ||
| url = "https://api.github.com/repos/${builtins.elemAt github 0}/${builtins.elemAt github 1}/tarball/${builtins.elemAt github 2}"; |
There was a problem hiding this comment.
This downloads the archive GitHub generates for the commit and checks it against the integrity Bun recorded in bun.lock. GitHub doesn't promise those generated archives stay byte-identical (they changed in 2023), so this fetch can start failing with a hash mismatch without any change in the repo. Could this source be fetched as unpacked source (for example fetchFromGitHub, with its own hash) instead, so the build doesn't depend on the exact archive bytes?
There was a problem hiding this comment.
GitHub sources use the direct legacy codeload endpoint and retain the SRI from bun.lock. For the current locked Ghostty archive, paired API/codeload downloads are byte-identical and match that SRI. This removes the REST request and its rate-limit cost, not the archive-regeneration risk. For a retained commit in an unchanged repository, GitHub guarantees extracted contents but not compressed bytes. Different compression changes the archive hash; the retained SRI then rejects it and the build fails. An unpacked-source hash is not interchangeable with Bun's archive checksum.
| if: steps.dependencies.outputs.changed == 'true' | ||
| run: nix develop --command bun nix/check-lock.ts | ||
|
|
||
| - name: Check CLI runtime and Desktop build |
There was a problem hiding this comment.
This builds the CLI and Desktop in full, sandboxed, on every PR that touches any package.json, bun.lock, nix/ or patches/, so the timeout goes from 15 to 60 minutes. Most dependency bumps would pay for that. Maintainers may want it limited, for example to pushes to dev or PRs that touch nix//flake.*, or the Desktop build made optional.
There was a problem hiding this comment.
The Nix workflow evaluates flake outputs only, with a 15-minute timeout. It checks CLI, Desktop and devShell derivations for x86_64-linux, aarch64-linux and aarch64-darwin. It does not install workspace dependencies, build packages or execute runtime checks; those remain explicit local verification.
643427f to
204ac21
Compare
There was a problem hiding this comment.
I could not verify this push: there is still no nix in this environment, so I compared the new commit with the last one I reviewed instead. Apart from the rebase, only nix/desktop.nix changed. It now stages the Nix CLI in packages/cli/dist/<package>/, where script/build.ts on dev looks for it, so the production missing-CLI check passes with the real Nix binary. It also calls electron-builder directly, since the package script was removed on dev. Both changes match the current Desktop build and look correct.
Public API: unchanged from my previous review (Nix flake outputs and the Nix CLI channel only).
The three earlier inline points (fetching other platforms' packages, the GitHub archive download, and the full CI build on dependency changes) are not addressed by this push and still apply. Nothing new to raise.
Use Bun's recorded source integrities instead of hashing its installed layout. Keep native Bun responsible for patches and peer contexts. Remove hash calibration and manual lock synchronization. Closes anomalyco#49063
204ac21 to
258a5bc
Compare
There was a problem hiding this comment.
I could not verify this push: there is still no nix in this environment, so I compared it with the last commit I reviewed. The new os/cpu filter in nix/bun-lock.nix addresses the point about fetching other platforms' packages. It reads the metadata from the right position for each kind of source, and node_modules.nix feeds it the same bunOs/bunCpu that bun install gets. I couldn't run the new matcher's edge cases (any, none, negated and unknown values) or the fixtures in nix/check-lock.ts, so a CI run of those would be good to see.
The GitHub archive point is now documented in CONTRIBUTING.md but the fetch is unchanged, and check-nix.yml is unchanged, so those two threads stay open. Public API: unchanged since my previous review. Nothing new to raise.
Preserve the former prod service configuration and discovery during the shared-channel cutover without overwriting existing shared files. Keep live incumbents available to the normal version replacement policy and retain explicit listener overrides. Exercise compiled upgrade and incumbent behavior in Nix runtime checks.
There was a problem hiding this comment.
I could not verify the Nix side of this push (there is still no nix here). The new migration tests pass with bun test test/service.test.ts in packages/cli.
Public API: packages/client now also exports Service.same (backwards compatible). Otherwise unchanged from my previous review.
The new commits move a Nix-only concern into core CLI service code that every release user runs. Every latest build now looks for service-prod.json in the config and state directories, and can adopt a service still running from it. migrateConfig now also accepts legacy-format files for every migration, not just this one. That code isn't Nix packaging, so I'd suggest moving the channel change (OPENCODE_CHANNEL in nix/opencode.nix) and this migration to their own pull request, where service maintainers can review them; details inline. The new service.test.ts cases are long multi-step scenarios. A smaller test of the adoption rule would be easier to follow.
The os/cpu filter from the earlier push is still in place. The GitHub archive and CI build threads stay open.
| legacyConfigFile: legacy ? path.join(global.config, legacy) : undefined, | ||
| // Preserve settings from Nix's former channel without replacing existing shared settings. | ||
| legacyConfigFile: | ||
| OPENCODE_CHANNEL === "latest" |
There was a problem hiding this comment.
This applies to every latest build, including npm and curl installs, not just Nix. Each service start now reads service-prod.json from the config and state directories, and may adopt a service still running from it (with the matching early return in server-process.ts). The migrateConfig change also accepts legacy-format files for every migration, not just this one. This is core service behaviour for a Nix-only channel rename, so I'd suggest splitting it into its own PR together with the OPENCODE_CHANNEL change in nix/opencode.nix. That keeps this PR about fetching dependencies and lets the service changes be reviewed on their own.
There was a problem hiding this comment.
This PR changes no packages/desktop, CLI, client or TUI sources and introduces no service, configuration or storage migration. Nix retains its existing prod channel. Desktop channel alignment remains outside this dependency-fetching change.
|
Verified on Linux x86_64 at Results
The Desktop backend answered Commands and native probesRepository root, with Bun 1.4.4 selected for bun run check
bun nix/check-lock.ts
nix flake check --no-build --all-systems
nix build .#opencode .#opencode-desktop .#checks.x86_64-linux.bun-cache --no-link --print-out-paths -LThe nine fixture outputs per consumer were explicitly rerun with Runtime probes used termctrl/Xvfb and private HOME/XDG/TMPDIR/database paths. The foreground CLI server created a terminal through Dependency graph
Limits
|
There was a problem hiding this comment.
This push only changes tests. The long multi-step migration test is now split into small cases, each with its own temporary directory: adoption, the rejection cases, and an existing shared registration being left alone. They are easier to follow now. bun test test/service.test.ts in packages/cli passes (41 pass, 1 skip). Public API: unchanged since my previous review.
No source changes, so the open threads on the GitHub archive fetch, the CI build scope and the prod → latest service migration in core CLI code still apply.
Preserve shared, named prod, then hashed prod migration precedence. Replace conflicting saved listeners through stop/handoff/ensure without changing explicit CLI overrides. Keep archive URLs and SRI while separating archive hosts from registry namespaces. Override fetch names only when extraction requires it. Cover offline native-Bun consumers and compiled single-owner upgrades.
There was a problem hiding this comment.
I could not verify the Nix side of this push: there is still no nix here, so the cache-namespace fix, the .tgz filename change and the new bun-cache check went unevaluated. bun test test/service.test.ts in packages/cli passes (44 pass, 1 skip).
Public API: adds the checks.<system>.bun-cache flake output. Otherwise unchanged from my previous review.
The new commit grows the core CLI migration: it now also covers hashed prod files, and the service startup path can stop the old service and start a replacement (inline). That makes the case for moving the prod → latest channel change and its migration into its own PR stronger. The open threads on the GitHub archive fetch, the CI build scope and that split still apply.
| ) | ||
| return | ||
| // A successor must inherit the handoff prepared after this process started. | ||
| yield* Service.stop({ file: serviceOptions.file, pty: legacy.state === "ready" ? "handoff" : "clear" }) |
There was a problem hiding this comment.
A starting serve --service process now stops another live service, then calls Service.ensure, which launches yet another serve --service process, and exits without serving. This runs for every stable install that has an old prod registration and saved hostname/port settings, regardless of the old service's version. It is a new step in the core service lifecycle, added for a Nix-only channel rename, and I don't think it should land inside a dependency-fetching refactor. Could it go with the rest of the prod → latest migration into its own PR, where the service maintainers can weigh it against the normal client version policy?
There was a problem hiding this comment.
This PR changes neither server-process.ts nor service-config.ts and introduces no channel cutover or additional service startup path. Native verification starts a real shell with the packaged CLI's embedded PTY helper.
There was a problem hiding this comment.
This push only changes comments and CONTRIBUTING.md, with no behaviour change, so there was nothing to run. Public API: unchanged from my previous review. The open threads on the GitHub archive fetch, the CI build scope, and splitting out the prod → latest service migration and its lifecycle change still apply. The new comment in service-config.ts says released versions can also own a prod registration, which affects stable installs well beyond Nix. That is one more reason to review it in its own PR.
Keep HTTP cache keys aligned with the consuming Bun so nixpkgs can move from 1.4.2 to 1.4.4 without breaking frozen offline installs. Preserve prod TUI state during the latest channel cutover. Replace legacy service owners in the requesting process and adopt the prepared terminal handoff before announcing readiness.
There was a problem hiding this comment.
I could not verify the Nix side of this push (there is still no nix here). The new TUI storage tests (8 pass in packages/tui) and the CLI service tests (44 pass, 1 skip in packages/cli) pass.
Public API: adds a ./pty-handoff export to @opencode/client (backwards compatible), on top of the earlier Service.same export and the Nix flake changes listed before.
None of the open threads is fixed. The service no longer launches a second process after stopping the old one, but it still stops a live service from core startup code (thread 6). The push also adds a third core migration, prod → latest TUI state, and changes the handoff flow for every service start. New concerns are inline: TUI startup can now fail on migration errors, the server duplicates the client's handoff step, and bun-cache.nix depends on Bun's internal cache key format. With the TUI, CLI service, client and Nix all involved, this PR no longer reads as a Nix dependency refactor. Splitting the channel cutover into its own PR would let it be reviewed properly.
| } | ||
| } | ||
|
|
||
| function migrateProdStorage(root: string, destination: string) { |
There was a problem hiding this comment.
This runs on every TUI start for every stable install, not only Nix users moving off prod. Any error other than ENOENT/EEXIST is rethrown from createStorage, so an unreadable or odd prod/tui entry (EACCES, ENOTDIR, a failed copy) stops the TUI from starting. The new test asserts exactly that. Also, because the copy runs on every start, a latest state file the user deleted comes back from prod the next time. A migration should not be able to crash startup: could failures be logged and ignored? And this channel cutover is a TUI behaviour change for all users that would be better reviewed in its own PR.
There was a problem hiding this comment.
This PR changes no TUI storage code. Nix keeps its existing prod channel; no legacy-state import, migration marker or additional startup error handling is included.
| const url = HttpServer.formatAddress(server.address) | ||
| if (serviceOptions !== undefined) | ||
| yield* Effect.tryPromise(() => | ||
| PtyHandoff.complete(serviceOptions.file, { id: instanceID, pid: process.pid, url }), |
There was a problem hiding this comment.
Every service start now clears the terminal handoff ticket itself, not just the legacy-migration path. The client already does this after ensure (client/src/effect/service.ts:95, promise/service.ts:77), so the step now happens in two places, and the server clears it before the client confirms the handoff. That changes the normal restart flow for every install. It would be better confined to the migration path, or reviewed separately with the service owners.
There was a problem hiding this comment.
This PR changes no server or client lifecycle code. Terminal handoff completion remains with the existing client lifecycle; no additional server-side completion is included.
| }; | ||
| in | ||
| { | ||
| name = "@T@${algorithm}-${builtins.substring 0 32 digest}@@@1"; |
There was a problem hiding this comment.
This builds Bun's internal cache folder names by hand (@T@<algo>-<first 32 hex>@@@1) and chooses the format with a bun.version check, to work around the pinned bun2nix. The build then depends on undocumented Bun internals that can change in any Bun release, and the next format change would only show up as a cache miss when installing offline. Could bun2nix be bumped to a version that supports Bun 1.4.4 instead, or the Bun version pinned, rather than copying its key format here?
There was a problem hiding this comment.
Pinned bun2nix master remains 0f2a1f0b6f42cebe3b149bf62d38754c5e0e9729. Its cache-entry creator receives neither archive integrity nor the selected consumer Bun, so a pin bump cannot provide Bun 1.4.4 HTTP cache keys. The adapter covers only HTTP tarball entries; registry/GitHub entries remain bun2nix-produced.
Nine offline fixtures passed under each Bun consumer (1.4.2/1.4.4), including lockfile v2 and scoped-override v3 cases, on Linux x86_64 (verification). Unknown future cache formats are not guaranteed. The upstream producer request remains open; this maintenance risk is not resolved.
Keep legacy TUI filesystem errors nonfatal and record completed imports so deleted latest state stays deleted. Failed imports remain retryable. Complete PTY handoff inside the server only on the local legacy replacement path. Normal ensures retain ready-acknowledgement cleanup.
There was a problem hiding this comment.
This push addresses two earlier points. The TUI prod → latest storage copy now logs failures instead of crashing startup, and a marker stops deleted latest state from coming back. The server now clears the terminal handoff ticket only after it has stopped an old prod service, so the normal restart flow is unchanged. The TUI storage tests (10 pass) and CLI service tests (44 pass, 1 skip) pass. I still couldn't run the Nix side.
Public API: unchanged from my previous review.
The remaining open threads still apply: stopping the old service from core startup and splitting the channel cutover into its own PR, the hand-built Bun cache keys, the GitHub archive fetch, and the CI build scope.
There was a problem hiding this comment.
This push only merges dev. The files this PR changes are identical to the previous head, so there was nothing new to run or review. Public API: unchanged from my previous review. The remaining open threads still apply: the GitHub archive fetch, the CI build scope, splitting the prod → latest cutover (including the service stop at startup) into its own PR, and the hand-built Bun cache keys.
There was a problem hiding this comment.
I could not verify this push. One of its new client tests fails consistently here. In packages/client, bun test test/service.test.ts test/service-reconnect.test.ts gives 44 pass and 1 fail: the new "an incumbent observation cannot retarget an owner published during its probe" fails 3 out of 3 runs on its own, after about 2s each (inline).
Public API: @opencode/client gains StopOptions.expected, and the value Service.incumbent returns now includes info. Both are additive and backwards compatible, on top of the earlier ./pty-handoff, Service.same and Nix flake changes.
Checking that the expected service still owns the registration before stopping it closes a real race. But it changes how ensure replaces services for every client, so this PR now spans the Nix build, the CLI service, the TUI and the client package. That reinforces the open request to move the prod → latest cutover and the service changes into their own PR. The remaining open threads (GitHub archive fetch, CI build scope, hand-built Bun cache keys) still apply.
| expect(await Bun.file(fixture.registration).exists()).toBe(false) | ||
| }) | ||
|
|
||
| test("an incumbent observation cannot retarget an owner published during its probe", async () => { |
There was a problem hiding this comment.
This fails consistently for me. Run on its own (bun test test/service.test.ts -t "incumbent observation" in packages/client) it failed 3 out of 3 times, each after about 2.02s, with Observed owner did not finish its probe: Service.incumbent gives up before the probe-gated fixture releases its first request. It depends on two spawned services, file-based gates, sleep loops in the fixture and a 15s deadline, so it is also timing-sensitive. Could it be made deterministic, for example by testing stop({ expected }) directly with a registration that differs from expected, instead of racing a live probe?
There was a problem hiding this comment.
This PR changes no packages/client code. It introduces no conditional-stop API, ownership-race test or probe-gated fixture change.
There was a problem hiding this comment.
This push fixes the client test that failed every time: it no longer depends on file gates and sleep loops, and it passes reliably here. Run on its own it passed 3 out of 3 times (about 31ms each), and the full service.test.ts + service-reconnect.test.ts run in packages/client gives 45 pass, 0 fail. The probe-gated fixture mode in test/fixture/service.ts is no longer used and can be dropped.
Public API: unchanged from my previous review. The other open threads (GitHub archive fetch, CI build scope, splitting the prod → latest cutover and service changes into their own PR, hand-built Bun cache keys) still apply.
Keep CLI, client and TUI behavior unchanged. Select the Nix prod service through Desktop's existing channel setting, including its early probe.
Use the shared build channel for service discovery and preserve the existing runtime override.
Keep executable reuse and cleanup within the raw build channel while preserving version and architecture identity.
6d3e2e8 to
17d18f2
Compare
|
Thanks for tackling #49063, getting rid of Right now this PR has grown pretty large though, so I'd like to narrow the scope before merging:
(Automated review) |
|
@rekram1-node The diff is limited to Nix packaging and contributor guidance.
Verification and limits: builds, TUI and embedded PTY pass. Standalone Desktop startup hits the existing registration mismatch; #60683 provides the separate runtime fix. Nix keeps |
# Conflicts: # .github/workflows/bot-nix-hashes.yml
Issue for this PR
Closes #49063
Closes #59882
Closes #59601
Related: #51796 (NixOS helper loading only; generic glibc portability and diagnostics are not addressed).
Type of change
What does this PR do?
Fetch Nix dependencies from
bun.locktext formats 1–3 instead of maintaining platform-specificnode_moduleshashes.How did you verify your code works?
Linux x86_64 at
122ead5: CLI/Desktop builds, packaged TUI welcome and embedded PTY shell passed.bun run checkpassed (35/35 cached tasks), as did lock import checks and nine offline fixtures under each Bun consumer (1.4.2/1.4.4).All three configured targets passed evaluation. CI remains evaluation-only with a 15-minute timeout. Desktop startup requires the separate service-discovery fix in #60683. ARM Linux/macOS runtimes are untested.
Commands, exact results and limits.
Checklist