Skip to content

Commit 708f1e6

Browse files
Copilotdata-douser
andauthored
Address PR #27 review feedback: path normalization, tests, and conventions (#28)
* Initial plan * Fix import ordering, restore dot prefix, normalize temp dir paths, add unit tests Co-authored-by: data-douser <70299490+data-douser@users.noreply.github.com> * Remove weak test case and document limitation in comment Co-authored-by: data-douser <70299490+data-douser@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: data-douser <70299490+data-douser@users.noreply.github.com>
1 parent a66c4a2 commit 708f1e6

File tree

7 files changed

+206
-42
lines changed

7 files changed

+206
-42
lines changed

server/dist/codeql-development-mcp-server.js

Lines changed: 33 additions & 33 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

server/dist/codeql-development-mcp-server.js.map

Lines changed: 3 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

server/src/lib/language-server.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,9 @@ import { setTimeout, clearTimeout } from 'timers';
99
import { pathToFileURL } from 'url';
1010
import { delimiter, join } from 'path';
1111
import { logger } from '../utils/logger';
12+
import { getPackageVersion } from '../utils/package-paths';
1213
import { getProjectTmpDir } from '../utils/temp-dir';
1314
import { getResolvedCodeQLDir } from './cli-executor';
14-
import { getPackageVersion } from '../utils/package-paths';
1515

1616
export interface LSPMessage {
1717
jsonrpc: '2.0';

server/src/lib/session-data-manager.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -403,6 +403,6 @@ function parseBoolEnv(envVar: string | undefined, defaultValue: boolean): boolea
403403

404404
// Export singleton instance with environment variable support
405405
export const sessionDataManager = new SessionDataManager({
406-
storageLocation: process.env.MONITORING_STORAGE_LOCATION || join(getProjectTmpBase(), 'ql-mcp-tracking'),
406+
storageLocation: process.env.MONITORING_STORAGE_LOCATION || join(getProjectTmpBase(), '.ql-mcp-tracking'),
407407
enableMonitoringTools: parseBoolEnv(process.env.ENABLE_MONITORING_TOOLS, false),
408408
});

server/src/utils/temp-dir.ts

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*/
99

1010
import { mkdirSync, mkdtempSync } from 'fs';
11-
import { join } from 'path';
11+
import { isAbsolute, join, resolve } from 'path';
1212
import { getPackageRootDir } from './package-paths';
1313

1414
/**
@@ -17,10 +17,14 @@ import { getPackageRootDir } from './package-paths';
1717
* Resolution order:
1818
* 1. `CODEQL_MCP_TMP_DIR` environment variable — for read-only package root
1919
* scenarios (e.g., npm global installs where the package directory is not
20-
* writable).
20+
* writable). Relative paths are resolved against process.cwd().
2121
* 2. `<packageRoot>/.tmp` — default; excluded from version control.
2222
*/
23-
const PROJECT_TMP_BASE = process.env.CODEQL_MCP_TMP_DIR || join(getPackageRootDir(), '.tmp');
23+
const PROJECT_TMP_BASE = process.env.CODEQL_MCP_TMP_DIR
24+
? (isAbsolute(process.env.CODEQL_MCP_TMP_DIR)
25+
? process.env.CODEQL_MCP_TMP_DIR
26+
: resolve(process.cwd(), process.env.CODEQL_MCP_TMP_DIR))
27+
: join(getPackageRootDir(), '.tmp');
2428

2529
/**
2630
* Return the project-local `.tmp` base directory, creating it if needed.

server/test/src/lib/cli-tool-registry.test.ts

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -571,6 +571,109 @@ describe('registerCLITool handler behavior', () => {
571571
);
572572
});
573573

574+
it('should resolve relative tests parameter against user workspace dir', async () => {
575+
const definition: CLIToolDefinition = {
576+
name: 'codeql_test_run',
577+
description: 'Run tests',
578+
command: 'codeql',
579+
subcommand: 'test run',
580+
inputSchema: {
581+
tests: z.array(z.string())
582+
}
583+
};
584+
585+
registerCLITool(mockServer, definition);
586+
587+
const handler = (mockServer.tool as ReturnType<typeof vi.fn>).mock.calls[0][3];
588+
589+
executeCodeQLCommand.mockResolvedValueOnce({
590+
stdout: 'All tests passed',
591+
stderr: '',
592+
success: true
593+
});
594+
595+
// Test with relative paths
596+
await handler({ tests: ['relative/test1.ql', '/absolute/test2.ql'] });
597+
598+
// executeCodeQLCommand signature: (subcommand, options, additionalArgs, cwd)
599+
const call = executeCodeQLCommand.mock.calls[0];
600+
const positionalArgs = call[2];
601+
602+
// First relative path should be resolved to absolute
603+
expect(positionalArgs[0]).not.toBe('relative/test1.ql');
604+
expect(positionalArgs[0]).toContain('relative/test1.ql');
605+
606+
// Second absolute path should remain unchanged
607+
expect(positionalArgs[1]).toBe('/absolute/test2.ql');
608+
});
609+
610+
it('should resolve relative database parameter against user workspace dir', async () => {
611+
const definition: CLIToolDefinition = {
612+
name: 'codeql_query_run',
613+
description: 'Run query',
614+
command: 'codeql',
615+
subcommand: 'query run',
616+
inputSchema: {
617+
database: z.string(),
618+
query: z.string()
619+
}
620+
};
621+
622+
registerCLITool(mockServer, definition);
623+
624+
const handler = (mockServer.tool as ReturnType<typeof vi.fn>).mock.calls[0][3];
625+
626+
executeCodeQLCommand.mockResolvedValueOnce({
627+
stdout: '{"columns":[]}',
628+
stderr: '',
629+
success: true
630+
});
631+
632+
// Test with relative database path
633+
await handler({ database: 'my-database', query: '/path/to/query.ql' });
634+
635+
// executeCodeQLCommand signature: (subcommand, options, additionalArgs, cwd)
636+
const call = executeCodeQLCommand.mock.calls[0];
637+
const options = call[1];
638+
639+
// Relative database path should be resolved to absolute
640+
expect(options.database).not.toBe('my-database');
641+
expect(options.database).toContain('my-database');
642+
});
643+
644+
it('should resolve relative dir/packDir parameter against user workspace dir', async () => {
645+
const definition: CLIToolDefinition = {
646+
name: 'codeql_pack_install',
647+
description: 'Install packs',
648+
command: 'codeql',
649+
subcommand: 'pack install',
650+
inputSchema: {
651+
dir: z.string()
652+
}
653+
};
654+
655+
registerCLITool(mockServer, definition);
656+
657+
const handler = (mockServer.tool as ReturnType<typeof vi.fn>).mock.calls[0][3];
658+
659+
executeCodeQLCommand.mockResolvedValueOnce({
660+
stdout: 'Installed successfully',
661+
stderr: '',
662+
success: true
663+
});
664+
665+
// Test with relative dir path
666+
await handler({ dir: 'my-pack-dir' });
667+
668+
// executeCodeQLCommand signature: (subcommand, options, additionalArgs, cwd)
669+
const call = executeCodeQLCommand.mock.calls[0];
670+
const cwd = call[3];
671+
672+
// Relative dir should be resolved to absolute for cwd
673+
expect(cwd).not.toBe('my-pack-dir');
674+
expect(cwd).toContain('my-pack-dir');
675+
});
676+
574677
it('should handle dir parameter as positional for pack_ls', async () => {
575678
const definition: CLIToolDefinition = {
576679
name: 'codeql_pack_ls',

0 commit comments

Comments
 (0)