Skip to content

Lifei/delete tauri backend acp#8582

Merged
jamadeo merged 13 commits into
mainfrom
lifei/delete-tauri-backend-acp
Apr 16, 2026
Merged

Lifei/delete tauri backend acp#8582
jamadeo merged 13 commits into
mainfrom
lifei/delete-tauri-backend-acp

Conversation

@lifeizhou-ap
Copy link
Copy Markdown
Collaborator

@lifeizhou-ap lifeizhou-ap commented Apr 16, 2026

Summary

Clean up PR following #8582

Testing

Tested locally

Copy link
Copy Markdown

@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: 386d15ab2b

ℹ️ 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".

Comment on lines +214 to +217
#[cfg(target_os = "windows")]
{
None
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Restore Windows goose binary discovery

find_goose_via_login_shell() now hard-returns None on Windows, and the only fallback search paths are Unix-specific (/opt/homebrew/bin, /usr/local/bin, etc.), so find_goose_binary() cannot locate goose.exe unless GOOSE_BIN is manually set. This makes default startup fail on Windows with "Unknown or unavailable agent provider: goose" even when Goose is installed on PATH, which is a functional regression from the previous cross-platform resolver.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This code was copied from the acp_client crate, which had the same Windows behavior. Not a regression from this PR. can improve later

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we'll need to do a bunch of testing on windows, I fear there hasn't been much so far in goose 2

Comment on lines +118 to +120
match tokio::net::TcpStream::connect(&addr).await {
Ok(_) => return Ok(()),
Err(_) => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Probe WebSocket readiness before returning from spawn

The readiness loop now treats a successful raw TCP connect as "server ready", but Goose clients immediately need a working WebSocket ACP endpoint (/acp); a process can accept TCP before ACP/WebSocket initialization is complete. In that startup window, getClient()/provider loading can fail intermittently right after app launch, so this check should validate the WebSocket handshake rather than only port liveness.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

In practice, goose serve doesn't accept TCP connections until it's fully initialized, so the TCP probe has been reliable. Removed the WebSocket handshake here to drop the tokio-tungstenite dependency. Happy to revisit if we see flaky startup issues though.

Copy link
Copy Markdown

@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: 076d2a6372

ℹ️ 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".

: undefined);
await acpLoadSession(sessionId, gooseSessionId, workingDir);
useChatStore.getState().setSessionLoading(sessionId, false);
const buffer = getAndDeleteReplayBuffer(sessionId);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Clear replay buffer on failed session load

getAndDeleteReplayBuffer(sessionId) now runs only on the success path, so a failed acpLoadSession leaves any partially buffered replay messages in the module-level map. If replay emits chunks before the load fails (for example, connection drops mid-replay), a retry can append onto stale buffered messages and later render duplicated/outdated history for that session. Add buffer cleanup in the error path (or in a finally) so failed loads cannot leak replay state across attempts.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

fixed

Copy link
Copy Markdown

@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: 219a5a897b

ℹ️ 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".

Comment on lines 101 to +103
await acpLoadSession(sessionId, gooseSessionId, workingDir);
useChatStore.getState().setSessionLoading(sessionId, false);
const buffer = getAndDeleteReplayBuffer(sessionId);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Add timeout guard around session replay load

loadSessionMessages now relies entirely on await acpLoadSession(...) to settle before clearing loadingSessionIds; if that ACP call stalls (e.g., dropped websocket or hung backend), the session remains permanently “loading” and the user never gets an error. The previous useAcpStream watchdog provided a 30s escape hatch, but this commit removed that path, so this flow now has no bounded-failure behavior.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

we will revisit this soon in another pr.

@lifeizhou-ap lifeizhou-ap requested a review from jamadeo April 16, 2026 10:47
Copy link
Copy Markdown

@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: 003b79aa9d

ℹ️ 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".

@@ -1,6 +1,5 @@
import { useCallback, useEffect, useState } from "react";
import { useCallback, useState } from "react";
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Restore context refresh after assistant responses

Removing the ACP completion listener here drops the only automatic refetchFiles() trigger in ContextPanel, so after an agent turn that edits files, the Changes panel can remain stale until the user manually refreshes or refocuses the window. This is a functional regression for normal chat-driven editing flows; add a replacement completion hook (for example, on ACP prompt completion/session notification) to refresh context data when the active session finishes.

Useful? React with 👍 / 👎.

resolve: {
alias: {
"@": resolve(__dirname, "./src"),
"@aaif/goose-sdk": resolve(__dirname, "./src/test/mocks/goose-sdk.ts"),
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Point Vitest goose-sdk alias to an existing mock file

The Vitest alias targets ./src/test/mocks/goose-sdk.ts, but this commit adds goose-acp.ts and no goose-sdk.ts file exists. That means any test that imports @aaif/goose-sdk (directly or transitively through ACP connection code) will fail during module resolution before test logic runs. Update the alias path or rename the mock file so ACP-related tests remain runnable.

Useful? React with 👍 / 👎.

@jamadeo jamadeo added this pull request to the merge queue Apr 16, 2026
Merged via the queue into main with commit e7a91c7 Apr 16, 2026
23 of 24 checks passed
@jamadeo jamadeo deleted the lifei/delete-tauri-backend-acp branch April 16, 2026 16:53
michaelneale added a commit that referenced this pull request Apr 17, 2026
* main: (37 commits)
  polish: refine sidebar activity indicators, add placeholder token, and tidy search field (#8606)
  feat: add /edit command to cli for on-demand prompt editing (#8566)
  docs(mcp): add Rendex MCP Server extension tutorial (#8541)
  Lifei/delete tauri backend acp (#8582)
  chore: set goose binaries as executable in package.json (#8589)
  feat: add Novita AI as declarative provider (#8432)
  feat: add Kimi Code provider with OAuth device flow authentication (#8466)
  fix: chat loading-state model placeholder (#8431)
  fix: expand tool calls by default when Response Style is Detailed (#8478)
  fix: create logs dir before writing llm request log (#8522)
  fix: enable token usage tracking and configurable stream timeout for Ollama provider (#8493)
  fix tauri-plugin-dialog version constraint to match other plugins (#8542)
  call goose serve from tauri frontend via goose-acp client (#8549)
  failed the script when bundle:default fails and cleanup "alpha"  (#8580)
  pass globally unique conversation identifier as sessionId in databricks api call (#8576)
  fix: use sqlx chrono decode for thread timestamps instead of manual parsing (#8575)
  docs: remove stale gemini-acp references (#8572)
  show individual untracked files in git changes widget (#8574)
  fix: update publishing flow to include new sdk dir (#8573)
  fix: remove double border on content in chat (#8545)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants