Repository navigation
fix(server): resolve the configured shell for session terminals - #61353
WuJunkai2004 wants to merge 4 commits into
Conversation
Session terminals resolved their shell via ShellSelect.environment(), which only reads process.env.SHELL, so the shell config option was ignored (always PowerShell on Windows). Resolve through the Location-scoped ShellSelect.Service in the persistentPty.create handler instead, matching the agent shell tool and pty.ts. Fixes anomalyco#51051
![opencodejr[bot]](https://avatars.githubusercontent.com/in/5497873?s=60&v=4){kind=small}
There was a problem hiding this comment.
This doesn't work yet: with this change, every request to create a session terminal fails with a 500.
In packages/server, bun test test/persistent-pty.test.ts passes 7/7 on the base and fails 8/8 with this PR, including the new test. tsc -b (the typecheck:tsc script) goes from 0 errors on the base to 89 errors across the server tests. Each one says ShellSelect.Service is required but never provided. The typecheck script itself (bun check) fails on the base too, because the script isn't found.
The cause is that persistentPty.create isn't behind any location middleware. Unlike the config.* routes this was modelled on, nothing provides the location-scoped ShellSelect.Service there (see inline). I couldn't drive the TUI repro on either checkout: the simulated model's reply never arrived.
Public API: unchanged.
| .handle( | ||
| "persistentPty.create", | ||
| Effect.fn(function* (ctx) { | ||
| const shell = yield* ShellSelect.Service |
There was a problem hiding this comment.
persistentPty.create has no location middleware (see protocol/src/groups/persistent-pty.ts), so the location-scoped ShellSelect.Service isn't provided here. This yield* fails every create with a 500, even when a command is sent, and the R requirement leaks into the routes layer, which is where the 89 tsc errors come from. The shell needs to be resolved inside the session's location. Two ways: attach SessionLocationMiddleware to this endpoint, or provide the session's instance (as middleware/session-location.ts does with instances.provide(session)) before yielding ShellSelect.Service. Resolving it lazily only when ctx.payload.command is undefined would also keep explicit-command terminals independent of it.
There was a problem hiding this comment.
Addressed in ba936f3 — the handler now provides the session's instance itself (same as middleware/session-location.ts: sessionInfo → instances.provide → ShellSelect.Service) and awaits plugin activation before resolving, since the config→shell state is populated asynchronously. Kept it lazy, so explicit commands never resolve, and best-effort: terminals whose session can't be resolved keep the environment shell, which a couple of the existing tests depend on. The full persistent-pty suite passes 8/8 on Linux with the new test included, and server typecheck:tsc matches the base error set.
Review on the first revision showed ShellSelect.Service is not provided to the persistentPty handlers: the experimental endpoints carry no location middleware, so the yield failed every terminal create and leaked into the routes layer. Resolve in the session's location instead, the same way middleware/session-location.ts does: sessionInfo, then instances.provide, then ShellSelect.Service, awaiting plugin activation so the config-to-shell state is populated. Resolution is lazy (explicit commands never enter it) and best effort: terminals whose session cannot be resolved keep the environment shell, which the existing tests depend on. Fixes anomalyco#51051
|
Good catch — you were right on all three points. Reworked in ba936f3: the handler now resolves the shell in the session's location itself (sessionInfo → instances.provide → ShellSelect.Service, mirroring middleware/session-location.ts), and only when the payload has no command. Resolution is best effort — terminals whose session can't be resolved keep the environment shell, which a few existing tests depend on (they create terminals on session IDs that were never registered). One thing that took a while to find on top of your notes: the config→shell state is populated by plugin activation, which is asynchronous, so the resolve has to await plugin activation first — without that it silently returned the environment shell in my Linux runs. The full persistent-pty suite now passes 8/8 on Linux (7 pre-existing + the new regression test, which is skipped on Windows hosts), and server typecheck:tsc reports the same error set as the unmodified base. Protocol is back to untouched — no API surface change anymore. |
There was a problem hiding this comment.
The 500 on terminal creation is fixed: persistent-pty.test.ts passes 8/8, and the new test fails on the base, where the terminal gets /bin/sh.
One thing still needs fixing: tsc -b in packages/server goes from 0 errors on the base to 89 with this PR. The services that leak into the routes layer are now Session.Service and Instance.Service, because they're yielded inside the per-request function (see inline). Locally, yielding them once in the group's setup brings it to 0 errors with all tests still passing.
Public API: unchanged.
| // middleware/session-location.ts provides them. Resolution is best effort: terminals whose session or | ||
| // location cannot be resolved keep the process-global environment shell, as before this change. | ||
| const configuredShell = Effect.fn("PersistentPty.configuredShell")(function* (sessionID: Session.ID) { | ||
| const sessions = yield* Session.Service |
There was a problem hiding this comment.
Yielding Session.Service and Instance.Service inside the per-request function adds them as HttpRouter.Request<"Requires", …> requirements of the routes layer. That's where the 89 tsc -b errors in the server tests come from now (the base has 0). Yield them once in the group's outer Effect.gen, next to PtyTicket.Service, and close over them here. I tried that locally: it gives 0 type errors and 8/8 in persistent-pty.test.ts.
There was a problem hiding this comment.
Done in cbaefe4 — Session.Service and Instance.Service are now yielded once in the group's setup next to PtyTicket.Service, and configuredShell closes over them. typecheck:tsc is clean end to end now, and the persistent-pty suite stays at 8/8 on Linux. (For the record, the 89 errors were never Windows-specific noise on my side either — an earlier base comparison I ran was contaminated by an already-committed leak, which is why I misread it as environmental. Your 0-on-base reading was the correct one.)
Yielding Session.Service and Instance.Service inside the per-request resolution added them as requirements of the routes layer (89 tsc -b errors in the server tests on an otherwise clean base). Yield them once in the group's setup next to PtyTicket.Service and close over them instead. Server typecheck:tsc is clean end to end now, and the persistent-pty suite stays at 8/8 on Linux.
There was a problem hiding this comment.
Both earlier points are addressed. Session terminal creation no longer returns a 500, and tsc -b in packages/server is back to 0 errors, the same as the base. persistent-pty.test.ts passes 8/8, and the new test fails on the base (the terminal gets /bin/sh instead of the configured shell). I didn't find any other issues.
Public API: unchanged.
There was a problem hiding this comment.
The new commit only merges dev; the change itself is the same as before. On top of the new dev, persistent-pty.test.ts passes 8/8 and tsc -b in packages/server reports 0 errors. Both earlier points stay addressed, and I found nothing new.
Public API: unchanged.
Issue for this PR
Closes #51051
Type of change
What does this PR do?
Session terminals resolve their shell through
ShellSelect.environment(), which only readsprocess.env.SHELL. That variable is normally unset on Windows, so the configuredshellis ignored and terminals open PowerShell. The agent's shell tool resolves through the Location-scoped service and honors the configuredshell; only terminals diverged (regressed in 2.2.4 when terminals moved to persistent PTYs in #60346).ShellSelectis Location-scoped: it is only provided to handlers by the location middleware, and the experimental persistent-pty endpoints carry no middleware (unlikeconfig.*, which the first revision of this patch modelled itself on — that leakedShellSelect.Serviceinto the routes layer and 500'd every create; thanks to the reviewer for catching it).So this resolves the shell inside the terminal's session location, the same way
middleware/session-location.tsdoes:sessionInfo()→instances.provide(session)→ShellSelect.Service, awaiting plugin activation first so the config→shell state is populated. Resolution happens lazily only whenpayload.commandis absent (explicit commands are untouched) and is best effort: a terminal whose session or location cannot be resolved keeps the environment shell, matching the old behavior (some existing tests create terminals on session IDs that were never registered).How did you verify your code works?
Added a regression test in
packages/server/test/persistent-pty.test.ts: it configures a shell via a projectopencode.json, creates a session at that location, creates a terminal without acommand, and assertsterminal.commandis the configured path and that the terminal actually ran it. All seven pre-existing tests pass an explicitcommand, so they never enter the new resolution path.Verified on a Linux box (Debian, bun 1.4.4, frozen lockfile): the full
persistent-pty.test.tssuite passes 8/8 (7 pre-existing + the new one, which is skipped on Windows hosts), andturbo run typecheck:tsc --filter=@opencode/serverreports the same error set as the unmodified base.Checklist
Added by the opencode bot (AI-generated).
Related: #51994, #57310