Skip to content

Commit 0e90b72

Browse files
jqnatividadclaude
andcommitted
fix(mcp): remove dead mutableConfig, fix misleading comment, and add installer tests
- Remove unused `mutableConfig` object in config.ts (dead code) - Update comment to accurately describe type-cast assignment approach (not Object.defineProperty) and document `as const` runtime behavior - Use MINIMUM_QSV_VERSION constant instead of hardcoded ">= 17.0.0" to prevent version string from going stale - Log errors in sendToolListChanged catch handler instead of swallowing - Export detectPackageManager and getManualInstructions for testability - Add installer.test.ts with unit tests for both functions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 8d9b045 commit 0e90b72

File tree

5 files changed

+72
-19
lines changed

5 files changed

+72
-19
lines changed

.claude/skills/src/config.ts

Lines changed: 3 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -906,16 +906,6 @@ export const config = {
906906
})(),
907907
} as const;
908908

909-
/**
910-
* Mutable configuration fields that can change after initialization.
911-
* These are separated from the frozen `config` object because they need
912-
* to be updated when qsv is installed or the binary path changes at runtime.
913-
*/
914-
const mutableConfig = {
915-
qsvBinPath: config.qsvBinPath,
916-
qsvValidation: config.qsvValidation,
917-
};
918-
919909
/**
920910
* Re-run qsv binary detection and validation.
921911
* Called after a successful installation to refresh the config state.
@@ -926,11 +916,10 @@ export function revalidateQsvBinary(): {
926916
validation: QsvValidationResult;
927917
} {
928918
const result = initializeQsvBinaryPath();
929-
mutableConfig.qsvBinPath = result.path;
930-
mutableConfig.qsvValidation = result.validation;
931919

932-
// Update the config object's mutable fields via Object.defineProperty
933-
// since the rest of config is frozen via `as const`
920+
// Update config via type-cast assignment. This works because `as const`
921+
// only produces a deeply-readonly TypeScript type — it does NOT call
922+
// Object.freeze() at runtime, so the object remains mutable in JS.
934923
(config as { qsvBinPath: string }).qsvBinPath = result.path;
935924
(config as { qsvValidation: QsvValidationResult }).qsvValidation = result.validation;
936925

.claude/skills/src/installer.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ type PackageManager = "homebrew" | "scoop" | "none";
2727
/**
2828
* Detect available package manager for the current platform.
2929
*/
30-
function detectPackageManager(): PackageManager {
30+
export function detectPackageManager(): PackageManager {
3131
const command = process.platform === "win32" ? "where" : "which";
3232

3333
if (process.platform === "darwin" || process.platform === "linux") {
@@ -143,7 +143,7 @@ async function installViaScoop(): Promise<InstallResult> {
143143
/**
144144
* Return manual installation instructions when no supported package manager is available.
145145
*/
146-
function getManualInstructions(platform: NodeJS.Platform): InstallResult {
146+
export function getManualInstructions(platform: NodeJS.Platform): InstallResult {
147147
const baseUrl = "https://github.com/dathere/qsv/releases/latest";
148148

149149
let instructions: string;

.claude/skills/src/mcp-server.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -694,7 +694,7 @@ class QsvMcpServer {
694694
});
695695
// Send tools/list_changed notification so the client re-fetches the full tool list
696696
if (setupResult.revalidated) {
697-
this.server.sendToolListChanged().catch(() => {});
697+
this.server.sendToolListChanged().catch((err: unknown) => console.error("[Server] Failed to send tools/list_changed:", err));
698698
}
699699
return setupResult.response;
700700
}

.claude/skills/src/mcp-tools.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ import type { SkillExecutor } from "./executor.js";
2626
import type { Server } from "@modelcontextprotocol/sdk/server/index.js";
2727
import { executeDescribegptWithSampling, runQsvCapture } from "./mcp-sampling.js";
2828
import type { SkillLoader } from "./loader.js";
29-
import { config, getDetectionDiagnostics } from "./config.js";
29+
import { config, getDetectionDiagnostics, MINIMUM_QSV_VERSION } from "./config.js";
3030
import { formatBytes, findSimilarFiles, errorResult, successResult, isReservedCachePath, reservedCachePathError, getErrorMessage, isNodeError, describegptFallbackResult } from "./utils.js";
3131
import {
3232
detectDuckDb,
@@ -4123,7 +4123,7 @@ export async function handleSetupCall(
41234123
`qsv was installed via ${result.method}, but validation failed:\n` +
41244124
`${revalidation.validation.error}\n\n` +
41254125
`The installed version may not meet the minimum requirements. ` +
4126-
`Please install qsv ${config.qsvValidation.version ? ">= 17.0.0" : ""} with Polars support ` +
4126+
`Please install qsv >= ${MINIMUM_QSV_VERSION} with Polars support ` +
41274127
`from https://github.com/dathere/qsv#installation`,
41284128
),
41294129
};
Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
/**
2+
* Unit tests for the installer module
3+
*/
4+
5+
import { test } from "node:test";
6+
import assert from "node:assert";
7+
import { detectPackageManager, getManualInstructions } from "../src/installer.js";
8+
9+
test("detectPackageManager returns a valid package manager type", () => {
10+
const result = detectPackageManager();
11+
assert.ok(
12+
result === "homebrew" || result === "scoop" || result === "none",
13+
`Expected homebrew, scoop, or none but got: ${result}`,
14+
);
15+
});
16+
17+
test("detectPackageManager returns homebrew or none on non-Windows", () => {
18+
if (process.platform !== "win32") {
19+
const result = detectPackageManager();
20+
assert.notStrictEqual(result, "scoop", "scoop should not be detected on non-Windows");
21+
}
22+
});
23+
24+
test("getManualInstructions returns instructions for macOS", () => {
25+
const result = getManualInstructions("darwin");
26+
assert.strictEqual(result.success, false);
27+
assert.strictEqual(result.method, "manual");
28+
assert.ok(result.instructions, "should have instructions");
29+
assert.ok(result.instructions!.includes("macOS"), "should mention macOS");
30+
assert.ok(result.instructions!.includes("Homebrew"), "should mention Homebrew");
31+
assert.ok(
32+
result.instructions!.includes("github.com/dathere/qsv/releases"),
33+
"should include releases URL",
34+
);
35+
});
36+
37+
test("getManualInstructions returns instructions for Windows", () => {
38+
const result = getManualInstructions("win32");
39+
assert.strictEqual(result.success, false);
40+
assert.strictEqual(result.method, "manual");
41+
assert.ok(result.instructions, "should have instructions");
42+
assert.ok(result.instructions!.includes("Windows"), "should mention Windows");
43+
assert.ok(result.instructions!.includes("Scoop"), "should mention Scoop");
44+
});
45+
46+
test("getManualInstructions returns instructions for Linux", () => {
47+
const result = getManualInstructions("linux");
48+
assert.strictEqual(result.success, false);
49+
assert.strictEqual(result.method, "manual");
50+
assert.ok(result.instructions, "should have instructions");
51+
assert.ok(result.instructions!.includes("Linux"), "should mention Linux");
52+
assert.ok(
53+
result.instructions!.includes("cargo install"),
54+
"should include build from source option",
55+
);
56+
});
57+
58+
test("getManualInstructions handles unknown platforms as Linux", () => {
59+
const result = getManualInstructions("freebsd" as NodeJS.Platform);
60+
assert.strictEqual(result.success, false);
61+
assert.strictEqual(result.method, "manual");
62+
assert.ok(result.instructions, "should have instructions for unknown platform");
63+
assert.ok(result.instructions!.includes("Linux"), "should fall back to Linux instructions");
64+
});

0 commit comments

Comments
 (0)