Skip to content

Require roots for all CLI tools #101

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
May 2, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
add tests, fix implementation
  • Loading branch information
jakemac53 committed May 1, 2025
commit b0ed06e1bb18e5fc7b223497f4890b3637517496
9 changes: 7 additions & 2 deletions pkgs/dart_tooling_mcp_server/lib/src/utils/cli_utils.dart
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ Future<CallToolResult> runCommandInRoots(
'registered project roots:\n\n${knownRoots.join('\n')}',
),
],
isError: true,
);
}

Expand All @@ -92,9 +93,13 @@ Future<CallToolResult> runCommandInRoots(

final commandWithPaths = List.of(command);
final paths =
(rootConfig[ParameterNames.paths] as List?)?.cast<String>() ??
(rootConfig[ParameterNames.paths] as List?)?.cast<String>().map(
(path) => rootUri.resolve(path).toString(),
) ??
defaultPaths;
final invalidPaths = paths.where((path) => !p.isWithin(rootUri.path, path));
final invalidPaths = paths.where(
(path) => !p.isWithin(rootUri.toString(), path),
);
if (invalidPaths.isNotEmpty) {
return CallToolResult(
content: [
Expand Down
6 changes: 3 additions & 3 deletions pkgs/dart_tooling_mcp_server/test/tools/dart_cli_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ void main() {
);
final result = await testHarness.callToolWithRetry(request);

// Verify the command was sent to the process maanger without error.
// Verify the command was sent to the process manager without error.
expect(result.isError, isNot(true));
expect(testProcessManager.commandsRan, [
['dart', 'fix', '--apply'],
Expand All @@ -73,7 +73,7 @@ void main() {
);
final result = await testHarness.callToolWithRetry(request);

// Verify the command was sent to the process maanger without error.
// Verify the command was sent to the process manager without error.
expect(result.isError, isNot(true));
expect(testProcessManager.commandsRan, [
['dart', 'format', '.'],
Expand All @@ -94,7 +94,7 @@ void main() {
);
final result = await testHarness.callToolWithRetry(request);

// Verify the command was sent to the process maanger without error.
// Verify the command was sent to the process manager without error.
expect(result.isError, isNot(true));
expect(testProcessManager.commandsRan, [
['dart', 'format', 'foo.dart', 'bar.dart'],
Expand Down
82 changes: 82 additions & 0 deletions pkgs/dart_tooling_mcp_server/test/utils/cli_utils_test.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

import 'package:dart_mcp/server.dart';
import 'package:dart_tooling_mcp_server/src/utils/cli_utils.dart';
import 'package:dart_tooling_mcp_server/src/utils/constants.dart';
import 'package:process/process.dart';
import 'package:test/fake.dart';
import 'package:test/test.dart';

void main() {
final processManager = FakeProcessManager();

test('cannot run commands with roots outside of known roots', () async {
final result = await runCommandInRoots(
CallToolRequest(
name: 'foo',
arguments: {
ParameterNames.roots: [
{ParameterNames.root: 'file:///bar/'},
],
},
),
command: ['fake'],
commandDescription: '',
processManager: processManager,
knownRoots: [Root(uri: 'file:///foo/')],
);
expect(result.isError, isTrue);
expect(
result.content.single,
isA<TextContent>().having(
(t) => t.text,
'text',
contains('Invalid root file:///bar/'),
),
);
});

test('cannot run commands with paths outside of known roots', () async {
final result = await runCommandInRoots(
CallToolRequest(
name: 'foo',
arguments: {
ParameterNames.roots: [
{
ParameterNames.root: 'file:///foo/',
ParameterNames.paths: [
'file:///bar/',
'../baz/',
'zip/../../zap/',
'ok.dart',
],
},
],
},
),
command: ['fake'],
commandDescription: '',
processManager: processManager,
knownRoots: [Root(uri: 'file:///foo/')],
);
expect(result.isError, isTrue);
expect(
result.content.single,
isA<TextContent>().having(
(t) => t.text,
'text',
allOf(
contains('Paths are not allowed to escape their project root'),
contains('bar/'),
contains('baz/'),
contains('zap/'),
isNot(contains('ok.dart')),
),
),
);
});
}

class FakeProcessManager extends Fake implements ProcessManager {}