Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces NPM-based distribution for the ACP-based TUI, packaging the goose text interface with platform-specific native binaries of the goose-acp-server. Users can run the TUI via npx @block/goose, which automatically installs and launches the appropriate native server binary for their platform.
Changes:
- Transformed the TUI package into a publicly distributable NPM package (
@block/goose) - Created platform-specific binary packages using optional dependencies for cross-platform support
- Added auto-launch capability for the bundled goose-acp-server binary with graceful fallback
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/text/package.json | Updated package metadata and added platform-specific optional dependencies |
| ui/text/src/cli.tsx | Added server binary detection, auto-launch, and lifecycle management |
| ui/text/scripts/postinstall.mjs | Resolves platform-specific binary paths during package installation |
| ui/text/scripts/build-native-packages.sh | Builds cross-platform native binaries for distribution |
| ui/text/scripts/publish.sh | Publishes all packages to npm registry |
| npm/goose-acp-server-*/package.json | Platform-specific package manifests for native binaries |
| npm/README.md | Documentation for native package structure and workflow |
Files not reviewed (1)
- ui/text/package-lock.json: Language not supported
| let binaryPath; | ||
| try { | ||
| // Resolve the package directory, then point at the binary inside it | ||
| const pkgDir = dirname(require.resolve(`${pkg}/package.json`)); | ||
| const binName = | ||
| process.platform === "win32" ? "goose-acp-server.exe" : "goose-acp-server"; | ||
| binaryPath = join(pkgDir, "bin", binName); | ||
| } catch { | ||
| // The optional dependency wasn't installed (e.g. wrong platform). That's fine. | ||
| console.warn( | ||
| `@block/goose: optional dependency ${pkg} not installed. ` + | ||
| `You will need to provide a server URL manually with --server.`, | ||
| ); | ||
| process.exit(0); | ||
| } | ||
|
|
||
| const outDir = join(__dirname, ".."); | ||
| mkdirSync(outDir, { recursive: true }); | ||
| writeFileSync( | ||
| join(outDir, "server-binary.json"), | ||
| JSON.stringify({ binaryPath }, null, 2) + "\n", | ||
| ); |
There was a problem hiding this comment.
The postinstall script constructs the binary path but doesn't verify the file actually exists at that location. If the package file list is incomplete or installation was interrupted, spawn attempts will fail with cryptic errors. Consider adding an existsSync check before writing server-binary.json.
ui/text/package.json
Outdated
| }, | ||
| "type": "module", | ||
| "bin": { | ||
| "goose": "dist/goose-text.js" |
There was a problem hiding this comment.
The binary entry point dist/goose-text.js doesn't match the TypeScript output. When tsc compiles src/cli.tsx, it will generate dist/cli.js, not dist/goose-text.js. This will cause the goose command to fail when the package is installed.
| "goose": "dist/goose-text.js" | |
| "goose": "dist/cli.js" |
| "scripts/postinstall.mjs", | ||
| "server-binary.json" |
There was a problem hiding this comment.
The postinstall script writes the binary path to server-binary.json, but this file is included in the package's "files" array. Since the file is generated during postinstall, it won't exist in the published package and doesn't need to be listed here. The "files" array should only include files that exist in the source repository before installation.
| "scripts/postinstall.mjs", | |
| "server-binary.json" | |
| "scripts/postinstall.mjs" |
| const binary = findServerBinary(); | ||
| if (binary) { | ||
| serverProcess = spawn(binary, ["--port", String(DEFAULT_PORT)], { | ||
| stdio: "ignore", | ||
| detached: false, | ||
| }); |
There was a problem hiding this comment.
The binary path from server-binary.json is used without verifying the file exists or is executable. If the optional dependency installation failed silently or the binary is corrupted, spawn will fail with an unclear error. Consider adding a file existence check before attempting to spawn.
alexhancock
left a comment
alexhancock
left a comment
There was a problem hiding this comment.
LGTM
Test failures look related to API keys which we'll need to resolve out of band of this PR
| @@ -0,0 +1,19 @@ | |||
| { | |||
| "name": "@block/goose-acp-server-darwin-arm64", | |||
| "version": "0.1.0", | |||
There was a problem hiding this comment.
these should maybe match the Cargo.toml version?
| @@ -0,0 +1,66 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
we probably don't need this script. We should run the packaging step in the release GHA workflow and pull the appropriate binaries from there
|
closing in favor of #7666 |
There are several ways we could go about distributing the ACP-based TUI.
This PR uses NPM to distribute the app, and creates platform-specific NPM packages that contain the native binaries. A user can then e.g.
npx @block/goose, which will install both the TUI app and the appropriate native goose server.