Skip to content

Commit e43dcc3

Browse files
gspencergooggspencergoog
committed
Incorporate validateRootConfig and unit tests for it
1 parent fce43e8 commit e43dcc3

File tree

5 files changed

+196
-24
lines changed

5 files changed

+196
-24
lines changed

‎pkgs/dart_mcp_server/lib/src/utils/cli_utils.dart‎

Lines changed: 17 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,7 @@ Future<CallToolResult> runCommandInRoot(
165165
}
166166

167167
final root = knownRoots.firstWhereOrNull(
168-
(root) => isUnderRoot(root, rootUriString, fileSystem),
168+
(root) => _isUnderRoot(root, rootUriString, fileSystem),
169169
);
170170
if (root == null) {
171171
return CallToolResult(
@@ -203,7 +203,7 @@ Future<CallToolResult> runCommandInRoot(
203203
(rootConfig?[ParameterNames.paths] as List?)?.cast<String>() ??
204204
defaultPaths;
205205
final invalidPaths = paths.where(
206-
(path) => !isUnderRoot(root, path, fileSystem),
206+
(path) => !_isUnderRoot(root, path, fileSystem),
207207
);
208208
if (invalidPaths.isNotEmpty) {
209209
return CallToolResult(
@@ -285,52 +285,51 @@ validateRootConfig(
285285
)..failureReason ??= CallToolFailureReason.noRootGiven,
286286
);
287287
}
288-
289-
final knownRoot = knownRoots.firstWhereOrNull(
290-
(root) => isUnderRoot(root, rootUriString, fileSystem),
291-
);
292-
if (knownRoot == null) {
288+
final rootUri = Uri.parse(rootUriString);
289+
if (rootUri.scheme != 'file') {
293290
return (
294291
root: null,
295292
paths: null,
296293
errorResult: CallToolResult(
297294
content: [
298295
TextContent(
299296
text:
300-
'Invalid root $rootUriString, must be under one of the '
301-
'registered project roots:\n\n${knownRoots.join('\n')}',
297+
'Only file scheme uris are allowed for roots, but got '
298+
'$rootUri',
302299
),
303300
],
304301
isError: true,
305-
)..failureReason ??= CallToolFailureReason.invalidRootPath,
302+
)..failureReason ??= CallToolFailureReason.invalidRootScheme,
306303
);
307304
}
308-
final root = Root(uri: rootUriString);
309305

310-
final rootUri = Uri.parse(rootUriString);
311-
if (rootUri.scheme != 'file') {
306+
final knownRoot = knownRoots.firstWhereOrNull(
307+
(root) => _isUnderRoot(root, rootUriString, fileSystem),
308+
);
309+
if (knownRoot == null) {
312310
return (
313311
root: null,
314312
paths: null,
315313
errorResult: CallToolResult(
316314
content: [
317315
TextContent(
318316
text:
319-
'Only file scheme uris are allowed for roots, but got '
320-
'$rootUri',
317+
'Invalid root $rootUriString, must be under one of the '
318+
'registered project roots:\n\n${knownRoots.join('\n')}',
321319
),
322320
],
323321
isError: true,
324-
)..failureReason ??= CallToolFailureReason.invalidRootScheme,
322+
)..failureReason ??= CallToolFailureReason.invalidRootPath,
325323
);
326324
}
325+
final root = Root(uri: rootUriString);
327326

328327
final paths =
329328
(rootConfig?[ParameterNames.paths] as List?)?.cast<String>() ??
330329
defaultPaths;
331330
if (paths != null) {
332331
final invalidPaths = paths.where(
333-
(path) => !isUnderRoot(root, path, fileSystem),
332+
(path) => !_isUnderRoot(root, path, fileSystem),
334333
);
335334
if (invalidPaths.isNotEmpty) {
336335
return (
@@ -373,7 +372,7 @@ Future<String> defaultCommandForRoot(
373372
/// Returns whether [uri] is under or exactly equal to [root].
374373
///
375374
/// Relative uris will always be under [root] unless they escape it with `../`.
376-
bool isUnderRoot(Root root, String uri, FileSystem fileSystem) {
375+
bool _isUnderRoot(Root root, String uri, FileSystem fileSystem) {
377376
// This normalizes the URI to ensure it is treated as a directory (for example
378377
// ensures it ends with a trailing slash).
379378
final rootUri = fileSystem.directory(Uri.parse(root.uri)).uri;

‎pkgs/dart_mcp_server/test/test_harness.dart‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -173,7 +173,7 @@ class TestHarness {
173173
final result = await mcpServerConnection.callTool(request);
174174
expect(
175175
result.isError,
176-
expectError ? true : isNot(true),
176+
expectError ? true : isNot(isTrue),
177177
reason: result.content.join('\n'),
178178
);
179179
return result;
@@ -185,11 +185,12 @@ class TestHarness {
185185
Future<CallToolResult> callToolWithRetry(
186186
CallToolRequest request, {
187187
int maxTries = 5,
188+
bool expectError = false,
188189
}) async {
189190
var tryCount = 0;
190191
while (true) {
191192
try {
192-
return await callTool(request);
193+
return await callTool(request, expectError: expectError);
193194
} catch (_) {
194195
if (tryCount++ >= maxTries) rethrow;
195196
}

‎pkgs/dart_mcp_server/test/tools/analyzer_test.dart‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -261,11 +261,19 @@ void main() {
261261
name: analyzeTool.name,
262262
arguments: {ParameterNames.roots: []},
263263
);
264-
final result = await testHarness.callToolWithRetry(request);
265-
expect(result.isError, isNot(true));
264+
final result = await testHarness.callToolWithRetry(
265+
request,
266+
expectError: true,
267+
);
268+
expect(result.isError, isTrue);
266269
expect(
267270
result.content.single,
268-
isA<TextContent>().having((t) => t.text, 'text', 'No errors'),
271+
isA<TextContent>().having(
272+
(t) => t.text,
273+
'text',
274+
'No roots set. At least one root must be set in order to use this '
275+
'tool.',
276+
),
269277
);
270278
});
271279

‎pkgs/dart_mcp_server/test/utils/cli_utils_test.dart‎

Lines changed: 163 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -279,6 +279,169 @@ void main() {
279279
);
280280
});
281281
});
282+
283+
group('validateRootConfig', () {
284+
test('succeeds with a valid root and no paths', () {
285+
final result = validateRootConfig(
286+
{ParameterNames.root: 'file:///project/'},
287+
knownRoots: [Root(uri: 'file:///project/')],
288+
fileSystem: fileSystem,
289+
);
290+
291+
expect(result.errorResult, isNull);
292+
expect(result.root, isNotNull);
293+
expect(result.root!.uri, 'file:///project/');
294+
expect(result.paths, isNull);
295+
});
296+
297+
test('succeeds with a root that is a subdirectory of a known root', () {
298+
final result = validateRootConfig(
299+
{ParameterNames.root: 'file:///project/sub'},
300+
knownRoots: [Root(uri: 'file:///project/')],
301+
fileSystem: fileSystem,
302+
);
303+
304+
expect(result.errorResult, isNull);
305+
expect(result.root, isNotNull);
306+
expect(result.root!.uri, 'file:///project/sub');
307+
expect(result.paths, isNull);
308+
});
309+
310+
test('succeeds with valid paths', () {
311+
final paths = ['./lib', 'lib/src/stuff.dart'];
312+
final result = validateRootConfig(
313+
{ParameterNames.root: 'file:///project/', ParameterNames.paths: paths},
314+
knownRoots: [Root(uri: 'file:///project/')],
315+
fileSystem: fileSystem,
316+
);
317+
318+
expect(result.errorResult, isNull);
319+
expect(result.root, isNotNull);
320+
expect(result.root!.uri, 'file:///project/');
321+
expect(result.paths, paths);
322+
});
323+
324+
test('uses default paths when none are provided', () {
325+
final defaultPaths = ['./lib'];
326+
final result = validateRootConfig(
327+
{ParameterNames.root: 'file:///project/'},
328+
defaultPaths: defaultPaths,
329+
knownRoots: [Root(uri: 'file:///project/')],
330+
fileSystem: fileSystem,
331+
);
332+
333+
expect(result.errorResult, isNull);
334+
expect(result.root, isNotNull);
335+
expect(result.root!.uri, 'file:///project/');
336+
expect(result.paths, defaultPaths);
337+
});
338+
339+
test('uses provided paths over default paths', () {
340+
final paths = ['./lib'];
341+
final defaultPaths = ['./test'];
342+
final result = validateRootConfig(
343+
{ParameterNames.root: 'file:///project/', ParameterNames.paths: paths},
344+
defaultPaths: defaultPaths,
345+
knownRoots: [Root(uri: 'file:///project/')],
346+
fileSystem: fileSystem,
347+
);
348+
349+
expect(result.errorResult, isNull);
350+
expect(result.root, isNotNull);
351+
expect(result.root!.uri, 'file:///project/');
352+
expect(result.paths, paths);
353+
});
354+
355+
test('fails if root config is null', () {
356+
final result = validateRootConfig(
357+
null,
358+
knownRoots: [Root(uri: 'file:///project/')],
359+
fileSystem: fileSystem,
360+
);
361+
362+
expect(result.errorResult, isNotNull);
363+
expect(result.root, isNull);
364+
expect(result.paths, isNull);
365+
expect(result.errorResult!.isError, isTrue);
366+
expect(
367+
(result.errorResult!.content.single as TextContent).text,
368+
contains('missing `root` key'),
369+
);
370+
});
371+
372+
test('fails if root config is missing root key', () {
373+
final result = validateRootConfig(
374+
{},
375+
knownRoots: [Root(uri: 'file:///project/')],
376+
fileSystem: fileSystem,
377+
);
378+
379+
expect(result.errorResult, isNotNull);
380+
expect(result.root, isNull);
381+
expect(result.paths, isNull);
382+
expect(result.errorResult!.isError, isTrue);
383+
expect(
384+
(result.errorResult!.content.single as TextContent).text,
385+
contains('missing `root` key'),
386+
);
387+
});
388+
389+
test('fails if root is outside of known roots', () {
390+
final result = validateRootConfig(
391+
{ParameterNames.root: 'file:///other/'},
392+
knownRoots: [Root(uri: 'file:///project/')],
393+
fileSystem: fileSystem,
394+
);
395+
396+
expect(result.errorResult, isNotNull);
397+
expect(result.root, isNull);
398+
expect(result.paths, isNull);
399+
expect(result.errorResult!.isError, isTrue);
400+
expect(
401+
(result.errorResult!.content.single as TextContent).text,
402+
contains('Invalid root file:///other/'),
403+
);
404+
});
405+
406+
test('fails if root has a non-file scheme', () {
407+
final result = validateRootConfig(
408+
{ParameterNames.root: 'http:///project/'},
409+
knownRoots: [Root(uri: 'file:///project/')],
410+
fileSystem: fileSystem,
411+
);
412+
413+
expect(result.errorResult, isNotNull);
414+
expect(result.root, isNull);
415+
expect(result.paths, isNull);
416+
expect(result.errorResult!.isError, isTrue);
417+
expect(
418+
(result.errorResult!.content.single as TextContent).text,
419+
contains('Only file scheme uris are allowed'),
420+
);
421+
});
422+
423+
test('fails with paths that escape the root', () {
424+
final paths = ['../outside.dart', '/other/place.dart'];
425+
final result = validateRootConfig(
426+
{ParameterNames.root: 'file:///project/', ParameterNames.paths: paths},
427+
knownRoots: [Root(uri: 'file:///project/')],
428+
fileSystem: fileSystem,
429+
);
430+
431+
expect(result.errorResult, isNotNull);
432+
expect(result.root, isNull);
433+
expect(result.paths, isNull);
434+
expect(result.errorResult!.isError, isTrue);
435+
expect(
436+
(result.errorResult!.content.single as TextContent).text,
437+
allOf(
438+
contains('Paths are not allowed to escape their project root'),
439+
contains('../outside.dart'),
440+
contains('/other/place.dart'),
441+
),
442+
);
443+
});
444+
});
282445
}
283446

284447
class FakeProcessManager extends Fake implements ProcessManager {}

‎pkgs/dart_mcp_server/test_fixtures/counter_app/android/local.properties‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1,2 @@
1-
flutter.sdk=/usr/local/google/home/jakemac/flutter
1+
flutter.sdk=/usr/local/google/home/gspencer/code/flutter
2+
sdk.dir=/usr/local/google/home/gspencer/Android/Sdk

0 commit comments

Comments
 (0)