Skip to content

tui: set up for publishing via github actions#8020

Merged
alexhancock merged 9 commits intomainfrom
alexhancock/npm-publishing
Mar 23, 2026
Merged

tui: set up for publishing via github actions#8020
alexhancock merged 9 commits intomainfrom
alexhancock/npm-publishing

Conversation

@alexhancock
Copy link
Collaborator

Make it possible to publish the new TUI under @aaif/goose to npm using https://github.com/changesets/action

This will unlock simple usage of the full dependency chain via npx @aaif/goose

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b2f853b91d

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines 3 to +6
"workspaces": [
"acp",
"text"
]
"text",
"goose-acp-server/*"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Regenerate the UI lockfile for the new workspace

Adding goose-acp-server/* as a workspace here without regenerating ui/package-lock.json leaves the lockfile out of sync (ui/package-lock.json still lists only acp and text under the root package). The new publish workflow runs npm ci in ui/, and on this tree npm ci --ignore-scripts --dry-run already exits before install while materializing the new workspaces, so the release job cannot get past dependency installation.

Useful? React with 👍 / 👎.


echo "Copying binaries for ${platform}..."
mkdir -p "${pkg_dir}/bin"
cp -v "${platform_dir}/bin/"* "${pkg_dir}/bin/"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Copy the native binaries from the artifact root

build-native-packages.yml uploads artifact/bin/ as each per-platform artifact, so after the collect/download cycle each native-binaries/goose-acp-server-* directory contains goose-acp-server* at its root rather than under a nested bin/ directory. Copying from ${platform_dir}/bin/* therefore misses every downloaded binary and causes the publish job to fail before any package is released.

Useful? React with 👍 / 👎.

@alexhancock alexhancock force-pushed the alexhancock/npm-publishing branch from b2f853b to e36fc86 Compare March 19, 2026 20:09
os: macos-13
target: x86_64-apple-darwin
- platform: linux-arm64
os: ubuntu-latest
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is where it should be ubuntu-24.04-arm

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e36fc868ce

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

jobs:
build-matrix:
name: Build ${{ matrix.platform }}
runs-on: ubuntu-24.04-arm

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Use the matrix runner for native package builds

In .github/workflows/build-native-packages.yml, build-matrix is pinned to ubuntu-24.04-arm, so the matrix.os values below it are never used. That makes the darwin and win32 legs run on a Linux ARM host instead of macOS/Windows, and even the linux-x64 leg is now being cross-built from ARM without the x86_64 setup this repo uses in build-cli.yml, so this reusable workflow cannot produce the platform artifacts that publish-npm.yml depends on.

Useful? React with 👍 / 👎.

Comment on lines +2 to +8
"@aaif/goose-acp": minor
"@aaif/goose": minor
"@aaif/goose-acp-server-darwin-arm64": minor
"@aaif/goose-acp-server-darwin-x64": minor
"@aaif/goose-acp-server-linux-arm64": minor
"@aaif/goose-acp-server-linux-x64": minor
"@aaif/goose-acp-server-win32-x64": minor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Avoid skipping the intended 0.1.0 npm release

Checked ui/.changeset/initial-release.md against the new workspace manifests: every package already starts at 0.1.0, so marking the "initial release" as minor will make the first changeset version run bump them straight to 0.2.0. If the goal is to publish these packages initially at 0.1.0, this changeset needs a smaller bump (or the manifests need to start from a lower version).

Useful? React with 👍 / 👎.

Copy link
Collaborator

@jamadeo jamadeo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just need to fix the build matrix bit. You want:

jobs:
  build-matrix:
    name: ...
    runs-on: ${{ matrix.os }}

and then

matrix:
  include:
    ...
    - platform: linux-arm64
      os: ubuntu-24.04-arm

I think

@alexhancock alexhancock force-pushed the alexhancock/npm-publishing branch from e36fc86 to 613ca20 Compare March 19, 2026 20:30
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 613ca20951

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines 2 to +6
"private": true,
"workspaces": [
"acp",
"text"
]
"scripts": {
"changeset": "changeset",
"version": "changeset version",
"release": "changeset publish"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Declare the UI packages as npm workspaces

publish-npm.yml installs dependencies by running npm ci in ui/, then immediately runs npm run build in ui/acp and ui/text (.github/workflows/publish-npm.yml:58-69). This root manifest still never declares a workspaces field, so npm only installs the root @changesets/cli dependency and does not install/link the acp, text, or native-package dependencies. In that state the release job cannot reach the build step because tsx, tsc, ink, etc. are missing from those package directories.

Useful? React with 👍 / 👎.

Comment on lines 39 to +42
"devDependencies": {
"@agentclientprotocol/sdk": "^0.14.1",
"@hey-api/openapi-ts": "^0.92.3",
"@types/node": "^20.0.0",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Regenerate ui/acp's npm lockfile

This adds @types/node to ui/acp/package.json, but ui/acp/package-lock.json still has the old dependency set. That breaks the new local/manual ui/acp build flow introduced here: npm ci in ui/acp now has to resolve a fresh tree from the registry (or fails in restricted/offline environments) instead of honoring the checked-in lockfile, so installs for this package are no longer reproducible.

Useful? React with 👍 / 👎.

@alexhancock alexhancock force-pushed the alexhancock/npm-publishing branch from 613ca20 to 3d8d903 Compare March 19, 2026 20:48
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3d8d903bd3

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

"dependencies": {
"@agentclientprotocol/sdk": "^0.14.1",
"@block/goose-acp": "^0.1.0",
"@aaif/goose-acp": "workspace:*",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Pin the ACP dependency before publishing @aaif/goose

If anyone uses the new ui/scripts/publish.sh path (or runs npm publish in ui/text before a changeset version rewrite), npm packs this manifest as-is. I checked npm pack in ui/text and the tarball still contains "@aaif/goose-acp": "workspace:*"; installing a package with that spec fails in npm with EUNSUPPORTEDPROTOCOL, so the published @aaif/goose package would be unusable for all npm consumers.

Useful? React with 👍 / 👎.

Comment on lines 18 to 20
"os": [
"linux"
],

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Declare the Linux native packages as glibc-only

These Linux manifests only constrain os and cpu, but the new build matrix emits aarch64-unknown-linux-gnu and x86_64-unknown-linux-gnu binaries. On Alpine or any other musl-based Linux, npm will still install the optional package, scripts/postinstall.mjs will resolve it, and Goose will then try to exec a glibc-linked binary that cannot start. Add a libc constraint here (and in the ARM64 manifest) or publish separate musl builds.

Useful? React with 👍 / 👎.

@alexhancock alexhancock force-pushed the alexhancock/npm-publishing branch from 3d8d903 to f8bf4eb Compare March 19, 2026 21:00
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f8bf4eb96d

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +80 to +81
mkdir -p artifact/bin
cp target/${{ matrix.target }}/release/goose-acp-server.exe artifact/bin/

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Use a Windows-compatible shell for artifact preparation

GitHub’s Actions docs say run steps on windows-latest default to PowerShell unless you set shell: bash, so the bashisms here (mkdir -p and cp .../) will not execute on the win32-x64 matrix leg. That leg fails before uploading its artifact, which in turn breaks the combined native-binaries-all artifact that publish-npm.yml depends on.

Useful? React with 👍 / 👎.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds like it could be true?

"dependencies": {
"@agentclientprotocol/sdk": "^0.14.1",
"@block/goose-acp": "^0.1.0",
"@aaif/goose-acp": "workspace:*",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep ui/text installable outside the workspace root

This workspace:* spec makes the package unusable when installed from ui/text directly instead of from the workspace root. I checked with npm install --ignore-scripts --dry-run in ui/text, and npm 11 immediately exits with EUNSUPPORTEDPROTOCOL on this dependency, so the documented local cd ui/text && npm i flow and other standalone builds/regression checks no longer work.

Useful? React with 👍 / 👎.

@alexhancock alexhancock force-pushed the alexhancock/npm-publishing branch 2 times, most recently from ab1b50b to 02e74d8 Compare March 20, 2026 00:55
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 02e74d80f5

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +13 to +14
"devDependencies": {
"@changesets/cli": "^2.27.1"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Lock @changesets/cli before invoking the release scripts

These new root scripts rely on the changeset binary, but the checked-in ui/package-lock.json still has no root devDependencies entry and no node_modules/@changesets/cli package. On a clean runner, npm ci in ui/ therefore will not install changeset, so the new changesets/action step dies with changeset: not found when it runs npm run version or npm run release.

Useful? React with 👍 / 👎.

@DOsinga DOsinga added the needs_human label to set when a robot looks at a PR and can't handle it label Mar 20, 2026
@DOsinga
Copy link
Collaborator

DOsinga commented Mar 20, 2026

sorry for the drive by push - experimenting with a new PR review bot - but it should be a good fix I hope

@alexhancock alexhancock force-pushed the alexhancock/npm-publishing branch 2 times, most recently from d0439c1 to bce20c2 Compare March 20, 2026 18:59
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

"@block/goose-acp": "^0.1.0",

P1 Badge Depend on @aaif/goose-acp before publishing @aaif/goose

This manifest still declares @block/goose-acp, but ui/text/src/tui.tsx:22 now imports @aaif/goose-acp. changeset publish packs the manifest, not the workspace lockfile, so npm consumers of @aaif/goose will install the old scope (or nothing, if that package is unavailable) and then fail when dist/tui.js tries to resolve @aaif/goose-acp.



P1 Badge Include goose-acp-server in the native package tarballs

The new build flow stages goose-acp-server{.exe} into each package (ui/acp/scripts/build-native.ts:75), but these manifests still whitelist bin/goose/bin/goose.exe. I verified with npm pack --dry-run in a temp copy of the Linux x64 package that only package.json is packed, so the published native packages would ship without any binary; the same mismatch exists in the sibling platform manifests added in this commit.


const binName = process.platform === "win32" ? "goose.exe" : "goose";
binaryPath = join(pkgDir, "bin", binName);

P1 Badge Point postinstall at goose-acp-server, not goose

The optional packages now contain goose-acp-server{.exe}, but this script still writes <pkg>/bin/goose(.exe) into server-binary.json. I reproduced this on Linux x64 by running postinstall.mjs against a temp install of @aaif/goose-acp-server-linux-x64; it emitted a path ending in bin/goose, which does not exist, so @aaif/goose cannot auto-launch its bundled server unless the user passes --server manually.

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@alexhancock alexhancock force-pushed the alexhancock/npm-publishing branch from bce20c2 to 8d59fb8 Compare March 20, 2026 19:16
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8d59fb8c2f

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

"resolved": "https://registry.npmjs.org/@alcalzone/ansi-tokenize/-/ansi-tokenize-0.2.5.tgz",
"integrity": "sha512-3NX/MpTdroi0aKz134A6RC2Gb2iXVECN4QaAXnvCIxxIm3C3AVB1mkUe8NaaiyvOpDfsrqWhYtj+Q6a62RrTsw==",
"version": "0.1.3",
"resolved": "https://global.block-artifacts.com/artifactory/api/npm/square-npm/@alcalzone/ansi-tokenize/-/ansi-tokenize-0.1.3.tgz",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Regenerate the UI lockfile against the public npm registry

The new release path runs npm ci in ui/ (.github/workflows/publish-npm.yml:58-61), and npm uses the lockfile’s resolved tarball URLs for that exact install. This change rewrites many previously public entries to https://global.block-artifacts.com/... (for example here), while the repo still configures the UI workspace for https://registry.npmjs.org/ (ui/.npmrc:1) and the workflow only authenticates to npmjs (publish-npm.yml:28-32). On GitHub runners outside Block’s network, dependency installation will fail before the packages can be built or published.

Useful? React with 👍 / 👎.

@alexhancock alexhancock force-pushed the alexhancock/npm-publishing branch from d74ee7b to b1a2d95 Compare March 20, 2026 19:41
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b1a2d95f6f

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +58 to +61
- name: Install dependencies
run: |
cd ui
npm ci

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Use a package manager that can install the new UI workspace

This release job now installs ui/ with npm, but the workspace committed here is pnpm-only. I checked cd ui && npm ci --ignore-scripts --dry-run, and npm aborts immediately because this commit deleted ui/package-lock.json; switching to npm install does not help, because npm then exits with EUNSUPPORTEDPROTOCOL on the new workspace:* dependencies (for example ui/text/package.json:34 and ui/desktop/package.json:72). In other words, the job cannot get past dependency installation, so it never reaches the build or publish steps.

Useful? React with 👍 / 👎.

Comment on lines +38 to 39
const binName = process.platform === "win32" ? "goose-acp-server.exe" : "goose-acp-server";
binaryPath = join(pkgDir, "bin", binName);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Don't resolve goose-acp-server while still spawning acp

This change switches the packaged executable from goose to goose-acp-server, but ui/text/src/tui.tsx:1148 still launches whatever path is in server-binary.json as spawn(binary, ["acp"]). The new Rust binary is defined in crates/goose-acp/src/bin/server.rs as a flat clap::Parser with only --host, --port, and --builtin, so goose-acp-server acp exits on startup with an unexpected-argument error. That breaks the default auto-launch path for every consumer of @aaif/goose who relies on the bundled server.

Useful? React with 👍 / 👎.

@alexhancock alexhancock force-pushed the alexhancock/npm-publishing branch from 09da19a to 39da119 Compare March 20, 2026 21:12
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 39da1192ba

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

"access": "public",
"baseBranch": "main",
"updateInternalDependencies": "patch",
"ignore": []

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Exclude the desktop app from the Changesets release set

Changesets' CLI docs say publish walks each package and runs npm publish for versions not already on npm, and their app-versioning guide says non-npm apps should be private. With ignore left empty here, ui/package.json now brings desktop into the workspace, but ui/desktop/package.json is still a public package named goose-app. Any release where that local version is ahead of the registry will therefore try to publish the Electron app alongside @aaif/goose, which is outside the intended release set and can fail the npm release job.

Useful? React with 👍 / 👎.

- name: Setup Node.js
uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4
with:
node-version: '20'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Upgrade the publish runner before installing the full UI workspace

If this workflow is switched to the repo's actual workspace manager, the install will still fail under the Node version pinned here: the newly added desktop workspace requires node: ^24.10.0 and pnpm: >=10.30.0 in ui/desktop/package.json:6-8. I verified locally that pnpm install --frozen-lockfile --lockfile-only --ignore-scripts aborts on those engine checks under Node 22 / pnpm 10.13, so a Node 20 release runner cannot install the full ui/ workspace unless desktop is excluded or the runner is upgraded.

Useful? React with 👍 / 👎.

Copy link
Collaborator

@michaelneale michaelneale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approving to keep push this along, very exciting.

@alexhancock alexhancock force-pushed the alexhancock/npm-publishing branch from 39da119 to 55825ca Compare March 23, 2026 13:31
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 55825ca3a7

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

*/
export type AddExtensionRequest = {
sessionId: string;
session_id: string;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Keep ACP request field names aligned with the wire format

These generated request types now use snake_case keys such as session_id, but the server still deserializes the same ACP requests with #[serde(rename_all = "camelCase")] in crates/goose-acp/src/custom_requests.rs (for example AddExtensionRequest, ReadResourceRequest, and UpdateWorkingDirRequest). Any consumer built against the published @aaif/goose-acp package who follows these shipped types/Zod schemas will send session_id/extension_name/working_dir over extMethod, and Goose will reject those calls because the runtime wire format is still sessionId/extensionName/workingDir.

Useful? React with 👍 / 👎.

@alexhancock alexhancock force-pushed the alexhancock/npm-publishing branch from 55825ca to 9b12b5a Compare March 23, 2026 13:50
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9b12b5a847

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

uses: changesets/action@6d3568c53fbe1db6c1f9ab1c7fbf9092bc18627f # v1
with:
publish: npm run release
version: npm run version

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Regenerate the pnpm lockfile in release PRs

When this job runs in the “open/update release PR” path, changesets/action only executes npm run version here, so the PR will contain version-bumped ui/**/package.json files but no refreshed ui/pnpm-lock.yaml. I checked .github/workflows/ci.yml:123 and .github/workflows/bundle-desktop.yml:149 (same pattern in the other desktop bundle workflows), and they still run pnpm install --frozen-lockfile from ui/desktop; those jobs will fail as soon as the release PR changes any package version or internal workspace range because the checked-in lockfile is now out of sync.

Useful? React with 👍 / 👎.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 82643dc16b

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines 7 to 9
method: "extensions/add",
requestType: "AddExtensionRequest",
responseType: "EmptyResponse",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve the _goose/ prefix in generated method metadata

The generated metadata now advertises Goose-specific RPCs as extensions/add, tools, resource/read, etc., but the server still registers those handlers under _goose/... in crates/goose-acp/src/server.rs (for example on_add_extension, on_get_tools, and on_read_resource). Any consumer that uses GOOSE_EXT_METHODS to drive extMethod() calls will start sending method names the server does not recognize and get method not found errors.

Useful? React with 👍 / 👎.

Comment on lines +32 to +34
method: "session/list",
requestType: null,
responseType: "ListSessionsResponse",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Remove session/list from the extension-method table

session/list is handled as a standard ACP request (on_list_sessions plus the ListSessionsRequest dispatcher in crates/goose-acp/src/server.rs), not as a #[custom_method] extension RPC. Adding it to GOOSE_EXT_METHODS makes the generated metadata claim that extMethod("session/list", ...) is valid, so any caller that iterates this table will hit the wrong code path for listing sessions.

Useful? React with 👍 / 👎.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d0aaea0387

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

"dependencies": {
"@agentclientprotocol/sdk": "^0.14.1",
"@block/goose-acp": "^0.1.0",
"@aaif/goose-acp": "workspace:*",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Don't point ui/text at an unbuilt ACP workspace

With workspace:* here, a normal workspace-root install now links ui/text to the sibling ui/acp package instead of the published SDK. That package still exposes ./dist/index.js / ./dist/index.d.ts, but ui/acp has no prepare/postinstall step and dist/ is not checked in, so a clean checkout cannot run npm run build or npm run start in ui/text until someone manually builds ui/acp first. Before this change the dependency came from npm, so this regresses the repo-local build flow even when installation succeeds from the workspace root.

Useful? React with 👍 / 👎.

Comment on lines +43 to +47
"@aaif/goose-acp-server-darwin-arm64": "workspace:*",
"@aaif/goose-acp-server-darwin-x64": "workspace:*",
"@aaif/goose-acp-server-linux-arm64": "workspace:*",
"@aaif/goose-acp-server-linux-x64": "workspace:*",
"@aaif/goose-acp-server-win32-x64": "workspace:*"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep missing native binaries from looking installed

Changing these optional deps to workspace:* makes a workspace install link the local ui/goose-acp-server-* package directories even on a clean checkout. Those packages git-ignore their bin/ directories, but ui/text/scripts/postinstall.mjs only resolves package.json and then writes server-binary.json without checking that bin/goose-acp-server* exists, so ui/text/src/tui.tsx later tries to spawn a non-existent file and auto-launch fails with ENOENT instead of falling back to --server. This affects source checkouts and CI workspaces where the native binaries have not been staged yet.

Useful? React with 👍 / 👎.

Comment on lines +80 to +81
mkdir -p artifact/bin
cp target/${{ matrix.target }}/release/goose-acp-server.exe artifact/bin/
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds like it could be true?

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7c79f0cd54

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

key: ${{ matrix.platform }}

- name: Build goose-acp-server
run: cargo build --release --target ${{ matrix.target }} --bin goose-acp-server

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Package the stdio ACP binary instead of goose-acp-server

These native packages are consumed by ui/text/src/tui.tsx:1148-1160 as a stdin/stdout ACP transport via ndJsonStream, but this workflow now builds goose-acp-server, whose entry point in crates/goose-acp/src/bin/server.rs only starts an HTTP/WebSocket server. The stdio ACP entry point is still goose acp (crates/goose-cli/src/cli.rs:719-720,1700 -> crates/goose-acp/src/server.rs:1660-1666), so even if the acp-argument issue were fixed, every published native package from this job would still be the wrong executable for the TUI's auto-launch path.

Useful? React with 👍 / 👎.

@alexhancock alexhancock added this pull request to the merge queue Mar 23, 2026
Merged via the queue into main with commit 997f27e Mar 23, 2026
27 of 28 checks passed
@alexhancock alexhancock deleted the alexhancock/npm-publishing branch March 23, 2026 17:34
wpfleger96 added a commit that referenced this pull request Mar 23, 2026
* origin/main:
  fix: handle reasoning content blocks in OpenAI-compat streaming parser (#8078)
  chore(acp): build native packages on latest mac (#8075)
  Display delegate sub agents logs in UI (#7519)
  Update tar version to avoid CVE-2026-33056 (#8073)
  refactor: consolidate duplicated dependencies into workspace (#8041)
  tui: set up for publishing via github actions (#8020)
  feat: feature-gate local inference dependencies (#7976)
  feat: ability to manage sub recipes in desktop ui (#6360)
lifeizhou-ap added a commit that referenced this pull request Mar 24, 2026
* main: (37 commits)
  fix: handle reasoning content blocks in OpenAI-compat streaming parser (#8078)
  chore(acp): build native packages on latest mac (#8075)
  Display delegate sub agents logs in UI (#7519)
  Update tar version to avoid CVE-2026-33056 (#8073)
  refactor: consolidate duplicated dependencies into workspace (#8041)
  tui: set up for publishing via github actions (#8020)
  feat: feature-gate local inference dependencies (#7976)
  feat: ability to manage sub recipes in desktop ui (#6360)
  Tweak the release process: no more merge to main (#7994)
  fix: gemini models via databricks (#8042)
  feat(apps): Pass toolInfo to MCP Apps via hostContext (#7506)
  fix: remove configured marker when deleting oauth provider configuration (#7887)
  docs: add vmware-aiops MCP extension documentation (#8055)
  Show setup instructions for ACP providers in settings modal (#8065)
  deps: replace sigstore-verification with sigstore-verify to kill vulns (#8064)
  feat(acp): add session/set_config and stabilize list, delete and close (#7984)
  docs: Correct `gosoe` typo to `goose` (#8062)
  fix: use default provider and model when provider in session no longer exists (#8035)
  feat: add GOOSE_SHELL env var to configure preferred shell (#7909)
  fix(desktop): fullscreen header bar + always-visible close controls (#8033)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs_human label to set when a robot looks at a PR and can't handle it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants