test: add unit coverage for cost-tracker, compact, knowledge, tool-utils - #71
Conversation
These core modules had no unit tests. Added deterministic, offline suites (node:test) covering their pure logic: - cost-tracker: session/per-model aggregation, derived totalTokens, defensive get() copy, reset - compact: token estimation, per-message-type accounting, compaction threshold, tool-result truncation, transcript rendering + compact prompt building - knowledge: index loading (always_load preload, available listing, missing files, malformed index) and prompt formatting - tool-utils: GCToolDefinition → AgentTool mapping, string vs object handler results, abort-signal forwarding All 26 new cases live under test/ so `npm test` runs them. Full suite: 53 passing.
shreyas-lyzr
left a comment
There was a problem hiding this comment.
26 new test cases across four previously-uncovered modules. All look correct.
A few things I checked explicitly:
- estimateTokens arithmetic (ceil(5/4)=2), estimateMessageTokens summation (1+2+51+1=55), and the 75% compaction threshold (800/1000=0.8) all hold.
- totalTokens fallback (input+output when field absent) is tested correctly.
- The defensive-copy test for CostTracker.get() properly verifies internal state survives external mutation of the snapshot.
- knowledge tests use mkdtemp + rm recursive cleanup — no leaked temp dirs.
- before() blocks are correctly placed at module scope, not inside describe(), which is required for node:test.
- All suites are offline — no network, no API keys, no side effects.
The follow-up note in the PR description about test/tests not being wired into the npm test glob is a real gap worth tracking separately.
There was a problem hiding this comment.
Pull request overview
Adds deterministic node:test unit suites for several core “pure logic” modules so their behavior is exercised offline and without external dependencies.
Changes:
- Added new unit tests for
CostTrackersession + per-model aggregation behavior. - Added new unit tests for compaction/token estimation utilities, truncation logic, and transcript/prompt building.
- Added new unit tests for knowledge index loading/formatting and tool-definition conversion utilities.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
test/tool-utils.test.ts |
Tests toAgentTool mapping behavior, handler result normalization, and abort-signal forwarding. |
test/knowledge.test.ts |
Tests knowledge index loading (including malformed/missing cases) and prompt formatting output. |
test/cost-tracker.test.ts |
Tests CostTracker accumulation, per-model aggregation, defensive snapshotting, and reset behavior. |
test/compact.test.ts |
Tests token estimation, compaction thresholding, tool-result truncation, transcript rendering, and compact-prompt building. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
execute() returns {content, details} via buildTool()'s wrapping (see
sdk.test.ts), not a raw string. Two assertions in mcp.test.ts predated
that contract and were comparing the wrapper object directly against a
string.
shreyas-lyzr
left a comment
There was a problem hiding this comment.
Good addition. Reviewed the four new test suites against their source implementations and ran the full suite locally after building.
Verified:
- cost-tracker: all six cases map cleanly to the implementation. The defensive-copy test correctly validates that key deletion on a snapshot doesn't corrupt internal state (shallow copy of modelUsage object is sufficient for that guarantee).
- compact: token arithmetic checks out — ceil(5/4)=2, tool_use overhead of 50 added, needsCompaction threshold at 0.75 all match the source. truncateToolResults head/tail splitting and messagesToText filtering (drops delta/system) are correctly exercised.
- knowledge: temp-dir setup/teardown is clean, covers all four index loading paths (missing, always_load, missing file skip, malformed YAML), and the formatKnowledgeForPrompt assertion matches the exact tag/attribute ordering the source produces.
- tool-utils: covers the two handler result shapes (string vs object with details) and abort signal forwarding, which are the only real branches in toAgentTool.
- mcp.test.ts fixes: the four changed assertions update from the old string return shape to the current content[0].text shape — correct.
Full suite: 65 pass, 0 fail.
Security pass: no new dependencies added, no secrets or credentials in the diff, no injection surface (pure offline unit tests with temp dirs).
One thing worth a follow-up (noted in the PR description): the existing suites under src/**/tests/ and src/tests/telemetry.test.ts are not picked up by the test glob and never run in CI. Worth wiring those in separately.
Minor release — first release to include MCP client support. - feat: MCP client support (#59) — connect stdio + http/sse MCP servers via QueryOptions.mcpServers / agent.yaml mcp_servers; tools are namespaced <server>__<tool> and torn down on every exit path. Adds @modelcontextprotocol/sdk. - test: unit coverage for cost-tracker, compact, knowledge, tool-utils (#71) Built from main (d3e25d7). Supersedes the deprecated 2.0.1. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Summary
Several core modules shipped with no unit tests. This PR adds deterministic, offline
node:testsuites for the pure logic in four of them — +26 test cases, all placed undertest/sonpm testactually runs them.Coverage before / after
Existing tests covered
sdk(tool helper, schema builder, programmatic hooks, query error paths) andtelemetry. These modules had none:cost-tracker.tstotalTokensfallback, defensiveget()copy,reset()compact.tsestimateTokensheuristic, per-message-type accounting (incl. tool_use overhead), 75% compaction threshold, tool-result head/tail truncation, transcript rendering (drops deltas/system noise), compact-prompt buildingknowledge.tsalways_loadpreload + trim, available listing, missing-file skip, malformed index → empty; prompt formatting (inline docs + read hint)tool-utils.tsGCToolDefinition → AgentToolmapping, string vs{text, details}handler results, abort-signal forwardingNotes
Follow-up worth flagging (not in this PR)
The
npm testscript globs onlytest/*.test.ts, so the existing suites undersrc/**/__tests__/(memory, sandbox-memory, skill-learner, task-tracker) andsrc/__tests__/telemetry.test.tsare never run by CI. Wiring those into the test script (or moving them undertest/) would be a good separate cleanup.