diff --git a/CHANGELOG.md b/CHANGELOG.md index 76c4471..d317270 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,40 @@ +## 1.57.2 + +A shell now opens where you would expect it to: the user's home. + +### Fixed + +- **A session starts in the user's home, not wherever the node was launched.** + `omnyshell node start` already did this — but in the CLI rather than in the + backend, so any embedder building its own `ProcessShellBackend` (as + OmnyServer does) got neither. Sessions opened in the node process's own + working directory, which for an agent installed as a service is wherever its + binary lives: `/usr/local/bin`. + + The decision now lives in one place, `resolveStartDirectory`, in the order + that matches who is entitled to make it: what the client asked for, then what + the node was configured with, then the user's home, and only then the node's + own directory. `exec` follows the same rule as an interactive shell, so the + two agree — and `ssh` set that expectation long ago. + + A home that does not exist is skipped rather than used: handing a missing + path to `Process.start` fails the session outright, which is worse than + opening somewhere unremarkable. + +### Changed + +- All three backends (pipe, `script` PTY, winpty) resolve their working + directory through that one function. They had drifted: the `script` PTY + backend — the default on Linux and macOS — never expanded a leading `~`, so + `--cwd ~/project` worked on the other two and not on it. The Windows MSYS + path translation was duplicated across two of them, and is now applied once, + where the decision is made. + +- `_resolveNodeHome` in the CLI keeps only what is particular to it — letting + the node profile's `HOME` override the process environment — and defers the + rest to `existingUserHome`, so the password-database lookup a service-run + node needs is not reimplemented beside it. + ## 1.57.1 A shell on a node run as a service had no `$HOME`. diff --git a/bin/omnyshell.dart b/bin/omnyshell.dart index 4e1f99f..503415d 100644 --- a/bin/omnyshell.dart +++ b/bin/omnyshell.dart @@ -29,6 +29,7 @@ import 'package:omnyshell/src/domain/entities/platform_info_io.dart'; import 'package:omnyshell/src/infrastructure/auth/node_git_credentials.dart'; import 'package:omnyshell/src/infrastructure/identity/certificate_names.dart'; import 'package:omnyshell/src/infrastructure/tls/ca_pinning.dart'; +import 'package:omnyshell/src/shared/utils/omnyshell_home.dart'; Future main(List args) async { final runner = @@ -2088,18 +2089,15 @@ String _resolveNodeShell(String? override) { return (shell != null && shell.trim().isNotEmpty) ? shell : '/bin/sh'; } -/// Resolves the node user's home directory — preferring the profile's `HOME`, -/// then the node process environment — used as the default working directory of -/// new sessions. Returns `null` when it cannot be resolved or does not exist, so -/// sessions fall back to the node's own cwd. -String? _resolveNodeHome(Map env) { - final home = - env['HOME'] ?? - Platform.environment['HOME'] ?? - Platform.environment['USERPROFILE']; - if (home == null || home.trim().isEmpty) return null; - return Directory(home).existsSync() ? home : null; -} +/// Resolves the node user's home directory, letting the node profile's `HOME` +/// override the process environment. +/// +/// The backends fall back to the user's home by themselves now, so this exists +/// only for that override. `existingUserHome` does the resolving — including +/// the password-database lookup a service-run node needs, and the check that +/// the directory is actually there. +String? _resolveNodeHome(Map env) => + existingUserHome(environment: {...Platform.environment, ...env}); /// Resolves the optional working-directory positional of `omnyshell local` and /// `omnyshell ide` ([command] names the one being run, for the error messages). diff --git a/lib/src/infrastructure/backend/process_shell_backend.dart b/lib/src/infrastructure/backend/process_shell_backend.dart index 00fc58c..ce8356b 100644 --- a/lib/src/infrastructure/backend/process_shell_backend.dart +++ b/lib/src/infrastructure/backend/process_shell_backend.dart @@ -50,16 +50,12 @@ class ProcessShellBackend implements ShellBackend { } final (executable, args) = resolveShellInvocation(request, defaultShell); - // Resolve a leading `~` (the client may pass `~/...` as the working dir, e.g. - // an ephemeral `run`/drive mount path) against the node user's home. On - // Windows, also translate an MSYS cwd (`/c/...`, as Git Bash reports `$PWD` - // and as TAB-completion's one-shot exec reuses it) into a Windows path - // `Process.start` can actually `chdir` into. - final raw = request.cwd; - var cwd = raw != null ? expandUserHome(raw) : workingDirectory; - if (Platform.isWindows && cwd != null && cwd.startsWith('/')) { - cwd = windowsPathFromMsys(cwd); - } + // Client's choice, then the node's, then the user's home — and only then + // wherever this process happens to be standing. See `resolveStartDirectory`. + final cwd = resolveStartDirectory( + requested: request.cwd, + configured: workingDirectory, + ); final process = await Process.start( executable, args, diff --git a/lib/src/infrastructure/backend/pty/script_pty_shell_backend.dart b/lib/src/infrastructure/backend/pty/script_pty_shell_backend.dart index e5555ee..04d6bef 100644 --- a/lib/src/infrastructure/backend/pty/script_pty_shell_backend.dart +++ b/lib/src/infrastructure/backend/pty/script_pty_shell_backend.dart @@ -93,7 +93,10 @@ class ScriptPtyShellBackend implements ShellBackend { final process = await Process.start( script, args, - workingDirectory: request.cwd ?? workingDirectory, + workingDirectory: resolveStartDirectory( + requested: request.cwd, + configured: workingDirectory, + ), // See `ProcessShellBackend`: a node run as a service has no `HOME`, and // a PTY session inherits that unless something fills it in. environment: withUserHome({ diff --git a/lib/src/infrastructure/backend/pty/winpty_shell_backend.dart b/lib/src/infrastructure/backend/pty/winpty_shell_backend.dart index 1cbb593..8ce8bf0 100644 --- a/lib/src/infrastructure/backend/pty/winpty_shell_backend.dart +++ b/lib/src/infrastructure/backend/pty/winpty_shell_backend.dart @@ -8,7 +8,6 @@ import '../../../domain/backend/pty_spec.dart'; import '../../../domain/backend/shell_backend.dart'; import '../../../domain/backend/shell_request.dart'; import '../../../domain/backend/shell_session.dart'; -import '../../../shared/utils/omnyshell_home.dart'; import '../shell_invocation.dart'; import 'winpty_ffi.dart'; import 'winpty_shell_session.dart'; @@ -105,9 +104,14 @@ class WinptyShellBackend implements ShellBackend { final inner = 'stty -echo 2>/dev/null; exec bash /dev/stdin'; final cmdline = '"$bash" -c "$inner"'; - // Resolve the working directory to a Windows path winpty can chdir into. - var cwd = expandUserHome(request.cwd ?? workingDirectory ?? ''); - if (cwd.startsWith('/')) cwd = windowsPathFromMsys(cwd); + // Client's choice, then the node's, then the user's home — already resolved + // to a path winpty can chdir into. See `resolveStartDirectory`. + final cwd = + resolveStartDirectory( + requested: request.cwd, + configured: workingDirectory, + ) ?? + ''; final env = { ...Platform.environment, diff --git a/lib/src/infrastructure/backend/shell_invocation.dart b/lib/src/infrastructure/backend/shell_invocation.dart index d26c5b3..5ee2bed 100644 --- a/lib/src/infrastructure/backend/shell_invocation.dart +++ b/lib/src/infrastructure/backend/shell_invocation.dart @@ -3,6 +3,7 @@ import 'dart:io'; import '../../domain/backend/shell_family.dart'; import '../../domain/backend/shell_request.dart'; import '../../domain/entities/session.dart'; +import '../../shared/utils/omnyshell_home.dart'; /// Resolves how a [ShellRequest] maps to an executable and arguments, shared by /// the pipe-based and PTY-based backends so both honour the same rules: @@ -145,6 +146,38 @@ String windowsPathFromMsys(String path) { return '$drive:\\${rest.replaceAll('/', r'\').replaceFirst(RegExp(r'^\\'), '')}'; } +/// The directory a session starts in, in order of who gets to decide. +/// +/// 1. [requested] — what the client asked for, which always wins. A leading `~` +/// is expanded; a directory that does not exist fails the spawn, loudly, +/// which is the right answer to an explicit request that cannot be honoured. +/// 2. [configured] — what the node was built with, for an embedder with an +/// opinion about where its sessions belong. +/// 3. The user's home. A shell that opens wherever the node process happens to +/// be standing — `/usr/local/bin`, for an agent installed as a service — is +/// nobody's idea of a starting point, and `ssh` set the expectation long +/// ago. Skipped when that directory does not exist: handing a missing path +/// to `Process.start` would fail the session outright. +/// 4. `null` — inherit the node's own working directory, as before. +/// +/// On Windows an MSYS path (`/c/Users/x`, the form Git Bash reports in `$PWD`) +/// is translated into one `chdir` accepts; see [windowsPathFromMsys]. +String? resolveStartDirectory({ + String? requested, + String? configured, + Map? environment, +}) { + final explicit = requested ?? configured; + final chosen = (explicit != null && explicit.trim().isNotEmpty) + ? expandUserHome(explicit) + : existingUserHome(environment: environment); + if (chosen == null) return null; + + return Platform.isWindows && chosen.startsWith('/') + ? windowsPathFromMsys(chosen) + : chosen; +} + /// The first *usable* Windows bash (Git Bash / WSL), or `null` when none works. /// Public shim over [_resolveWindowsBash] so the winpty backend can reuse the /// same validated bash the interactive session launches. diff --git a/lib/src/shared/utils/omnyshell_home.dart b/lib/src/shared/utils/omnyshell_home.dart index 74d74cb..396a593 100644 --- a/lib/src/shared/utils/omnyshell_home.dart +++ b/lib/src/shared/utils/omnyshell_home.dart @@ -78,6 +78,25 @@ int? _selfUid(String procStatusPath) { return null; } +/// The user's home directory, but only when it exists on disk. +/// +/// The distinction matters wherever the answer is about to be used as a +/// working directory: `Process.start` fails outright on one that is not there, +/// so a home that has been resolved but never created is worse than no answer. +String? existingUserHome({ + Map? environment, + String passwdPath = '/etc/passwd', + String procStatusPath = '/proc/self/status', +}) { + final home = resolveUserHome( + environment: environment, + passwdPath: passwdPath, + procStatusPath: procStatusPath, + ); + if (home == null || home.trim().isEmpty) return null; + return Directory(home).existsSync() ? home : null; +} + /// Returns [environment] with `HOME` filled in, when nothing else supplies one. /// /// A shell without `HOME` is subtly broken rather than obviously so: `cd ~` diff --git a/lib/src/version.dart b/lib/src/version.dart index 017c901..1a11ba5 100644 --- a/lib/src/version.dart +++ b/lib/src/version.dart @@ -3,4 +3,4 @@ /// This is the single source of truth for "what build is this": it is rendered /// in the CLI banner and is the default a node reports as its /// [NodeConfig.agentVersion] / [PlatformInfo.agentVersion]. -const String omnyShellVersion = '1.57.1'; +const String omnyShellVersion = '1.57.2'; diff --git a/pubspec.yaml b/pubspec.yaml index 6376e13..acaee30 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -4,7 +4,7 @@ description: >- connect to a Hub by node identity (not host:port); the Hub authenticates, authorizes and brokers encrypted sessions to Nodes over WebSocket-on-TLS. Ships Hub, Node, Client and CLI implementations behind first-class Dart APIs. -version: 1.57.1 +version: 1.57.2 repository: https://github.com/OmnyGrid/omnyshell environment: diff --git a/test/unit/backend/process_shell_backend_test.dart b/test/unit/backend/process_shell_backend_test.dart index f7147b9..44d9286 100644 --- a/test/unit/backend/process_shell_backend_test.dart +++ b/test/unit/backend/process_shell_backend_test.dart @@ -85,5 +85,29 @@ void main() { other.resolveSymbolicLinksSync(), ); }); + + // With nothing configured a session used to open wherever the node process + // happened to be standing — `/usr/local/bin` for an agent installed as a + // service, which is nobody's idea of a starting point. `ssh` set that + // expectation long ago, and `exec` follows the same rule so the two agree. + test('starts in the user home when nothing is configured', () async { + final home = Platform.environment['HOME']; + if (home == null || !Directory(home).existsSync()) { + markTestSkipped('no usable HOME in this environment'); + return; + } + + for (final mode in [SessionMode.exec, SessionMode.shell]) { + final session = await ProcessShellBackend().start( + ShellRequest(mode: mode, command: 'pwd'), + ); + final out = (await collect(session.stdout)).trim(); + expect( + Directory(out).resolveSymbolicLinksSync(), + Directory(home).resolveSymbolicLinksSync(), + reason: 'a $mode session should open at home', + ); + } + }); }, skip: Platform.isWindows ? 'POSIX shell semantics' : null); } diff --git a/test/unit/backend/shell_invocation_test.dart b/test/unit/backend/shell_invocation_test.dart index 67321f7..f2d4383 100644 --- a/test/unit/backend/shell_invocation_test.dart +++ b/test/unit/backend/shell_invocation_test.dart @@ -1,3 +1,5 @@ +import 'dart:io'; + import 'package:omnyshell/omnyshell.dart'; import 'package:omnyshell/src/infrastructure/backend/shell_invocation.dart'; import 'package:test/test.dart'; @@ -126,4 +128,68 @@ void main() { expect(args, const ['-v']); }); }); + + // Who decides where a session opens. Before this, an unconfigured backend + // left it to wherever the node process was standing — `/usr/local/bin` for an + // agent installed as a service. + group('resolveStartDirectory', () { + final home = Platform.environment['HOME']; + final hasHome = home != null && Directory(home).existsSync(); + + test('the client asked, so the client wins', () { + expect( + resolveStartDirectory(requested: '/srv/app', configured: '/opt/node'), + '/srv/app', + ); + }); + + test('then what the node was built with', () { + expect(resolveStartDirectory(configured: '/opt/node'), '/opt/node'); + }); + + test('a blank choice is no choice', () { + // An empty string handed to `Process.start` is not "the current + // directory", it is a failure — so it has to fall through like a null. + if (!hasHome) { + markTestSkipped('no usable HOME in this environment'); + return; + } + expect(resolveStartDirectory(configured: ' '), home); + expect(resolveStartDirectory(requested: ''), home); + }); + + test('otherwise the user home', () { + if (!hasHome) { + markTestSkipped('no usable HOME in this environment'); + return; + } + expect(resolveStartDirectory(), home); + }); + + test('but never a home that is not there', () { + // Guessing a missing directory into `Process.start` fails the session + // outright, which is worse than opening somewhere unremarkable. + final dir = Directory.systemTemp.createTempSync('omnyshell-nohome-'); + addTearDown(() => dir.deleteSync(recursive: true)); + + expect( + resolveStartDirectory( + environment: {'HOME': '${dir.path}/was-never-created'}, + ), + isNull, + reason: 'null means "inherit the node cwd", which at least exists', + ); + }); + + test('expands a leading ~ in an explicit choice', () { + if (!hasHome) { + markTestSkipped('no usable HOME in this environment'); + return; + } + expect( + resolveStartDirectory(requested: '~/projects'), + '$home${Platform.pathSeparator}projects', + ); + }); + }); }