fix(env): harden the PATH preset per review (T-511)

Three holes from the T-511 adversarial review pass:

- An entry containing the PATH separator smuggled extra tokens into
  the joined PATH — a stray trailing ':' yields an EMPTY token, which
  POSIX shells resolve as CWD (the dot-in-PATH hazard). The CLI verb
  and the settings control now reject such entries, and applyPathPreset
  skips malformed stored values that predate the check.
- The gitdir pointer a worktree resolution follows is repo-controlled
  text; the resolved main root is now validated (must hold a real
  .git directory) before its preset key is trusted, so a crafted
  pointer can't alias an arbitrary path's preset.
- A pane spawned with a cwd below the workspace root hashed the
  subdirectory and silently missed the workspace preset; the lookup
  now keys any in-workspace cwd to the workspace root
  (presetLookupRoot).

Also: the Add button pairs buttonBackground with its own
buttonHoverBackground token instead of borrowing the list-item hover
token, and the hosted-Claude leg gains an end-to-end orchestrator test
(preset lookup → spawn env).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

lib/builtin/tools_settings/src/path_preset_control.dart

@@ -77,6 +77,10 @@ class _PathPresetControlState extends State<PathPresetControl> {
       d = d == '~' ? home : '$home${d.substring(1)}';
     }
     if (d.isEmpty || !(d.startsWith('/') || RegExp(r'^[A-Za-z]:[/\\]').hasMatch(d))) return null;
+    // One dir per entry — an embedded PATH separator would smuggle extra
+    // (possibly empty → CWD) tokens into the joined PATH.
+    final body = RegExp(r'^[A-Za-z]:').hasMatch(d) ? d.substring(2) : d;
+    if (body.contains(':') || body.contains(';')) return null;
     while (d.length > 1 && d.endsWith('/')) {
       d = d.substring(0, d.length - 1);
     }
@@ -293,7 +297,7 @@ class _PathPresetControlState extends State<PathPresetControl> {
             onTap: () => _add(services, cwd),
             builder: (ctx, hovered, _) => Container(
               padding: const EdgeInsets.symmetric(horizontal: 12, vertical: 5),
-              decoration: BoxDecoration(color: hovered ? tokens.listItemHoverBackground : tokens.buttonBackground, borderRadius: BorderRadius.circular(4)),
+              decoration: BoxDecoration(color: hovered ? tokens.buttonHoverBackground : tokens.buttonBackground, borderRadius: BorderRadius.circular(4)),
               child: ClideText(addLabel, color: tokens.buttonForeground, fontSize: clideFontCaption),
             ),
           ),

lib/main.dart

@@ -63,7 +63,7 @@ import 'package:clide/src/daemon/search_commands.dart';
 import 'package:clide/src/editor/registry.dart' show EditorRegistry;
 import 'package:clide/src/git/client.dart';
 import 'package:clide/src/cli/argv_dispatch.dart';
-import 'package:clide/src/env/path_preset.dart' show applyPathPreset, pathPresetKey, presetDirsFrom;
+import 'package:clide/src/env/path_preset.dart' show applyPathPreset, pathPresetKey, presetDirsFrom, presetLookupRoot;
 import 'package:clide/src/env/shell_env.dart' show primeLoginShellPath, resolvedToolPath;
 import 'package:clide/src/env/supporter_binaries.dart';
 import 'package:clide/widgets/src/icons/phosphor_glyphs.g.dart' show kPhosphorGlyphs;
@@ -322,7 +322,7 @@ Future<void> main() async {
         final settings = kernelSettings;
         final base = resolvedToolPath();
         if (settings == null) return base;
-        return applyPathPreset(base, presetDirsFrom((k) => settings.get<Object>(k), cwd ?? workRoot.path));
+        return applyPathPreset(base, presetDirsFrom((k) => settings.get<Object>(k), presetLookupRoot(cwd, workRoot.path)));
       },
     );
     // D-6 parity (T-219, D-83): make the tabs the user sees in the GUI

lib/src/daemon/env_path_commands.dart

@@ -174,6 +174,11 @@ Future<IpcResponse> _dispatch(
     }
     final absolute = d.startsWith('/') || RegExp(r'^[A-Za-z]:[/\\]').hasMatch(d);
     if (!absolute) return (out, 'not an absolute path: $raw');
+    // One dir per entry: an embedded PATH separator would expand into extra
+    // tokens at join time — and a stray trailing ':' yields an EMPTY token,
+    // which POSIX shells resolve as CWD (the classic dot-in-PATH hazard).
+    final body = RegExp(r'^[A-Za-z]:').hasMatch(d) ? d.substring(2) : d;
+    if (body.contains(':') || body.contains(';')) return (out, 'entry contains a PATH separator: $raw');
     while (d.length > 1 && d.endsWith('/')) {
       d = d.substring(0, d.length - 1);
     }

lib/src/env/path_preset.dart

@@ -29,8 +29,8 @@ const pathPresetKeyPrefix = 'app.env.pathPrepend.';
 /// The settings key holding [workspaceRoot]'s preset — app layer, suffixed
 /// with the FNV-1a hash (the same one D-70 derives for the socket path) of the
 /// [presetRootFor]-resolved repo root, so all worktrees of a repo share one key.
-String pathPresetKey(String workspaceRoot, {bool Function(String path)? isFile, String? Function(String path)? readFile}) =>
-    '$pathPresetKeyPrefix${fnv1a64Hex(canonicalWorkspaceKey(presetRootFor(workspaceRoot, isFile: isFile, readFile: readFile)))}';
+String pathPresetKey(String workspaceRoot, {bool Function(String path)? isFile, String? Function(String path)? readFile, bool Function(String path)? isDir}) =>
+    '$pathPresetKeyPrefix${fnv1a64Hex(canonicalWorkspaceKey(presetRootFor(workspaceRoot, isFile: isFile, readFile: readFile, isDir: isDir)))}';
 
 /// The directory whose identity keys the preset: the MAIN repo root when
 /// [workspaceRoot] is a linked git worktree, else [workspaceRoot] itself
@@ -42,11 +42,20 @@ String pathPresetKey(String workspaceRoot, {bool Function(String path)? isFile,
 /// that shape — a normal repo (`.git` directory), no `.git` at all, a
 /// submodule pointer — keys off [workspaceRoot] unchanged.
 ///
-/// [isFile]/[readFile] are injectable for tests; defaults touch the real fs.
-String presetRootFor(String workspaceRoot, {bool Function(String path)? isFile, String? Function(String path)? readFile}) {
+/// The pointer content is REPO-controlled, so the resolved target is
+/// validated before it is trusted: the candidate main root must actually
+/// hold a `.git` directory (a genuine repo), else the pointer is ignored and
+/// the workspace keys off itself. Without that check a crafted `.git` file
+/// could alias an arbitrary path's preset key (same-user only — the preset
+/// values themselves stay user-authored — but the boundary is cheap to hold).
+///
+/// [isFile]/[readFile]/[isDir] are injectable for tests; defaults touch the
+/// real fs.
+String presetRootFor(String workspaceRoot, {bool Function(String path)? isFile, String? Function(String path)? readFile, bool Function(String path)? isDir}) {
   final root = _stripTrailingSep(workspaceRoot);
   final probe = isFile ?? _isFile;
   final read = readFile ?? _readFile;
+  final dirProbe = isDir ?? _isDir;
   final gitPointer = '$root/.git';
   if (!probe(gitPointer)) return root;
   final content = read(gitPointer);
@@ -58,7 +67,21 @@ String presetRootFor(String workspaceRoot, {bool Function(String path)? isFile,
   const marker = '/.git/worktrees/';
   final idx = resolved.indexOf(marker);
   if (idx <= 0) return root;
-  return resolved.substring(0, idx);
+  final mainRoot = resolved.substring(0, idx);
+  if (!dirProbe('$mainRoot/.git')) return root;
+  return mainRoot;
+}
+
+/// The root to key a spawn-time preset lookup on: [workspaceRoot] when [cwd]
+/// is the workspace root or anywhere below it (a pane spawned in a subdir
+/// must share the workspace's preset), else [cwd] itself (a spawn in an
+/// unrelated directory keys off that directory's own repo).
+String presetLookupRoot(String? cwd, String workspaceRoot) {
+  final ws = _stripTrailingSep(workspaceRoot);
+  if (cwd == null || cwd.isEmpty) return ws;
+  final c = _stripTrailingSep(cwd);
+  if (c == ws || c.startsWith('$ws/')) return ws;
+  return c;
 }
 
 /// Read [workspaceRoot]'s preset through an injected settings [read] (key →
@@ -69,8 +92,9 @@ List<String> presetDirsFrom(
   String workspaceRoot, {
   bool Function(String path)? isFile,
   String? Function(String path)? readFile,
+  bool Function(String path)? isDir,
 }) {
-  final raw = read(pathPresetKey(workspaceRoot, isFile: isFile, readFile: readFile));
+  final raw = read(pathPresetKey(workspaceRoot, isFile: isFile, readFile: readFile, isDir: isDir));
   if (raw is! List) return const [];
   return [
     for (final e in raw)
@@ -81,11 +105,17 @@ List<String> presetDirsFrom(
 /// Pure prepend: [preset] dirs (de-duplicated, order kept) ahead of [base],
 /// with base entries that repeat a preset dir dropped so the preset always
 /// wins. An empty preset returns [base] unchanged.
+///
+/// Contract: one directory per entry. An entry containing [sep] is malformed
+/// (the CLI/UI reject it at input time; this guards stored values that
+/// predate the check) and is skipped — joined verbatim it would smuggle
+/// extra tokens into PATH, and a trailing separator yields an EMPTY token,
+/// which POSIX shells resolve as CWD.
 String applyPathPreset(String base, List<String> preset, {String sep = ':'}) {
   final dirs = <String>[];
   for (final d in preset) {
     final t = d.trim();
-    if (t.isNotEmpty && !dirs.contains(t)) dirs.add(t);
+    if (t.isNotEmpty && !t.contains(sep) && !dirs.contains(t)) dirs.add(t);
   }
   if (dirs.isEmpty) return base;
   final baseParts = base.isEmpty ? const <String>[] : base.split(sep);
@@ -108,6 +138,8 @@ List<String> missingLoginShellDirs({required String? loginPath, required String
 
 bool _isFile(String path) => FileSystemEntity.typeSync(path) == FileSystemEntityType.file;
 
+bool _isDir(String path) => FileSystemEntity.typeSync(path) == FileSystemEntityType.directory;
+
 String? _readFile(String path) {
   try {
     return File(path).readAsStringSync();

test/builtin/claude/session_orchestrator_test.dart

@@ -53,6 +53,19 @@ void main() {
     expect(spawnedArgs.single, isNot(contains('--effort')));
   });
 
+  test('the workspace PATH preset reaches the spawned session env (D-106)', () async {
+    final envs = <Map<String, String>?>[];
+    final preset = ClaudeSessionOrchestrator(
+      processFactory: ({required sessionArgs, required cwd, env}) async {
+        envs.add(env);
+        return _FakeProc();
+      },
+      pathPresetFor: (cwd) => cwd == '/repo' ? const ['/opt/go/bin'] : const [],
+    );
+    await preset.spawn(SpawnSpec(id: 'p1', role: 'primary', sessionId: 'p1-uuid', cwd: '/repo'));
+    expect(envs.single?['PATH'], startsWith('/opt/go/bin:'), reason: 'preset dirs lead the delta PATH');
+  });
+
   test('a fresh session gets the skills nudge; resume + fork do not (T-490)', () async {
     String appendPrompt(List<String> args) {
       final i = args.indexOf('--append-system-prompt');

test/builtin/tools_settings/path_preset_control_test.dart

@@ -81,6 +81,29 @@ void main() {
     expect(dirs(f.tempDir.path), isEmpty);
   });
 
+  testWidgets('an entry containing a PATH separator is rejected (CWD-token guard)', (tester) async {
+    await tester.runAsync(() => f.services.settings.setProjectDir(f.tempDir));
+    await pump(tester);
+    await tester.pump();
+    await tester.enterText(find.byType(EditableText), '/opt/go/bin:');
+    await tester.tap(find.text('Add entry'));
+    await tester.pump();
+    expect(find.textContaining('absolute path'), findsOneWidget);
+    expect(dirs(f.tempDir.path), isEmpty);
+  });
+
+  testWidgets('~/ expands against HOME and Enter submits (parity with the CLI verb)', (tester) async {
+    final home = Platform.environment['HOME'];
+    if (home == null || home.isEmpty) return; // no HOME in this environment — the guard path is CLI-tested
+    await tester.runAsync(() => f.services.settings.setProjectDir(f.tempDir));
+    await pump(tester);
+    await tester.pump();
+    await tester.enterText(find.byType(EditableText), '~/go/bin/');
+    await tester.testTextInput.receiveAction(TextInputAction.done);
+    await tester.pump();
+    expect(dirs(f.tempDir.path), ['$home/go/bin']);
+  });
+
   testWidgets('an existing dir renders without the missing tag', (tester) async {
     await tester.runAsync(() async {
       await f.services.settings.setProjectDir(f.tempDir);

test/daemon/env_path_commands_test.dart

@@ -32,7 +32,7 @@ void main() {
   String? loginPath;
   var processPath = '/usr/bin:/bin';
 
-  void wire({String? cwd = '/repo', bool withStore = true}) {
+  void wire({String? cwd = '/repo', bool withStore = true, String? home = '/home/u'}) {
     store = _FakeStore();
     published = [];
     existingDirs = {};
@@ -45,7 +45,7 @@ void main() {
       publisher: () =>
           (p, c, data) => published.add((publisher: p, channel: c, data: data)),
       workspaceCwd: () => cwd,
-      home: () => '/home/u',
+      home: () => home,
       dirExists: (dir) => existingDirs.contains(dir),
       loginPath: () => loginPath,
       processPath: () => processPath,
@@ -91,6 +91,26 @@ void main() {
       expect(empty.error?.hint, contains('clear'));
     });
 
+    test('add and remove apply the same guards as set (relative + separator)', () async {
+      wire();
+      for (final action in ['set', 'add', 'remove']) {
+        final rel = await run([action, 'go/bin']);
+        expect(rel.ok, isFalse, reason: '$action relative');
+        expect(rel.error?.message, contains('not an absolute path'));
+        final sep = await run([action, '/a:']);
+        expect(sep.ok, isFalse, reason: '$action separator');
+        expect(sep.error?.message, contains('PATH separator'));
+      }
+      expect(store.byCwd, isEmpty, reason: 'nothing was written');
+    });
+
+    test('~ with no HOME errors instead of storing a broken entry', () async {
+      wire(home: null);
+      final r = await run(['set', '~/go/bin']);
+      expect(r.ok, isFalse);
+      expect(r.error?.message, contains('cannot expand'));
+    });
+
     test('a leading-dash entry is rejected by the schema (T-104 guard)', () async {
       wire();
       final r = await d.dispatch(

test/src/env/path_preset_test.dart

@@ -34,6 +34,11 @@ void main() {
     test('honours a custom separator', () {
       expect(applyPathPreset(r'C:\bin', [r'C:\go'], sep: ';'), r'C:\go;C:\bin');
     });
+
+    test('skips a malformed entry containing the separator (would smuggle a CWD token)', () {
+      expect(applyPathPreset('/usr/bin', ['/a:', '/ok']), '/ok:/usr/bin');
+      expect(applyPathPreset('/usr/bin', ['/a::/b']), '/usr/bin');
+    });
   });
 
   group('missingLoginShellDirs', () {
@@ -65,20 +70,45 @@ void main() {
 
     test('an in-repo .worktrees worktree resolves to the main repo root (absolute gitdir)', () {
       expect(
-        presetRootFor('/repo/.worktrees/fix', isFile: (p) => p == '/repo/.worktrees/fix/.git', readFile: (p) => 'gitdir: /repo/.git/worktrees/fix\n'),
+        presetRootFor(
+          '/repo/.worktrees/fix',
+          isFile: (p) => p == '/repo/.worktrees/fix/.git',
+          readFile: (p) => 'gitdir: /repo/.git/worktrees/fix\n',
+          isDir: (p) => p == '/repo/.git',
+        ),
         '/repo',
       );
     });
 
     test('a relative gitdir pointer resolves against the worktree root', () {
       expect(
-        presetRootFor('/repo/.worktrees/fix', isFile: (p) => p == '/repo/.worktrees/fix/.git', readFile: (p) => 'gitdir: ../../.git/worktrees/fix'),
+        presetRootFor(
+          '/repo/.worktrees/fix',
+          isFile: (p) => p == '/repo/.worktrees/fix/.git',
+          readFile: (p) => 'gitdir: ../../.git/worktrees/fix',
+          isDir: (p) => p == '/repo/.git',
+        ),
         '/repo',
       );
     });
 
     test('a worktree outside the repo still resolves to the main root', () {
-      expect(presetRootFor('/tmp/wt', isFile: (p) => p == '/tmp/wt/.git', readFile: (_) => 'gitdir: /srv/repos/main/.git/worktrees/wt'), '/srv/repos/main');
+      expect(
+        presetRootFor(
+          '/tmp/wt',
+          isFile: (p) => p == '/tmp/wt/.git',
+          readFile: (_) => 'gitdir: /srv/repos/main/.git/worktrees/wt',
+          isDir: (p) => p == '/srv/repos/main/.git',
+        ),
+        '/srv/repos/main',
+      );
+    });
+
+    test('a pointer whose target is not a real repo is ignored (repo-controlled content)', () {
+      expect(
+        presetRootFor('/evil', isFile: (p) => p == '/evil/.git', readFile: (_) => 'gitdir: /home/u/victim/.git/worktrees/x', isDir: (_) => false),
+        '/evil',
+      );
     });
 
     test('a gitdir pointer without the worktrees marker (submodule-style) keys off itself', () {
@@ -91,7 +121,15 @@ void main() {
     });
 
     test('backslashed gitdir (Windows-written pointer) still matches', () {
-      expect(presetRootFor('/repo/.worktrees/x', isFile: (p) => p == '/repo/.worktrees/x/.git', readFile: (_) => r'gitdir: /repo/.git\worktrees\x'), '/repo');
+      expect(
+        presetRootFor(
+          '/repo/.worktrees/x',
+          isFile: (p) => p == '/repo/.worktrees/x/.git',
+          readFile: (_) => r'gitdir: /repo/.git\worktrees\x',
+          isDir: (p) => p == '/repo/.git',
+        ),
+        '/repo',
+      );
     });
 
     test('resolves a REAL worktree layout on disk (no injected probes)', () {
@@ -116,7 +154,12 @@ void main() {
     test('a worktree and its main repo share one key; trailing slash is unified', () {
       final main = pathPresetKey('/repo', isFile: (_) => false, readFile: (_) => null);
       final slash = pathPresetKey('/repo/', isFile: (_) => false, readFile: (_) => null);
-      final wt = pathPresetKey('/repo/.worktrees/fix', isFile: (p) => p == '/repo/.worktrees/fix/.git', readFile: (_) => 'gitdir: /repo/.git/worktrees/fix');
+      final wt = pathPresetKey(
+        '/repo/.worktrees/fix',
+        isFile: (p) => p == '/repo/.worktrees/fix/.git',
+        readFile: (_) => 'gitdir: /repo/.git/worktrees/fix',
+        isDir: (p) => p == '/repo/.git',
+      );
       expect(slash, main);
       expect(wt, main);
       expect(pathPresetKey('/other', isFile: (_) => false, readFile: (_) => null), isNot(main));