Repository navigation
fix(tui): use server home for remote directory changes - #55203
nakulmaurya wants to merge 23 commits into
Conversation
Updated test to include paths in the expected JSON response.
Added optional 'home' property to 'paths' in ServerInfoOutput type.
Add a test for server home directory completion and recent paths.
|
The following comment was made by an LLM, it may be inaccurate: |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix works. I ran the #55171 reproduction (attached TUI with a different home than the server) from source. On the base branch, /cd ~/ autocomplete fails and /cd ~ errors with Location not found: <client home>. With this PR, autocomplete lists the server home and /cd ~ succeeds. The new server and TUI tests pass, typecheck is clean in tui, server and protocol, and regenerating the client gives no diff.
Public API: adds an optional paths.home field to the server.info response (ServerInfo in protocol, and the generated ServerInfo/ServerInfoOutput client types). This is backwards compatible: existing clients ignore the field, and the TUI falls back to its own home when an older server doesn't send it.
Concerns, inline:
/cdinside a session now depends on a successfulserver.infocall, even for input without~.- The new test can be trimmed.
Other home-relative displays (the prompt footer's location label, the open and workspace dialogs, the path-format context) still use the client home. They don't break anything and could be a follow-up.
Related: #47665
| const sessionID = props.sessionID | ||
| const session = sessionID ? data.session.get(sessionID) : undefined | ||
| const sourceProjectID = session?.projectID ?? data.location.info()?.project.id | ||
| const home = await client.resolveServerHome().catch((error) => { |
There was a problem hiding this comment.
This makes every /cd wait for server.info() and abort with a toast if it fails, including inside a session, where session.move gets the raw input and the server already expands ~ itself. Could this lookup run only when the input starts with ~? Or, in the session branch, could a failure fall back instead of blocking the move? Then a server-info hiccup won't stop plain /cd ../foo.
There was a problem hiding this comment.
Addressed in 2ed36eb and covered by 82f5925. Only non-session tilde changes now await server-home metadata; plain /cd and all session moves bypass that dependency. Three rendered-command regressions use server.info = 503 and verify relative home-screen changes plus raw relative/tilde session.move payloads. They fail before this change and pass afterward.
| expect(home).not.toBe("/Users/remote-client") | ||
| const completions = yield* Effect.promise(() => directoryAutocomplete(api.file, { directory: current }, "~", home)) | ||
| expect(completions[0]).toEqual({ value: "~", absolute: home }) | ||
| const destination = yield* Effect.promise(() => api.location.get({ location: { directory: home } })) |
There was a problem hiding this comment.
Most of this test calls helpers (directoryAutocomplete, location.get, directoryRecentValue) with a home passed in by hand, so it doesn't exercise the new /cd or autocomplete wiring. expect(setup.renderer.isDestroyed).toBe(false) and the /Users/remote-client/project assertion check nothing related to the fix. Could you cut it down to the core check, that resolveServerHome() returns the server's home rather than the client paths.home, and drop the rest?
There was a problem hiding this comment.
Addressed in b78aa18. The real-server/client-context test now checks only that resolveServerHome returns the server home despite a different client paths.home. Removed the manually parameterized helper checks and renderer-liveness assertion. Command wiring is covered separately by the rendered regressions in 82f5925.
There was a problem hiding this comment.
Thanks, confirmed in b78aa18: the test now checks only that resolveServerHome() returns the server home, which resolves this thread. The one open point is the separate comment on the new app-lifecycle.test.tsx cases (their size and the 2s sleep race).
|
Review feedback addressed in 82f5925: metadata failures no longer block plain /cd or session moves, and the server-home context test is trimmed. Full TUI suite: 1,504 passed, 2 skipped; the three new command regressions fail before the fix and pass after it. Local typecheck has identical pre-existing diagnostics on the unmodified branch; bun run check stops at an oxlint allocator panic. The published tree matches the tested code. Could a maintainer approve the pending workflows and request a fresh agent review? Other home-relative display changes are left for a follow-up, as suggested. |
There was a problem hiding this comment.
Both earlier points are resolved. /cd now waits on server.info only for home-screen ~ input; relative paths and session moves no longer depend on it. server-home.test.tsx is down to the one check that matters. I re-ran the #55171 reproduction on the new head: /cd ~/ autocomplete lists the server home and /cd ~ succeeds. The TUI tests and typecheck pass.
Public API: unchanged since my last review, still only the optional paths.home on the server.info response (backwards compatible).
One remaining point: the new app-lifecycle test is heavy for what it checks (inline).
Related: #47665
| import { createAppFixture } from "./fixture/app" | ||
| import type { PluginInfo } from "@opencode/client" | ||
|
|
||
| test.each([ |
There was a problem hiding this comment.
This is about 60 lines of hand-mocked routes and session fixtures, plus a 2s Bun.sleep race, all to cover a guard of a few lines. The race timeout could get flaky on slow CI. I'd keep a single case (the session tilde move with /api/info returning 503) and wait on the request without the sleep, or drop the test. The fix doesn't really need it.
There was a problem hiding this comment.
Kept one session-tilde move case and removed the two-second race. It now awaits the move request directly. The separate short completion case covers the newly reported relative-autocomplete bug, not another copy of the command guard. Final TUI suite passes with 1,505 tests and 2 skipped.
|
Independent verification + a few review findings from a parallel fix attempt of this bug (we confirmed the macOS→Linux A few things our review run caught that look like they apply here:
For what it's worth, we also hit and confirmed the same subtlety you avoided here: yielding |
Modify home resolution logic based on input base path.
|
Addressed the remaining review feedback in a608e3a. The command regression is reduced to one session-tilde case with no sleep race. Regenerated protocol and website OpenAPI snapshots, fixed relative autocomplete during metadata failure, and normalized empty server-home values. Full TUI suite passes with 1,505 tests and 2 skipped; server-process tests, generated-file checks, and website typecheck/build pass. Local TUI typecheck is memory-limited, and the root check still hits an oxlint allocator panic. The published tree matches the tested tree. Session-recents warm-up and other display paths remain explicit nonblocking follow-ups. |
There was a problem hiding this comment.
Still fixes #55171: I re-ran the reproduction on this head. /cd ~/ autocomplete lists the server home and /cd ~ succeeds. The TUI tests pass (40/40), but typecheck in packages/tui now fails because of the new server-home test (inline).
Resolved since last review: the app-lifecycle tests are now small and don't use the sleep race, and relative autocomplete no longer waits on server.info. The refreshed OpenAPI snapshots, including the Provider.Encoding entries, are needed: check:generated fails on the base and passes here.
Public API: adds the optional paths.home to the server.info response, the OpenAPI spec, and the generated ServerInfo/ServerInfoOutput client types. This is backwards compatible.
Related: #47665
| test.each([undefined, ""])("treats an unavailable server home (%j) as missing", async (home) => { | ||
| const server = Bun.serve({ | ||
| port: 0, | ||
| fetch: createFetch( |
There was a problem hiding this comment.
This breaks bun run typecheck in packages/tui (TS2322): createFetch(...).fetch has the client fetch(input, init) signature, not Bun.serve's (req, server) handler. Neither server code path sends an empty home, so this test and the || undefined in client.tsx aren't really needed. I'd drop them, which also fixes the typecheck.
There was a problem hiding this comment.
Removed both speculative boundary cases and empty-home normalization as suggested. The real-server regression and relative autocomplete coverage remain
There was a problem hiding this comment.
Thanks, confirmed in 92cb614: the empty-home test and the || undefined in client.tsx are gone, and server-home.test.tsx no longer has the Bun.serve call behind the TS2322 error. I only read the code and didn't re-run typecheck, so it's worth confirming that bun run typecheck in packages/tui passes on this head.
Removed a test case that checks for unavailable server home handling.
|
Addressed the latest agent review in 92cb614: removed the speculative empty-home handling and incorrectly typed boundary tests. Retained the real-server and relative-autocomplete regressions |
|
Bump. This is such an annoying bug. It'll be greatly appreciated if maintainer can take action. |
|
Automated PR Cleanup Thank you for contributing to opencode. Due to the high volume of PRs from users and AI agents, we periodically close older PRs using automated criteria so maintainers can focus review time on the most active and community-supported contributions. This PR was closed because it matched the following cleanup criteria:
PRs created within the last month are not affected by this cleanup. If you believe this PR was closed incorrectly, or if you are still actively working on it, please leave a comment explaining why it should be reopened. A maintainer can review and reopen it if appropriate. Thanks again for taking the time to contribute. |
Issue for this PR
Closes #55171
Type of change
What does this PR do?
Use the server's home for
/cd ~,~/...autocomplete, and recent directory paths, rather than the attached client's home. Add optionalpaths.hometo server info, cache it per client, and regenerate API clients. Older servers retain the existing fallback; session moves still send raw input for server-side resolution. Following review, only tilde input awaits server-home metadata. Plain directory changes, session moves, and relative directory autocomplete no longer depend on server.info succeeding. Home-relative autocomplete still fails closed when server metadata cannot be read, rather than substituting a remote client home.How did you verify your code works?
bun run checkstill stops at an oxlint allocator panic. These are not counted as passing checks.No model/API credits used. Native macOS-to-Linux interactive testing was not performed.
Nonblocking follow-up
Session recent-directory bookkeeping can still use the client-home fallback before server-home metadata is cached. The move itself remains server-resolved. Other home-relative display changes remain outside this fix.
Screenshots / recordings
No visual layout changes. Regression coverage uses the real TUI renderer and HTTP server with different client/server home directories.
Checklist
Added by the opencode bot (AI-generated).
Related: #47665
Public API: adds optional
paths.hometo the server.info response, OpenAPI spec and generated client types (backwards compatible)