acp migration (ui) : ui connect to acp directly instead of goosed - #10081
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31cc0c4401
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| const { isPackaged = false, resourcesPath } = options; | ||
| const binaryName = process.platform === 'win32' ? 'goose.exe' : 'goose'; |
There was a problem hiding this comment.
Bundle the goose binary for ACP-only builds
When GOOSE_BACKEND_ACP_ONLY is enabled in a packaged desktop app, this path lookup searches for resources/bin/goose, but the packaging workflows I checked only copy goosed into ui/desktop/src/bin (bundle-desktop.yml:139, bundle-desktop-linux.yml:147, bundle-desktop-intel.yml:100, bundle-desktop-windows.yml:146). That means startGooseServe throws “Goose binary not found” before any window opens for packaged ACP-only builds; either package the goose CLI binary as well or start the binary that is actually bundled.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c45e8722a
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| window.alert('failed to start goose backend process'); | ||
| return; | ||
| const backendAcpOnly = window.appConfig.get('GOOSE_BACKEND_ACP_ONLY') === true; | ||
| if (!backendAcpOnly) { |
There was a problem hiding this comment.
Hide REST-only local inference in ACP-only mode
When GOOSE_BACKEND_ACP_ONLY=true, this branch leaves the generated REST client unconfigured and no goosed REST server is started, but ACP capabilities still enable local inference UI (FeaturesContext) and those flows still call generated REST helpers such as syncFeaturedModels()/listLocalModels() in LocalInferenceSettings.loadModels. In an ACP-only build with local-inference support, opening Local Inference settings or the local-model onboarding picker will fetch /local-inference/... against the renderer origin instead of a backend and fail; either migrate/hide these REST-only flows in ACP-only mode or keep a REST endpoint for them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
this will fixed when we migrate local inference from rest to acp
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e318b21b9b
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
* main: task(acp): upgrade SDK and use new HTTP/WS crate (#10082) fix(providers): reject non-object tool-call arguments instead of panicking (#9832) docs: name the message field in the hooks payload guide (#9913) fix(cli): save /edit prompts to history (#10011) feat(i18n): add fr, de, it, pt, id, ms, vi, zh-TW desktop locales (#10072) feat (acp+): Use ACP permission manager for tool permissions (#10066)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c5ccf4605
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (backendAcpOnly) { | ||
| throw new Error('Sampling fallback is not available in direct ACP mode'); |
There was a problem hiding this comment.
Preserve MCP app sampling in ACP-only mode
In ACP-only desktop builds, MCP apps can still render through ACP, but any app that sends sampling/createMessage now immediately fails here because the REST sampling fallback is disabled and no replacement ACP path is used. The goosed path below still posts to /sessions/{sessionId}/sampling/message, so only GOOSE_DESKTOP_BACKEND=acp breaks MCP apps that rely on host sampling; either route this through ACP or keep a usable fallback for this mode.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
mcp app sampling will be removed in another pr
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1c261c7cc
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| useEffect(() => { | ||
| if (backendAcpOnly) { | ||
| return; |
There was a problem hiding this comment.
Restore MCP-UI proxy in ACP-only mode
When GOOSE_DESKTOP_BACKEND=acp and an MCP server returns an MCP-UI externalUrl resource, this early return leaves proxyUrl unset even though UIResourceRenderer is still configured to support externalUrl. Those resources rely on the backend proxy to be fetched/rendered safely, so ACP-only desktop builds will render raw HTML resources but fail or bypass the expected proxy path for external MCP-UI resources. Either expose an ACP-mode /mcp-ui-proxy equivalent and derive it from the ACP URL, or hide/disable externalUrl support in this mode.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
legacy MCP-UI will be removed too
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 74d87f5834
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } catch (error) { | ||
| console.warn('[Analytics] Failed to initialize analytics:', error); | ||
| const backendAcpOnly = window.appConfig.get('GOOSE_DESKTOP_BACKEND') === 'acp'; | ||
| if (!backendAcpOnly) { |
There was a problem hiding this comment.
Keep free provider setup out of ACP-only mode
Beyond the existing local-inference case, this ACP-only branch also leaves the generated REST client unconfigured while first-run onboarding still exposes the Tetrate/NanoGPT cards: FreeOptionCards.handleSetup calls startTetrateSetup()/startNanogptSetup(), which post via generated REST helpers to /handle_tetrate and /handle_nanogpt. In a GOOSE_DESKTOP_BACKEND=acp build, choosing either free provider will therefore fetch the renderer origin instead of a backend and fail; hide/migrate those setup actions or keep a REST endpoint for them in this mode.
Useful? React with 👍 / 👎.
* main: (31 commits) test: generic validator for declarative providers (#10010) UI acp migratoin: Decouple desktop UI types from generated OpenAPI types (Part 2) (#10149) Remove MCP sampling support (#10087) Support TLS for ACP serve (#10088) feat (ui): Migrate dictation local model manager to ACP (#10131) Fix MCP app sandbox bridge lifecycle (#10064) fix(bedrock): send inference config (max_tokens, temperature) on Converse (#9889) feat(providers): support OpenRouter request parameters (#9276) Migrate local inference model management to ACP (#10124) (attempt to) fix disk space errors in linux release builds (#10024) feat: add --edit session flag to edit conversation before forking (#9799) feat: add iFlytek Spark and Astron MaaS providers (#9837) fix(desktop): dedupe Nostr session deep link imports (#9918) [codex] Add SessionStart hook parity outside CLI (#9970) feat(providers): add Fireworks AI declarative provider (#9990) fix(providers): don't retry deterministically-permanent 400s (thinking-block immutability) (#10005) fix(deps): downgrade pkcs8 to v0.10 to match sec1/pkcs1 v0.7 (#10119) chore(deps): bump actions/cache from 5.0.2 to 6.0.0 (#10051) Make OpenAI Responses API store param configurable (#10040) remove unsupported model (#10121) ...
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 222d3b1dad
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| let gooseServeResult: Awaited<ReturnType<typeof startGooseServe>>; | ||
| try { | ||
| gooseServeResult = await startGooseServe({ |
There was a problem hiding this comment.
Ship the binary that main starts by default
In the default release/canary bundle paths I checked, the reusable workflows are called without backend, so bundle-desktop*.yml defaults to goosed; however this non-external startup path now always calls startGooseServe, which resolves and spawns resources/bin/goose in packaged apps. Fresh evidence beyond the prior ACP-only note is that this is no longer limited to an ACP-only build flag: default packaged builds still copy/build goosed (and mac/Linux packaging even removes it via prepare-platform-binaries.js) while the app unconditionally looks for goose, so those bundles fail before opening the first chat window unless the default bundle backend is changed or runtime dispatch is restored.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02efeb2eb6
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const httpBase = httpBaseFromAcpWebSocketUrl(acpUrl).replace(/\/+$/, ''); | ||
| const proxyUrl = new URL(`${httpBase}/mcp-app-proxy`); |
There was a problem hiding this comment.
Avoid routing remote MCP apps through loopback-only proxy
When the configured ACP backend is on another host, deriving the sandbox URL from acpUrl makes the renderer load https://remote/.../mcp-app-proxy. The standalone ACP proxy this points at rejects non-loopback clients in crates/goose/src/acp/mcp_app_proxy.rs and stores guest HTML on a 127.0.0.1 sidecar, so MCP Apps fail to initialize for external backends even though the settings UI supports backends running elsewhere. Keep this proxy local to the desktop app or provide a remote-safe proxy path for non-loopback ACP backends.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14e4b68432
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const currentPath = process.env[pathKey] || ''; | ||
|
|
||
| const env: Record<string, string | undefined> = { | ||
| ...process.env, |
There was a problem hiding this comment.
Redact inherited tokens before logging spawn env
When Goose Desktop is launched from a shell or CI environment that contains credentials such as GITHUB_TOKEN, AWS_SESSION_TOKEN, or *_PASSWORD, this now copies the entire parent environment into spawnOptions; the logger.info('Spawn options', ...) path below only redacts names containing secret or key, so those inherited tokens are written to the desktop logs. Either avoid logging the full inherited env or expand the redaction to cover token/password/credential-style variables before serializing it.
Useful? React with 👍 / 👎.
|
Documentation preview deployed: https://pr-10081.goose-pr-previews-poc.pages.dev |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96ece22af4
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| trustedExternalHostname = new URL(externalBaseUrl).hostname; | ||
| pinnedCertFingerprint = externalBackend.certFingerprint | ||
| ? normalizeFingerprint(externalBackend.certFingerprint) | ||
| : null; |
There was a problem hiding this comment.
Keep external TLS pins with the backend lease
When an external backend window is already open, changing/disabling the external backend and opening another window overwrites these process-wide trustedExternalHostname/pinnedCertFingerprint values, while the old window still gets its original ACP URL from its lease. Once that old window has to verify TLS again (for example after a WSS reconnect or any HTTPS request to its backend), the certificate check compares against the new backend pin or rejects the old host entirely, breaking the existing window; store the trusted host/fingerprint with the same per-window backend lease or allow multiple active pins.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf0a912d79
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }, | ||
| }); | ||
| if (response.ok) { | ||
| const authResponse = await fetch(acpUrl); |
There was a problem hiding this comment.
Add a timeout to the ACP auth probe
When an external backend or reverse proxy returns /status but leaves /acp hanging, this awaited fetch has no AbortSignal, so the 30s retry deadline is never reached and createChat can wait indefinitely instead of showing the external-backend error. Please apply the same bounded probe behavior used for local readiness to the external status/auth checks.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73c1d5e9e3
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 'npx', | ||
| 'uvx', | ||
| 'goosed', | ||
| 'goose', |
There was a problem hiding this comment.
Update docs-generated built-in install links
This allowlist now rejects cmd=goosed, but fresh evidence beyond the prior thread is that documentation/src/utils/install-links.ts still generates built-in extension cards with cmd=goosed. When users click those built-in MCP install links from the docs, Desktop will fail with “Invalid command: goosed” instead of installing the extension; update that generator to emit cmd=goose&arg=mcp... or keep a compatibility alias until all published links are migrated.
Useful? React with 👍 / 👎.
* main: fix: tolerate partial Responses output items (#10108) fix(databricks): always use OpenAI-compatible "parameters" for tools (#10101) fix(providers): unescape shell-escaped image paths (#10098) chore(deps-dev): bump js-yaml from 4.1.1 to 4.2.0 in /documentation (#10107) regenerate schema (#10166) chore(deps): bump actions/stale from 10.1.1 to 10.3.0 (#10050) chore(deps): bump actions/attest-build-provenance from 4.1.0 to 4.1.1 (#10049) chore(deps): bump pnpm/action-setup from 6.0.8 to 6.0.9 (#9892) Update EmpirioLabs provider display name to EmpirioLabs AI (#10126) add declarative provider support to goose-providers crate (#9992) fix(desktop): seed mac auto-update config (#10132) Update release testing instructions (#10122)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48052aaf59
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| const hostname = url.hostname.toLowerCase().replace(/^\[(.*)\]$/, '$1'); | ||
| return hostname === 'localhost' || hostname === '::1' || hostname.startsWith('127.'); |
There was a problem hiding this comment.
Validate loopback hosts as IP literals
The new loopback check still treats DNS names such as wss://127.evil.com/acp or wss://127.0.0.1.example.com/acp as local because URL.hostname is the literal hostname and it only checks startsWith('127.'). In that external-backend scenario, McpAppRenderer will build and load /mcp-app-proxy for a remote backend instead of disabling the loopback-only proxy, so MCP apps can fail or route through an unintended remote proxy; parse the host as an IP address before applying the 127/8 rule.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1fac360c77
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 'npx', | ||
| 'uvx', | ||
| 'goosed', | ||
| 'goose', |
There was a problem hiding this comment.
Update the standalone install-link generator
The allowlist now rejects goosed, but a repo-wide search still finds the standalone generator emitting built-in MCP links with cmd=goosed at ui/install-link-generator/script.js:104. Users who create a built-in install link there will get “Invalid command: goosed” in Desktop instead of installing; fresh evidence beyond the earlier docs threads is this remaining ui/install-link-generator emitter. Please migrate that generator to the accepted goose mcp ... form or keep a compatibility alias before removing goosed here.
Useful? React with 👍 / 👎.
Summary
Moved Goose Desktop off the generated REST/OpenAPI
goosedpath and onto the ACP server exposed bygoose serve.What changed
goose serve --platform desktopso the ACP server can run withGooseDesktopplatform context.goosedstartup with localgoose servestartup over HTTPS/WSS, including readiness checks, TLS fingerprint capture, startup diagnostics, and process cleanup./status, validating/acpauth, and supporting configured cert fingerprints.@hey-api/openapi-ts, generated API files, OpenAPI checks, and oldgoosedintegration wiring.gooseCLI binary instead ofgoosed.Testing
Cleanup after the migration release