fix: update npm package name and Node version; test: implement memory and telemetry tests

[Oxygen56]

Summary

Two changes:

1. Bugfixes

• install.sh: Fix outdated unscoped npm package name (gitagent → @open-gitagent/gitagent) and incorrect Node.js version check (≥18 → ≥20 to match package.json engines field)
• Documentation.md: Fix stale version string (1.3.3 → 1.5.2) and same npm package name issue
• README.md: Fix npm badge URL, Node version requirement, and FAQ reference

2. Tests

• src/tools/tests/memory.test.ts: 7 real tests replacing 3 it.todo() stubs — covering load (stored content, empty/missing file, heading-only file), save (writes content+git commit, default message, content required validation), and abort signal handling
• src/tests/telemetry.test.ts: 4 real tests replacing 2 it.todo() stubs — covering graceful init without endpoint, init with test provider, idempotency, and shutdown state reset

Test Plan

• All 27 existing tests pass with no regressions
• All 11 new tests pass (7 memory + 4 telemetry)
• Build succeeds with tsc
• install.sh version check correctly requires Node 20+

[shreyas-lyzr]

Review Summary

Two blocking bugs in the test imports will prevent the test suite from running. Once fixed, this PR correctly updates stale docs and adds valuable test coverage.

---

Blocking Issues

1. Incorrect OpenTelemetry imports

src/tests/telemetry.test.ts:15-16

InMemorySpanExporter and SimpleSpanProcessor are imported from @opentelemetry/sdk-trace-node, but these are not exported from that package. They live in @opentelemetry/sdk-trace-base.

Impact: Tests fail at runtime with module not found errors.

Fix:

import { InMemorySpanExporter, SimpleSpanProcessor } from '@opentelemetry/sdk-trace-base';

2. Memory test imports compiled output instead of source

src/tools/tests/memory.test.ts:15

await import('../../../dist/tools/memory.js') assumes compiled output exists before tests run. This breaks npm test before npm run build.

Fix: Import from source like the telemetry test does:

const mod = await import('../memory.js'); // or '../memory.ts'

---

Additional Issues

src/tools/tests/memory.test.ts:102 — Span assertion race condition

Getting finished spans immediately after span.end() may return empty array if SimpleSpanProcessor hasn't flushed. Add:

await provider.forceFlush();
const spans = exporter.getFinishedSpans();

src/tools/tests/memory.test.ts:27-28 — Git config scope

Use --local flag to prevent polluting global git config:

git config --local user.email "test@gitagent.test"

---

What Looks Good

• Documentation fixes are all correct (npm package name, Node version)
• Test structure is solid — good use of helpers, cleanup, and edge case coverage
• The bugfixes in install.sh/docs are real issues worth fixing

[shreyas-lyzr]

Blocking: InMemorySpanExporter and SimpleSpanProcessor are not exported from @opentelemetry/sdk-trace-node. Import from @opentelemetry/sdk-trace-base instead.

[Oxygen56]

Fixed in 4486c21: InMemorySpanExporter and SimpleSpanProcessor now import from @opentelemetry/sdk-trace-base, while NodeTracerProvider stays in @opentelemetry/sdk-trace-node.

[shreyas-lyzr]

Blocking: Importing from dist/ breaks tests run before compilation. Import from source: await import("../memory.js") or "../memory.ts".

[Oxygen56]

Fixed in 4486c21: now imports from source (../memory.ts) instead of dist/, matching the telemetry test pattern.

[shreyas-lyzr]

Suggestion: Add await provider.forceFlush(); before checking finished spans to avoid race condition where spans haven't been exported yet.

[Oxygen56]

Good catch — fixed in 4486c21. Note: the forceFlush() was added to src/__tests__/telemetry.test.ts (line 104, where the span assertion actually lives) rather than this file, since memory.test.ts does not use OpenTelemetry spans. The race condition was in the telemetry test "initTelemetry creates an SDK instance when endpoint is configured" where exporter.getFinishedSpans() is called after span.end(). Added await provider.forceFlush() before reading spans there.

[shreyas-lyzr]

Suggestion: Use git config --local instead of plain git config to scope test user settings to the temp repo only.

[Oxygen56]

Fixed in 4486c21: both git config commands now use --local to explicitly scope to the temporary test repo.

[Oxygen56]

Thanks for the thorough review @shreyas-lyzr! All four issues should be addressed in 4486c21:

1. OpenTelemetry imports: Fixed — InMemorySpanExporter and SimpleSpanProcessor now import from @opentelemetry/sdk-trace-base, with NodeTracerProvider staying in @opentelemetry/sdk-trace-node.

2. Memory test import: Fixed — now imports from source (../memory.ts) instead of compiled dist/, matching the telemetry test pattern.

3. Span flush race: Fixed — added await provider.forceFlush() before getFinishedSpans() so the span is guaranteed to land in the in-memory exporter.

4. Git config scope: Fixed — both git config calls now use --local explicitly.

All 7 memory tests + 4 telemetry tests + 8 existing tests pass with npx tsx --test. Appreciate you catching these — especially the import issues which would have been blockers.

[shreyas-lyzr]

Code Review Analysis

I've analyzed all 338 additions across 7 files. Here's my assessment:

Part 1: Documentation & Install Fixes ✅

Documentation.md:1-4

• Version bump 1.3.3 → 1.5.2: Clean
• npm package gitagent → @open-gitagent/gitagent: Correct

Documentation.md:86

• npm update -g gitagent → npm update -g @open-gitagent/gitagent: Correct

README.md:6

• npm badge URL updated to scoped package: Correct

README.md:59 & 755

• Node requirement 18+ → 20+: Matches package.json engines field

install.sh:94-96

• Version check changed from < 18 to < 20: Correct
• Error message updated to match

package-lock.json:3259

• detect-libc marked "peer": true: This is an auto-generated change, likely from npm install. Not concerning.

Verdict on Part 1: No bugs. All changes are consistency fixes.

---

Part 2: Test Implementation — Memory Tool

src/tools/tests/memory.test.ts

Bug 1: Missing cleanup on test failure (Minor)

Lines 30-34, 70-74, 81-85, 103-107, 135-139, 154-158, 166-170, 180-184

Every test creates a temp directory with setupRepo() but wraps cleanup in finally blocks. This is correct except:

await cleanup(dir);

If the test times out or is externally killed, the finally may not run. Node.js test runner doesn't guarantee finally execution on SIGTERM/SIGKILL.

Severity: Low. Test pollution is contained to /tmp. Worst case: orphaned temp dirs in CI.

Fix: Use after() hook at suite level OR accept this as standard practice (most test frameworks have this issue).

Risk 1: Git config pollution (Low)

Lines 27-28

execSync('git config --local user.email "test@gitagent.test"', { cwd: dir });
execSync('git config --local user.name "Test Agent"', { cwd: dir });

Uses --local flag, so config is scoped to the temp repo. No pollution of global git config. ✅

Bug 2: Hardcoded commit message assertion is brittle (Low)

Line 127

assert.ok(log.includes("First save"), `git log should contain commit: ${log}`);

If git changes its log format or adds unexpected commits (hooks, etc.), this could fail. However, the test controls the repo fully, so this is low risk.

Mitigation present: The test outputs ${log} in the failure message for debugging.

Risk 2: execSync without error handling (Medium → Low)

Lines 122-126

const log = execSync("git log --oneline", {
  cwd: dir,
  encoding: "utf-8",
});

If git is not installed or the repo is corrupted, execSync throws. However:

• The test already creates a git repo in setupRepo(), so git is available
• Test failures are acceptable here — the test should fail if git breaks
• The test framework will catch and report the error

Verdict: Not a bug. This is the expected behavior.

---

Part 3: Test Implementation — Telemetry

src/tests/telemetry.test.ts

Bug 3: Environment variable restoration logic flaw (Medium)

Lines 56-71

const saved = process.env.OTEL_EXPORTER_OTLP_ENDPOINT;
delete process.env.OTEL_EXPORTER_OTLP_ENDPOINT;

try {
  await assert.doesNotReject(
    () => initTelemetry({}),
    "initTelemetry must never throw, even without an endpoint",
  );
} finally {
  if (saved !== undefined) {
    process.env.OTEL_EXPORTER_OTLP_ENDPOINT = saved;
  }
}

The bug: If the original value was undefined (not set), the finally block does NOT restore it — it leaves the key deleted. This is correct for this specific case.

BUT: If saved === "" (empty string), the condition saved !== undefined passes, and the empty string is restored. However, delete was used to remove it, which is asymmetric.

The correct pattern:

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 {
  // test
} finally {
  if (wasSet) {
    process.env.OTEL_EXPORTER_OTLP_ENDPOINT = saved;
  } else {
    delete process.env.OTEL_EXPORTER_OTLP_ENDPOINT;
  }
}

Impact: If another test runs after this one and expects the env var to be unset, but it was originally set to "", it will see "" instead of undefined. This could cause a test pollution issue.

Severity: Medium. The current code works for the common case (undefined → undefined), but fails for the edge case of empty string.

Risk 3: Global telemetry state pollution (Medium → Mitigated)

Lines 31-39

afterEach(async () => {
  await shutdownTelemetry();
  try {
    trace.disable();
  } catch {
    /* ignore */
  }
});

OpenTelemetry's global tracer provider is shared across tests. The afterEach calls shutdownTelemetry() which resets _initialized = false in telemetry.ts, but does NOT unregister the global provider.

Consequence: If test A registers a provider, test B may inherit it even after shutdown.

Mitigation present: The test explicitly calls trace.disable() to attempt cleanup. However, @opentelemetry/api does not expose a disable() method on the trace namespace (it's on the TracerProvider). This line will always throw and be silently caught.

Actual issue: The trace.disable() call does nothing. The real cleanup happens in shutdownTelemetry(), which sets _initialized = false. Future calls to initTelemetry() will re-register a new provider, overwriting the old one.

Verdict: This is likely okay for tests, but the trace.disable() line is dead code and misleading.

Bug 4: Test does not verify _testProvider is actually used (Low)

Lines 76-109

The test passes _testProvider: provider and then verifies spans appear in the exporter. However, it does NOT verify that the production code path (OTLP exporter creation) is skipped. If the code ignored _testProvider and still initialized the OTLP exporter, the test might pass (if the span also went through the test provider).

Mitigation: The test sets OTEL_EXPORTER_OTLP_ENDPOINT, so the production code would attempt to create an OTLPTraceExporter. If that succeeded, the test would still pass. However, there's no real OTLP endpoint, so the production path would likely fail silently (the SDK wraps errors).

Severity: Low. The test is testing the right thing (span export), even if the code path is ambiguous.

---

Part 4: tsconfig.json Change

tsconfig.json:16

"exclude": ["node_modules", "dist", "**/__tests__"]

Previously: ["node_modules", "dist"]

Analysis:

• Excludes all __tests__ directories from compilation
• Correct behavior: Test files should not be compiled into dist/
• Potential issue: If production code accidentally imports from __tests__, TypeScript won't catch it at build time (but the import will fail at runtime)

Verdict: This is standard practice. No bug.

---

Summary: Bugs Found

#	Location	Severity	Description
1	memory.test.ts (multiple)	Low	Temp directory cleanup relies on finally, may not run on external kill
2	memory.test.ts:127	Low	Hardcoded git log assertion could break on unexpected commits
3	telemetry.test.ts:56-71	Medium	Env var restoration doesn't handle empty string case correctly
4	telemetry.test.ts:35	Low	trace.disable() is dead code (method doesn't exist)
5	telemetry.test.ts:76-109	Low	Test doesn't verify production code path is skipped when _testProvider is used

---

Risks Assessed

1. Test pollution (memory.test.ts): Low. Temp dirs are isolated, git config is scoped.
2. Test pollution (telemetry.test.ts): Medium. Global OTel state could leak between tests, but current cleanup is adequate for the test suite's needs.
3. Brittle assertions: Low. Git log checks could theoretically break, but unlikely.
4. Dead code in tests: Low. trace.disable() does nothing but doesn't break anything.

---

Final Verdict

Request changes due to Bug #3 (env var restoration). The other issues are minor and can be addressed in a follow-up or accepted as-is.

Recommended fix:

it("initTelemetry without OTLP endpoint does not throw and leaves module in a consistent state", 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",
    );
  } finally {
-   if (saved !== undefined) {
+   if (wasSet) {
      process.env.OTEL_EXPORTER_OTLP_ENDPOINT = saved;
+   } else {
+     delete process.env.OTEL_EXPORTER_OTLP_ENDPOINT;
    }
  }
});

Alternatively, if you're confident no tests set OTEL_EXPORTER_OTLP_ENDPOINT="", the current code is acceptable.

Optional improvements:

• Remove the dead trace.disable() call (line 35)
• Add a test that verifies _testProvider actually bypasses the production OTLP exporter

But these are nits. The core functionality is solid. All 27 tests pass, no regressions.

[Oxygen56]

Thanks for the thorough line-by-line re-review. Here is how each finding is addressed in a5a7036:

Bug #3 (env var restoration) — fixed. Adopted the "KEY" in process.env pattern you suggested. The wasSet flag handles the empty-string edge case and keeps the restore/delete symmetric.

Bug #4 (trace.disable() dead code) — kept it. I checked the @opentelemetry/api source in node_modules and trace.disable() is a real method on TraceAPI. It calls unregisterGlobal() to remove the global tracer provider, then installs a fresh ProxyTracerProvider. Without it the global tracer provider from a previous test can leak into the next one. The try/catch is there because unregisterGlobal can throw if the API was never registered. Added a comment in the code.

Bug #1 (finally may not run on SIGKILL) — acknowledged, this is universal across all test frameworks. Accepted as-is.

Bug #2 (brittle git log assertion) — acknowledged. The test controls the entire repo and includes ${log} in the failure message for debugging, so risk is minimal.

Bug #5 (test does not verify production path is skipped) — fair observation, but the test verifies correct span export behavior which is the right thing to test, and _testProvider is gated at the top of initTelemetry().

All 11 new tests + 8 existing tests pass with npx tsx --test.