fix: address review feedback — import source, endpoint guard, test discoverability

Oxygen56 · Oxygen56 · commit c645a8a7eecd · 2026-06-12T18:49:24.000+08:00
- telemetry.test.ts: import InMemorySpanExporter/SimpleSpanProcessor from
  @opentelemetry/sdk-trace-node (listed dep) instead of sdk-trace-base
  (transitive only, would fail at runtime)
- telemetry.ts: add early-return guard when no OTLP endpoint is configured
  and no _testProvider is given, so initTelemetry truly is a no-op
- Move test files from src/__tests__/ to test/ so npm test glob finds them
- Switch test runner to tsx (handles .js→.ts resolution that
  --experimental-strip-types cannot, required by memory.ts → shared.ts)
- Revert tsconfig.json exclude change (tests no longer under src/)
diff --git a/package.json b/package.json
@@ -43,7 +43,7 @@
     "build": "tsc",
     "dev": "tsc --watch",
     "start": "node dist/index.js",
-    "test": "node --test test/*.test.ts --experimental-strip-types"
+    "test": "npx tsx --test test/*.test.ts"
   },
   "engines": {
     "node": ">=20"
diff --git a/src/__tests__/telemetry.test.ts b/src/__tests__/telemetry.test.ts
diff --git a/src/telemetry.ts b/src/telemetry.ts
@@ -71,6 +71,11 @@ const _slots = {
 export async function initTelemetry(opts: TelemetryOptions): Promise<void> {
 	if (_initialized) return;
 
+	// When there is no endpoint configured and no test provider, telemetry
+	// has nowhere to send data — skip the dynamic SDK imports entirely.
+	const hasEndpoint = opts.exporterEndpoint || process.env.OTEL_EXPORTER_OTLP_ENDPOINT;
+	if (!opts._testProvider && !hasEndpoint) return;
+
 	try {
 		// Test path — register a caller-supplied TracerProvider directly.
 		if (opts._testProvider) {
diff --git a/test/memory.test.ts b/test/memory.test.ts
@@ -12,45 +12,38 @@ import { join } from "path";
 import { tmpdir } from "os";
 import { execSync } from "child_process";
 
-let createMemoryTool: typeof import("../memory.ts").createMemoryTool;
+let createMemoryTool: typeof import("../src/tools/memory.ts").createMemoryTool;
 
 before(async () => {
-	const mod = await import("../memory.ts");
+	const mod = await import("../src/tools/memory.ts");
 	createMemoryTool = mod.createMemoryTool;
 });
 
 describe("memory tool", () => {
-	/** Create a temporary directory with git init and return the path. */
 	async function setupRepo(): Promise<string> {
 		const dir = await mkdtemp(join(tmpdir(), "gitagent-memory-test-"));
 		execSync("git init -q", { cwd: dir });
-		// Configure git user for commits
 		execSync('git config --local user.email "test@gitagent.test"', { cwd: dir });
 		execSync('git config --local user.name "Test Agent"', { cwd: dir });
 		return dir;
 	}
 
-	/** Clean up a temp directory. */
 	async function cleanup(dir: string): Promise<void> {
 		await rm(dir, { recursive: true, force: true }).catch(() => {});
 	}
 
-	// ── load ─────────────────────────────────────────────────────────
-
 	describe("load", () => {
 		it("returns stored memory content", async () => {
 			const dir = await setupRepo();
 			try {
 				const tool = createMemoryTool(dir);
 
-				// First, save some memory content
 				await tool.execute("call-1", {
 					action: "save",
 					content: "# Memory\n\n- Remember to buy milk\n- Project uses TypeScript",
 					message: "Initial memory",
 				});
 
-				// Now load it
 				const result = await tool.execute("call-2", { action: "load" });
 
 				assert.ok(result.content);
@@ -67,7 +60,6 @@ describe("memory tool", () => {
 			try {
 				const tool = createMemoryTool(dir);
 
-				// Load from a repo with no memory file
 				const result = await tool.execute("call-1", { action: "load" });
 
 				assert.equal(result.content[0].text, "No memories yet.");
@@ -79,7 +71,6 @@ describe("memory tool", () => {
 		it("returns 'No memories yet.' when memory file has only heading", async () => {
 			const dir = await setupRepo();
 			try {
-				// Write the default heading-only memory file
 				await mkdir(join(dir, "memory"), { recursive: true });
 				await writeFile(join(dir, "memory", "MEMORY.md"), "# Memory", "utf-8");
 
@@ -93,8 +84,6 @@ describe("memory tool", () => {
 		});
 	});
 
-	// ── save ─────────────────────────────────────────────────────────
-
 	describe("save", () => {
 		it("writes content and commits to git", async () => {
 			const dir = await setupRepo();
@@ -113,15 +102,13 @@ describe("memory tool", () => {
 				);
 				assert.ok(result.content[0].text.includes("First save"));
 
-				// Verify the file was written
 				const { readFile } = await import("fs/promises");
 				const fileContent = await readFile(
 					join(dir, "memory", "MEMORY.md"),
 					"utf-8",
 				);
 				assert.ok(fileContent.includes("Saved entry one"));
 
-				// Verify the git commit exists
 				const log = execSync("git log --oneline", {
 					cwd: dir,
 					encoding: "utf-8",
@@ -164,7 +151,6 @@ describe("memory tool", () => {
 					() =>
 						tool.execute("call-1", {
 							action: "save",
-							// content intentionally omitted
 						}),
 					/content is required for save action/,
 				);
@@ -174,8 +160,6 @@ describe("memory tool", () => {
 		});
 	});
 
-	// ── abort signal ─────────────────────────────────────────────────
-
 	describe("abort signal", () => {
 		it("throws when signal is already aborted", async () => {
 			const dir = await setupRepo();
diff --git a/test/telemetry-init.test.ts b/test/telemetry-init.test.ts
@@ -0,0 +1,121 @@
+/**
+ * Tests for the telemetry module (src/telemetry.ts) — init/shutdown/idempotency.
+ *
+ * These tests verify that initTelemetry correctly gates on the OTLP
+ * endpoint environment variable: it MUST return without enabling telemetry
+ * when no endpoint is configured, and it MUST successfully create an SDK
+ * instance when an endpoint (or test provider) is provided.
+ */
+import { describe, it, before, afterEach } from "node:test";
+import assert from "node:assert/strict";
+import { trace } from "@opentelemetry/api";
+import {
+	NodeTracerProvider,
+	InMemorySpanExporter,
+	SimpleSpanProcessor,
+} from "@opentelemetry/sdk-trace-node";
+
+let initTelemetry: typeof import("../src/telemetry.ts").initTelemetry;
+let shutdownTelemetry: typeof import("../src/telemetry.ts").shutdownTelemetry;
+let isTelemetryEnabled: typeof import("../src/telemetry.ts").isTelemetryEnabled;
+
+before(async () => {
+	const mod = await import("../src/telemetry.ts");
+	initTelemetry = mod.initTelemetry;
+	shutdownTelemetry = mod.shutdownTelemetry;
+	isTelemetryEnabled = mod.isTelemetryEnabled;
+});
+
+afterEach(async () => {
+	await shutdownTelemetry();
+	try {
+		trace.disable();
+	} catch {
+		/* ignore */
+	}
+});
+
+describe("telemetry init", () => {
+	function makeTestProvider() {
+		const exporter = new InMemorySpanExporter();
+		const provider = new NodeTracerProvider({
+			spanProcessors: [new SimpleSpanProcessor(exporter)],
+		});
+		return { exporter, provider };
+	}
+
+	it("returns without enabling telemetry when no OTLP endpoint is configured", async () => {
+		const saved = process.env.OTEL_EXPORTER_OTLP_ENDPOINT;
+		const wasSet = "OTEL_EXPORTER_OTLP_ENDPOINT" in process.env;
+		delete process.env.OTEL_EXPORTER_OTLP_ENDPOINT;
+
+		try {
+			await assert.doesNotReject(
+				() => initTelemetry({}),
+				"initTelemetry must never throw, even without an endpoint",
+			);
+
+			assert.equal(
+				isTelemetryEnabled(),
+				false,
+				"telemetry must remain disabled when no endpoint is configured",
+			);
+		} finally {
+			if (wasSet) {
+				process.env.OTEL_EXPORTER_OTLP_ENDPOINT = saved;
+			} else {
+				delete process.env.OTEL_EXPORTER_OTLP_ENDPOINT;
+			}
+		}
+	});
+
+	it("creates an SDK instance when endpoint is configured", async () => {
+		process.env.OTEL_EXPORTER_OTLP_ENDPOINT = "http://localhost:4318";
+		const { exporter, provider } = makeTestProvider();
+
+		try {
+			await initTelemetry({
+				serviceName: "test-svc",
+				_testProvider: provider,
+			});
+
+			assert.equal(
+				isTelemetryEnabled(),
+				true,
+				"telemetry must be enabled after initTelemetry with _testProvider",
+			);
+
+			const tracer = trace.getTracer("test");
+			const span = tracer.startSpan("test-span");
+			span.end();
+
+			await provider.forceFlush();
+			const spans = exporter.getFinishedSpans();
+			assert.equal(spans.length, 1, "span should be exported");
+			assert.equal(spans[0].name, "test-span");
+		} finally {
+			delete process.env.OTEL_EXPORTER_OTLP_ENDPOINT;
+		}
+	});
+
+	it("is idempotent", async () => {
+		const { provider: provider1 } = makeTestProvider();
+		const { provider: provider2 } = makeTestProvider();
+
+		await initTelemetry({ _testProvider: provider1 });
+		assert.equal(isTelemetryEnabled(), true);
+
+		await initTelemetry({ _testProvider: provider2 });
+		assert.equal(isTelemetryEnabled(), true);
+	});
+
+	it("shutdownTelemetry resets the initialized state", async () => {
+		const { provider } = makeTestProvider();
+
+		await initTelemetry({ _testProvider: provider });
+		assert.equal(isTelemetryEnabled(), true);
+
+		await shutdownTelemetry();
+		assert.equal(isTelemetryEnabled(), false);
+	});
+});
diff --git a/tsconfig.json b/tsconfig.json
