feat(agent): unify session env via EnvInfoer - #26099
Merged
Merged
Conversation
SSH session tests need to inject a deterministic shell and environment to replace the fragile bufio.Scanner and blind-write patterns behind the flaky failures in coder/internal#1560. Expose an EnvInfo seam on agent.Options and agentssh.Config so a test can supply its own usershell.EnvInfoer. Production leaves it unset, so NewServer falls back to SystemEnvInfo and the resolved shell, environment, and working directory stay unchanged.
Member
Author
|
/coder-agents-review |
Contributor
|
Chat: Review in progress | View chat deep-review v0.7.1 | Round 1 | Last posted: Round 1, 5 findings (1 P3, 2 Nit, 2 Note), COMMENT. Review Finding inventoryFindings
Round logRound 1Panel. 0 P0-P2, 1 P3, 2 Nit, 2 Note. 3 dropped. Reviewed against eac7ee4..def4d8a. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
johnstcn
approved these changes
Jun 5, 2026
johnstcn
left a comment
johnstcn
left a comment
Member
There was a problem hiding this comment.
LGTM! My only regret is that I named it EnvInfoer and not something better. 🤦
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Clean seam work. The injection path from agent.Options.EnvInfo through agentssh.Config.EnvInfo to sessionStart is correct, production behavior is unchanged, and the test proves the full chain. 14 reviewers found no bugs; findings are minor.
Severity count: 0 P0, 0 P1, 0 P2, 1 P3, 2 Nit, 2 Note.
"Boring code. Exactly what stage-one seam work should be. ♦" (Hisoka)
🤖 This review was automatically generated with Coder Agents.
The EnvInfo seam stopped at agentssh. agentproc resolved its working-directory home fallback via os.UserHomeDir directly, so an injected EnvInfo could not reach the process API the way it reaches SSH sessions. The stacked working-directory unification needs that path to honor EnvInfo. Thread a usershell.EnvInfoer through NewAPI and newManager and use m.envInfo.HomeDir() for the home fallback in resolveWorkDir. With EnvInfo nil (the production default), SystemEnvInfo.HomeDir() resolves the same home, so behavior is unchanged.
mafredri
changed the title
Reorder NewAPI and newManager parameters so the backing dependencies (pathStore, envInfo) precede the updateEnv and workingDir callbacks. This groups the env-resolution inputs source-first ahead of the closures that consume them. No behavior change.
Drop the parenthetical re-listing EnvInfoer's method set and the redundant host-environment restatement. Keep the load-bearing claims: nil default to usershell.SystemEnvInfo, tests-only, and the per-session container override. Addresses review findings CRF-1 and CRF-2.
mafredri
marked this pull request as ready for review
June 5, 2026 18:16
mafredri
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SSH session tests lean on fragile
bufio.Scannerand blind-write patterns againstNewSession, which is the source of the flaky bare-EOF failures tracked in coder/internal#1560. The fix is a deterministic, protected test tool, landed in stages. This PR is stage one: the seam the tool depends on.It adds an injectable
usershell.EnvInfoertoagent.Optionsandagentssh.Config.sessionStartnow sources the session's user, shell, home directory, and environment from it, andNewServerdefaults it tousershell.SystemEnvInfo. Production never sets the field, so the resolved shell, environment, and working directory stay identical to before. The container override forExperimentalContainersstill takes precedence per session. The only new capability is that a test can force a deterministic shell.The same
EnvInfoeris threaded into the process API (agentproc), whose working-directory home fallback previously calledos.UserHomeDirdirectly.NewAPIandnewManagernow take anEnvInfoer(nil defaults toSystemEnvInfo), andresolveWorkDiruses it for the home fallback. This keeps the seam consistent across SSH and the process API, and lets the stacked working-directory unification honor injected EnvInfo instead of hardcodingSystemEnvInfo. Production behavior is unchanged.TestAgent_SSHEnvInfoShellcovers the path fromagent.Options.EnvInfoto the executed session command, andTestStartProcess/DefaultWorkDirUsesInjectedEnvInfoHomeproves the injected home is honored as the process working-directory fallback.Implementation plan and where this PR fits
The redesign replaces fragile
bufio.Scannerand blind-write SSH session test patterns with a deterministic, protected, composable tool. It ships in stages:agentsshand theagentprocprocess API.resolveWorkingDirstat-validation bug fix, unifying SSH and process-API working-directory resolution.agenttestsession driver plususershelltest.EnvInfo, with a sentinel-prompt readiness mechanism validated on Linux and Windows.NewSessionsites and the cliptytestpath, by area.Locked decisions relevant here:
usershell.EnvInfoer. The default staysSystemEnvInfo.PR1 scope: add
EnvInfotoagentssh.Configandagent.Options, default it inNewServer, source it insessionStart, thread it intoagentproc.NewAPI, and assert that an injectedShell()changes the session shell and an injectedHomeDir()changes the process working directory.