test(core): add unit tests for isToolBuilder and ensureBuilt by gabrypavanello · Pull Request #182 · AndurilCode/mcp-apps-kit

gabrypavanello · 2026-02-24T03:04:03Z

What

Add 5 unit tests for two helpers in packages/core/src/builder/ensure-built.ts that were publicly exported but had zero test coverage.

isToolBuilder

Returns true for an unbuilt builder
Returns false for a built ToolDef
Returns false for primitives (null, undefined, number, string, plain object)

ensureBuilt

Builds a raw builder into a ToolDef with the inferred name applied
Returns a pre-built ToolDef by reference, unchanged

Why

The tech-debt scanner flagged ensure-built.ts as untested. These helpers are used by the codegen pipeline to auto-build tools that omit .build() — worth having regression coverage.

Checklist

All 832 tests pass (pnpm run test)
Build passes (pnpm run build)
No production code changed (tests only)

Kaizen 2026-02-24 — one small fix per day.

Both helpers in src/builder/ensure-built.ts were exported from core's public index but had zero test coverage. Add 5 focused tests covering: - isToolBuilder: true for unbuilt builder, false for built ToolDef, false for primitives (null/undefined/number/string/object) - ensureBuilt: builds a raw builder into a ToolDef, returns a pre-built ToolDef by reference unchanged Kaizen 2026-02-24.

coderabbitai · 2026-02-24T03:04:20Z

📝 Walkthrough

Summary by CodeRabbit

Tests
- Enhanced test coverage for tool builder runtime utilities with new test cases validating builder validation and construction workflows.

Walkthrough

Adds unit tests for tool builder runtime utilities (isToolBuilder and ensureBuilt) in the core test suite. The tests verify behavior when processing unbuilt tool builders and already-built ToolDef instances.

Changes

Cohort / File(s)	Summary
Tool Builder Tests `packages/core/tests/unit/builder.test.ts`	New test cases for `isToolBuilder` and `ensureBuilt` utilities, verifying correct identification of tool builders, construction of ToolDef instances, and passthrough behavior for already-built definitions.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately and concisely describes the main change: adding unit tests for isToolBuilder and ensureBuilt functions.
Description check	✅ Passed	The description clearly relates to the changeset, explaining what tests were added, why they matter, and providing verification that existing tests pass.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings (stacked PR)
📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch kaizen/2026-02-24

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

🧹 Nitpick comments (1)

packages/core/tests/unit/builder.test.ts (1)
4-4: Consider importing from the public index for consistency.

Both isToolBuilder and ensureBuilt are re-exported from ../../src/index (confirmed in packages/core/src/index.ts lines 314–315), yet this import reaches into the internal module path while line 3 uses the public surface. Importing from the index also verifies the re-export itself.
♻️ Suggested change
-import { isToolBuilder, ensureBuilt } from "../../src/builder/ensure-built";
+import { isToolBuilder, ensureBuilt } from "../../src/index";
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/core/tests/unit/builder.test.ts` at line 4, The test imports
isToolBuilder and ensureBuilt from an internal path; change the import to use
the public package surface (import from "../../src/index" or simply "../../src")
so the test uses the re-exports and verifies them; update the import statement
that currently references "../../src/builder/ensure-built" to import {
isToolBuilder, ensureBuilt } from the package's public index (matching the
existing line that imports from the public surface).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@packages/core/tests/unit/builder.test.ts`:
- Line 4: The test imports isToolBuilder and ensureBuilt from an internal path;
change the import to use the public package surface (import from
"../../src/index" or simply "../../src") so the test uses the re-exports and
verifies them; update the import statement that currently references
"../../src/builder/ensure-built" to import { isToolBuilder, ensureBuilt } from
the package's public index (matching the existing line that imports from the
public surface).

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 45ec721 and bf6b057.

📒 Files selected for processing (1)

packages/core/tests/unit/builder.test.ts

claude · 2026-02-24T03:08:46Z

test

claude · 2026-02-24T03:09:09Z

placeholder test 2

claude · 2026-02-24T03:09:20Z

This is a blockquote test

claude · 2026-02-24T03:09:30Z

.handle(async () => ({}))

claude · 2026-02-24T03:09:46Z

Code Review: PR #182 — `test(core): add unit tests for isToolBuilder and ensureBuilt`

Good addition overall — the PR is focused, no production code is touched, and the test structure is clean. A few issues are worth addressing before merging.

Bug / Coverage Gap: `ensureBuilt` name-inference path is not exercised

The ensureBuilt test creates a builder with an explicit name:

const builder = tool("myTool")
  .describe("Greet")
  .input(z.object({}))
  .handle(async () => ({}));
const def = ensureBuilt(builder, "greetTool");

Because _setName uses ??= (only sets if this._name is nullish), and tool("myTool") already sets the name in the constructor, _setName("greetTool") is a no-op. The inferred name "greetTool" is silently ignored.

The test then checks def.description, def.handler, and def.input — but never asserts def.name. As written, the test doesn't actually exercise the documented primary purpose of ensureBuilt:

The ToolBuilderImpl constructor accepts an optional name (constructor(name?: TName)), so a nameless builder can be created directly. A test like the following would cover the real path:

it("applies the inferred name when no name was pre-set", () => {
  const builder = new ToolBuilderImpl()
    .describe("Greet")
    .input(z.object({}))
    .handle(async () => ({}));
  const def = ensureBuilt(builder, "greetTool");
  expect(def.name).toBe("greetTool");
});

Without this, the name-inference branch is still uncovered.

claude · 2026-02-24T03:10:57Z

simple test 3

claude · 2026-02-24T03:11:08Z

CLAUDE.md

This repository’s primary agent instructions live in AGENTS.md.

Before starting any work, read ./AGENTS.md and follow it as the single source of truth.

claude · 2026-02-24T03:11:22Z

@context (Mission)

Build a single TypeScript codebase that runs interactive apps on MCP Apps and ChatGPT via a shared tool + UI model. Optimize for strict type-safety, clean public APIs, and backwards-compatible changes across packages.

@stack (Technical DNA)

Monorepo: pnpm workspaces + Nx (nx run-many)
Language: TypeScript (strict, noImplicitAny, noUncheckedIndexedAccess, verbatimModuleSyntax)
Runtime: Node.js >= 18 (runtime packages), >= 20 (development/CLI/CI; don't change without updating engines/tooling)
Schema/validation: Zod v4 (not v3)
Server: Express v5 (not v4) via @mcp-apps-kit/core
Testing: Vitest; repo-level coverage thresholds enforced (50% lines/functions/branches/statements)
Lint/format: ESLint (flat config) + Prettier; pre-commit runs lint-staged
Build: tsup per package; TypeDoc for docs (pnpm docs)

@knowledge Graph (Context Links)

@Map (File Structure)

packages/core/: server framework (createApp, adapters, middleware, plugins, OAuth). Public API is re-exported from src/index.ts.
packages/ui/: client SDK (protocol detection, adapters, theming helpers). Must support both MCP Apps + ChatGPT host shapes.
packages/ui-react/: React bindings (provider + hooks). Hooks must be usable without host-specific branching in app code.
packages/ui-react-builder/: build tooling to bundle React components into self-contained HTML; includes optional Vite plugin.
packages/testing/: test utilities (test server/client, matchers, property/behavior/LLM eval helpers). Clean up servers in tests.
packages/create-app/: CLI scaffolder (templates + integration tests). Generated projects intentionally avoid pnpm workspace coupling.
examples/: reference integrations; use for manual smoke checks (don’t copy docs into core packages).

@workflow (How To Work Here)

Build: pnpm build
Test: pnpm test (or faster: pnpm -C packages/<pkg> test)
Lint: pnpm lint
Format: pnpm format (check) / pnpm format:write (fix)
Type-check: pnpm typecheck
Run example: pnpm -C examples/minimal dev

@rules (Dos & Don’ts)

ALWAYS run pnpm build && pnpm test && pnpm lint && pnpm typecheck before finishing any task (repo policy; broken builds block everyone).
NEVER use Zod v3 or Express v4 assumptions; this repo is Zod 4 + Express 5.
NEVER introduce any in production code; use unknown + narrowing (ESLint enforces @typescript-eslint/no-explicit-any).
NEVER bypass the export contract: public APIs must flow through each package’s src/index.ts (avoid deep imports across packages).
ALWAYS await next() in Koa-style middleware; failing to do so silently breaks the middleware chain.
ALWAYS add/adjust tests under the owning package’s tests/ when changing behavior; coverage thresholds are enforced at the repo level.
AVOID cross-package circular dependencies; prefer moving shared types/utilities “down” (or into a dedicated package) instead of importing “up”.

@memory (Self-Correction Loop)

If you encounter repeated errors (for example, failing commands, misused scripts, incorrect imports, or configuration pitfalls) or discover a new best practice while working in this repository, you MUST update this file. Add the specific failure case and its correct resolution to @rules or @workflow so future agents do not repeat the same mistake.

claude · 2026-02-24T03:11:38Z

CLAUDE.md

This repository’s primary agent instructions live in AGENTS.md.

Before starting any work, read ./AGENTS.md and follow it as the single source of truth.

claude · 2026-02-24T03:12:38Z

import { describe, it, expect, expectTypeOf } from "vitest";
import { z } from "zod";
import { tool } from "../../src/index";
import { isToolBuilder, ensureBuilt } from "../../src/builder/ensure-built";
import type { ToolDef } from "../../src/types/tools";

describe("Tool Builder", () => {
it("builds a minimal tool", () => {
const t = tool("greet")
.describe("Greet")
.input(z.object({ name: z.string() }))
.handle(async ({ name }) => ({ message: Hi ${name} }))
.build();

expect(t.description).toBe("Greet");
expect(t.input).toBeInstanceOf(z.ZodObject);
expect(t.handler).toBeDefined();

});

it("applies annotations via shortcuts", () => {
const t = tool("read")
.describe("Read")
.input(z.object({}))
.output(z.object({ data: z.string() }))
.readOnly()
.expensive()
.idempotent()
.destructive()
.handle(async () => ({ data: "test" }))
.build();

expect(t.annotations).toEqual({
  readOnlyHint: true,
  openWorldHint: true,
  idempotentHint: true,
  destructiveHint: true,
});

});

it("applies visibility", () => {
const t = tool("vis")
.describe("Visible")
.input(z.object({}))
.visibility("model")
.handle(async () => ({}))
.build();

expect(t.visibility).toBe("model");

});

it("applies title", () => {
const t = tool("titled")
.describe("Described")
.title("My Title")
.input(z.object({}))
.handle(async () => ({}))
.build();

expect(t.title).toBe("My Title");

});

it("applies UI", () => {
// String reference (legacy/config key)
const t1 = tool("ui1")
.describe("UI 1")
.input(z.object({}))
.uiRef("my-ui")
.handle(async () => ({}))
.build();
expect(t1.ui).toBe("my-ui");

// Path (creates UIDef)
const t2 = tool("ui2")
  .describe("UI 2")
  .input(z.object({}))
  .ui("widget.html")
  .handle(async () => ({}))
  .build();
expect(t2.ui).toEqual({ html: "widget.html" });

// Object definition
const t3 = tool("ui3")
  .describe("UI 3")
  .input(z.object({}))
  .ui({ html: "<div></div>" })
  .handle(async () => ({}))
  .build();
expect(t3.ui).toEqual({ html: "<div></div>" });

});

it("throws if required steps are missing", () => {
const builder = tool("bad") as any;
expect(() => builder.build()).toThrow("Tool requires description");

builder.describe("desc");
expect(() => builder.build()).toThrow("Tool requires input schema");

builder.input(z.object({}));
expect(() => builder.build()).toThrow("Tool requires handler");

});

it("infers types correctly", () => {
const t = tool("infer")
.describe("Infer")
.input(z.object({ val: z.number() }))
.output(z.object({ str: z.string() }))
.handle(async ({ val }) => ({ str: String(val) }))
.build();

expectTypeOf(t).toMatchTypeOf<ToolDef>();

});
it("handles schema normalization", () => {
// Empty object -> z.object({})
const t1 = tool("empty")
.describe("Empty")
.input({})
.handle(async () => ({}))
.build();
expect(t1.input).toBeInstanceOf(z.ZodObject);
expect((t1.input as z.ZodObject).shape).toEqual({});

// Inline shape -> z.object({...})
const t2 = tool("inline")
  .describe("Inline")
  .input({ name: z.string() })
  .handle(async () => ({}))
  .build();
expect(t2.input).toBeInstanceOf(z.ZodObject);
expect((t2.input as z.ZodObject<any>).shape.name).toBeInstanceOf(z.ZodString);

});

it("normalizes visibility aliases", () => {
const t1 = tool("v1")
.describe("V1")
.input(z.object({}))
.visibility("mcp")
.handle(async () => ({}))
.build();
expect(t1.visibility).toBe("app");

const t2 = tool("v2")
  .describe("V2")
  .input(z.object({}))
  .visibility("chatgpt")
  .handle(async () => ({}))
  .build();
expect(t2.visibility).toBe("model");

});

it("applies optional configuration methods", () => {
const t = tool("opt")
.describe("Optional")
.input(z.object({}))
.widgetAccessible(true)
.invokingMessage("invok")
.invokedMessage("invokD")
.fileParams(["file1"])
.handle(async () => ({}))
.build();

expect(t.widgetAccessible).toBe(true);
expect(t.invokingMessage).toBe("invok");
expect(t.invokedMessage).toBe("invokD");
expect(t.fileParams).toEqual(["file1"]);

});

it("handles UI heuristics and edge cases", () => {
// "my-layout" -> key (no dot, no slash)
const t1 = tool("key1")
.describe("Key 1")
.input(z.object({}))
.ui("my-layout")
.handle(async () => ({}))
.build();
expect(t1.ui).toBe("my-layout");

// "my.ui" -> path because of extension detection
const t2 = tool("path_ext")
  .describe("Path Ext")
  .input(z.object({}))
  .ui("my.ui")
  .handle(async () => ({}))
  .build();
expect(t2.ui).toEqual({ html: "my.ui" });

// "my.ui" enforced as key using uiRef
const t3 = tool("key_ref")
  .describe("Key Ref")
  .input(z.object({}))
  .uiRef("my.ui")
  .handle(async () => ({}))
  .build();
expect(t3.ui).toBe("my.ui");

// "section/widget" -> path because of slash
const t4 = tool("path_slash")
  .describe("Path Slash")
  .input(z.object({}))
  .ui("section/widget")
  .handle(async () => ({}))
  .build();
expect(t4.ui).toEqual({ html: "section/widget" });

// Inline HTML
const t5 = tool("html")
  .describe("HTML")
  .input(z.object({}))
  .ui("<div></div>")
  .handle(async () => ({}))
  .build();
expect(t5.ui).toEqual({ html: "<div></div>" });

});

it("enforces runtime safety for context", () => {
const t = tool("safe") as any;

// Extract method to lose context
const build = t.build;
expect(() => build.call({})).toThrow("ToolBuilder method called with invalid context");

// output
const output = t.output;
expect(() => output.call({}, z.object({}))).toThrow(
  "ToolBuilder method called with invalid context"
);

// handle
const handle = t.handle;
expect(() => handle.call({}, async () => ({}))).toThrow(
  "ToolBuilder method called with invalid context"
);

});
});

describe("isToolBuilder", () => {
it("returns true for an unbuilt tool builder", () => {
const builder = tool("check")
.describe("Check")
.input(z.object({}))
.handle(async () => ({}));
expect(isToolBuilder(builder)).toBe(true);
});

it("returns false for a built ToolDef", () => {
const def = tool("check")
.describe("Check")
.input(z.object({}))
.handle(async () => ({}))
.build();
expect(isToolBuilder(def)).toBe(false);
});

it("returns false for primitives", () => {
expect(isToolBuilder(null)).toBe(false);
expect(isToolBuilder(undefined)).toBe(false);
expect(isToolBuilder(42)).toBe(false);
expect(isToolBuilder("string")).toBe(false);
expect(isToolBuilder({})).toBe(false);
});
});

describe("ensureBuilt", () => {
it("builds an unbuilt builder and returns a ToolDef", () => {
const builder = tool("myTool")
.describe("Greet")
.input(z.object({}))
.handle(async () => ({}));
const def = ensureBuilt(builder, "greetTool");
expect(def.description).toBe("Greet");
expect(typeof def.handler).toBe("function");
expect(def.input).toBeInstanceOf(z.ZodObject);
});

it("returns an already-built ToolDef unchanged (same reference)", () => {
const def = tool("existing")
.describe("Existing")
.input(z.object({}))
.handle(async () => ({}))
.build();
const result = ensureBuilt(def, "shouldBeIgnored");
expect(result).toBe(def);
expect(result.description).toBe("Existing");
});
});

claude · 2026-02-24T03:13:07Z

CLAUDE.md

This repository’s primary agent instructions live in AGENTS.md.

Before starting any work, read ./AGENTS.md and follow it as the single source of truth.

claude · 2026-02-24T03:13:27Z

.handle(async () => ({}))

claude · 2026-02-24T03:13:37Z

test with backticks: ensureBuilt

claude · 2026-02-24T03:14:19Z

Summary

	Finding
Bug	`ensureBuilt` name-inference path is never exercised; the main `ensureBuilt` test effectively only covers the "builder-to-ToolDef conversion" path with a pre-named builder, not the "infer name when absent" codegen path
Minor	No `def.name` assertion in the pre-named builder test
Minor	No type-guard narrowing assertion for `isToolBuilder` (optional)
Non-issue	Deep import pattern is consistent with existing tests in this package

The name-inference gap is the substantive one — worth fixing before this lands to ensure the coverage goal is actually met.

claude · 2026-02-24T03:19:39Z

@context (Mission)

Build a single TypeScript codebase that runs interactive apps on MCP Apps and ChatGPT via a shared tool + UI model. Optimize for strict type-safety, clean public APIs, and backwards-compatible changes across packages.

@stack (Technical DNA)

Monorepo: pnpm workspaces + Nx (nx run-many)
Language: TypeScript (strict, noImplicitAny, noUncheckedIndexedAccess, verbatimModuleSyntax)
Runtime: Node.js >= 18 (runtime packages), >= 20 (development/CLI/CI; don't change without updating engines/tooling)
Schema/validation: Zod v4 (not v3)
Server: Express v5 (not v4) via @mcp-apps-kit/core
Testing: Vitest; repo-level coverage thresholds enforced (50% lines/functions/branches/statements)
Lint/format: ESLint (flat config) + Prettier; pre-commit runs lint-staged
Build: tsup per package; TypeDoc for docs (pnpm docs)

@knowledge Graph (Context Links)

@Map (File Structure)

packages/core/: server framework (createApp, adapters, middleware, plugins, OAuth). Public API is re-exported from src/index.ts.
packages/ui/: client SDK (protocol detection, adapters, theming helpers). Must support both MCP Apps + ChatGPT host shapes.
packages/ui-react/: React bindings (provider + hooks). Hooks must be usable without host-specific branching in app code.
packages/ui-react-builder/: build tooling to bundle React components into self-contained HTML; includes optional Vite plugin.
packages/testing/: test utilities (test server/client, matchers, property/behavior/LLM eval helpers). Clean up servers in tests.
packages/create-app/: CLI scaffolder (templates + integration tests). Generated projects intentionally avoid pnpm workspace coupling.
examples/: reference integrations; use for manual smoke checks (don’t copy docs into core packages).

@workflow (How To Work Here)

Build: pnpm build
Test: pnpm test (or faster: pnpm -C packages/<pkg> test)
Lint: pnpm lint
Format: pnpm format (check) / pnpm format:write (fix)
Type-check: pnpm typecheck
Run example: pnpm -C examples/minimal dev

@rules (Dos & Don’ts)

ALWAYS run pnpm build && pnpm test && pnpm lint && pnpm typecheck before finishing any task (repo policy; broken builds block everyone).
NEVER use Zod v3 or Express v4 assumptions; this repo is Zod 4 + Express 5.
NEVER introduce any in production code; use unknown + narrowing (ESLint enforces @typescript-eslint/no-explicit-any).
NEVER bypass the export contract: public APIs must flow through each package’s src/index.ts (avoid deep imports across packages).
ALWAYS await next() in Koa-style middleware; failing to do so silently breaks the middleware chain.
ALWAYS add/adjust tests under the owning package’s tests/ when changing behavior; coverage thresholds are enforced at the repo level.
AVOID cross-package circular dependencies; prefer moving shared types/utilities “down” (or into a dedicated package) instead of importing “up”.

@memory (Self-Correction Loop)

If you encounter repeated errors (for example, failing commands, misused scripts, incorrect imports, or configuration pitfalls) or discover a new best practice while working in this repository, you MUST update this file. Add the specific failure case and its correct resolution to @rules or @workflow so future agents do not repeat the same mistake.

claude · 2026-02-24T03:22:39Z

test 99

claude · 2026-02-24T03:25:21Z

test > ()

claude · 2026-02-24T03:27:29Z

test>a

claude · 2026-02-24T03:27:46Z

CLAUDE.md

This repository’s primary agent instructions live in AGENTS.md.

Before starting any work, read ./AGENTS.md and follow it as the single source of truth.

claude · 2026-02-24T03:29:28Z

CLAUDE.md

This repository’s primary agent instructions live in AGENTS.md.

Before starting any work, read ./AGENTS.md and follow it as the single source of truth.

claude · 2026-02-24T03:34:18Z

CLAUDE.md

This repository’s primary agent instructions live in AGENTS.md.

Before starting any work, read ./AGENTS.md and follow it as the single source of truth.

CLAUDE.md

This repository’s primary agent instructions live in AGENTS.md.

Before starting any work, read ./AGENTS.md and follow it as the single source of truth.

claude · 2026-02-24T03:35:02Z

test backtick text

claude · 2026-02-24T03:35:14Z

"test blockquote with apostrophe: don't"

claude · 2026-02-24T03:36:10Z

Code Review: PR #182 — `test(core): add unit tests for isToolBuilder and ensureBuilt`

Good addition overall — the PR is focused, no production code is touched, and the test structure is clean. A few issues are worth addressing before merging.

Bug / Coverage Gap: `ensureBuilt` name-inference path is not exercised

The ensureBuilt test creates a builder with an explicit name:

const builder = tool("myTool")
  .describe("Greet")
  .input(z.object({}))
  .handle(async () => ({}));
const def = ensureBuilt(builder, "greetTool");

Because _setName uses ??= (only sets if this._name is nullish), and tool("myTool") already sets the name in the constructor, _setName("greetTool") is a no-op. The inferred name "greetTool" is silently ignored.

The test then checks def.description, def.handler, and def.input — but never asserts def.name. As written, the test doesn't actually exercise the documented primary purpose of ensureBuilt:

"Sets the inferred name (if not already set)" — the codegen use case where the builder is created without a name and the filename-derived name is injected.

The ToolBuilderImpl constructor accepts an optional name (constructor(name?: TName)), so a nameless builder can be created directly. A test like the following would cover the real path:

it("applies the inferred name when no name was pre-set", () => {
  const builder = new ToolBuilderImpl()
    .describe("Greet")
    .input(z.object({}))
    .handle(async () => ({}));
  const def = ensureBuilt(builder, "greetTool");
  expect(def.name).toBe("greetTool");
});

Without this, the name-inference branch is still uncovered.

Minor: no assertion on `name` in the existing `ensureBuilt` test

Even for the pre-named builder test, it would be clearer to assert what the resulting def.name is (i.e., "myTool", confirming _setName did not clobber a pre-set name). This makes the intent explicit and protects against future regressions.

Minor: missing type-narrowing assertion for `isToolBuilder`

isToolBuilder is a TypeScript type guard (value is ToolBuilderImpl<string>). The runtime behavior is tested, but the type narrowing is not verified. The existing suite already uses expectTypeOf (see builder.test.ts:110). A simple addition would be:

it("narrows the type to ToolBuilderImpl after positive check", () => {
  const builder = tool("check").describe("Check").input(z.object({})).handle(async () => ({}));
  if (isToolBuilder(builder)) {

claude · 2026-02-24T03:42:03Z

expectTypeOf(builder).toMatchTypeOf<ToolBuilderImpl<string>>();

}
});

claude · 2026-02-24T03:42:31Z

This is minor and can be skipped if type tests are considered out of scope for this PR.

Not an issue: deep import of `../../src/builder/ensure-built`

This follows the established pattern in the same package — adapters.test.ts imports from ../../src/adapters and errors.test.ts imports from ../../src/utils/errors. The AGENTS.md rule about avoiding deep imports applies across packages, not within a package's own test suite.

Summary

	Finding
Bug	`ensureBuilt` name-inference path is never exercised; the main `ensureBuilt` test effectively only covers the "builder-to-ToolDef conversion" path with a pre-named builder, not the "infer name when absent" codegen path
Minor	No `def.name` assertion in the pre-named builder test
Minor	No type-guard narrowing assertion for `isToolBuilder` (optional)
Non-issue	Deep import pattern is consistent with existing tests in this package

The name-inference gap is the substantive one — worth fixing before this lands to ensure the coverage goal is actually met.

claude · 2026-02-24T03:45:57Z

expectTypeOf(builder).toMatchTypeOf<ToolBuilderImpl<string>>();

}
});

claude · 2026-02-24T03:46:15Z

Code Review: PR #182 — `test(core): add unit tests for isToolBuilder and ensureBuilt`

Good addition overall — the PR is focused, no production code is touched, and the test structure is clean. A few issues are worth addressing before merging.

Bug / Coverage Gap: `ensureBuilt` name-inference path is not exercised

The ensureBuilt test creates a builder with an explicit name:

const builder = tool("myTool")
  .describe("Greet")
  .input(z.object({}))
  .handle(async () => ({}));
const def = ensureBuilt(builder, "greetTool");

Because _setName uses ??= (only sets if this._name is nullish), and tool("myTool") already sets the name in the constructor, _setName("greetTool") is a no-op. The inferred name "greetTool" is silently ignored.

The test then checks def.description, def.handler, and def.input — but never asserts def.name. As written, the test doesn't actually exercise the documented primary purpose of ensureBuilt:

"Sets the inferred name (if not already set)" — the codegen use case where the builder is created without a name and the filename-derived name is injected.

The ToolBuilderImpl constructor accepts an optional name (constructor(name?: TName)), so a nameless builder can be created directly. A test like the following would cover the real path:

it("applies the inferred name when no name was pre-set", () => {
  const builder = new ToolBuilderImpl()
    .describe("Greet")
    .input(z.object({}))
    .handle(async () => ({}));
  const def = ensureBuilt(builder, "greetTool");
  expect(def.name).toBe("greetTool");
});

Without this, the name-inference branch is still uncovered.

Minor: no assertion on `name` in the existing `ensureBuilt` test

Even for the pre-named builder test, it would be clearer to assert what the resulting def.name is (i.e., "myTool", confirming _setName did not clobber a pre-set name). This makes the intent explicit and protects against future regressions.

Minor: missing type-narrowing assertion for `isToolBuilder`

isToolBuilder is a TypeScript type guard (value is ToolBuilderImpl<string>). The runtime behavior is tested, but the type narrowing is not verified. The existing suite already uses expectTypeOf (see builder.test.ts:110). A simple addition would be:

it("narrows the type to ToolBuilderImpl after positive check", () => {
  const builder = tool("check").describe("Check").input(z.object({})).handle(async () => ({}));
  if (isToolBuilder(builder)) {
    expectTypeOf(builder).toMatchTypeOf<ToolBuilderImpl<string>>();
  }
});

This is minor and can be skipped if type tests are considered out of scope for this PR.

Not an issue: deep import of `../../src/builder/ensure-built`

This follows the established pattern in the same package — adapters.test.ts imports from ../../src/adapters and errors.test.ts imports from ../../src/utils/errors. The AGENTS.md rule about avoiding deep imports applies across packages, not within a package's own test suite.

Summary

	Finding
Bug	`ensureBuilt` name-inference path is never exercised; the main `ensureBuilt` test effectively only covers the "builder-to-ToolDef conversion" path with a pre-named builder, not the "infer name when absent" codegen path
Minor	No `def.name` assertion in the pre-named builder test
Minor	No type-guard narrowing assertion for `isToolBuilder` (optional)
Non-issue	Deep import pattern is consistent with existing tests in this package

The name-inference gap is the substantive one — worth fixing before this lands to ensure the coverage goal is actually met.

coderabbitai bot reviewed Feb 24, 2026

View reviewed changes

Uh oh!

Conversation

gabrypavanello commented Feb 24, 2026

What

Why

Checklist

Uh oh!

coderabbitai bot commented Feb 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Walkthrough

Changes

Estimated code review effort

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

claude bot commented Feb 24, 2026

Uh oh!

claude bot commented Feb 24, 2026

Uh oh!

claude bot commented Feb 24, 2026

Uh oh!

claude bot commented Feb 24, 2026

Uh oh!

claude bot commented Feb 24, 2026

Code Review: PR #182 — test(core): add unit tests for isToolBuilder and ensureBuilt

Bug / Coverage Gap: ensureBuilt name-inference path is not exercised

Uh oh!

claude bot commented Feb 24, 2026

Uh oh!

claude bot commented Feb 24, 2026

CLAUDE.md

Uh oh!

claude bot commented Feb 24, 2026

@context (Mission)

@stack (Technical DNA)

@knowledge Graph (Context Links)

@Map (File Structure)

@workflow (How To Work Here)

@rules (Dos & Don’ts)

@memory (Self-Correction Loop)

Uh oh!

claude bot commented Feb 24, 2026

CLAUDE.md

Uh oh!

claude bot commented Feb 24, 2026

Uh oh!

claude bot commented Feb 24, 2026

CLAUDE.md

Uh oh!

claude bot commented Feb 24, 2026

Uh oh!

claude bot commented Feb 24, 2026

Uh oh!

claude bot commented Feb 24, 2026

Summary

Uh oh!

claude bot commented Feb 24, 2026

@context (Mission)

@stack (Technical DNA)

@knowledge Graph (Context Links)

@Map (File Structure)

@workflow (How To Work Here)

@rules (Dos & Don’ts)

@memory (Self-Correction Loop)

Uh oh!

claude bot commented Feb 24, 2026

Uh oh!

claude bot commented Feb 24, 2026

Uh oh!

claude bot commented Feb 24, 2026

Uh oh!

claude bot commented Feb 24, 2026

CLAUDE.md

Uh oh!

claude bot commented Feb 24, 2026

CLAUDE.md

Uh oh!

claude bot commented Feb 24, 2026

coderabbitai bot commented Feb 24, 2026 •

edited

Loading

Code Review: PR #182 — `test(core): add unit tests for isToolBuilder and ensureBuilt`

Bug / Coverage Gap: `ensureBuilt` name-inference path is not exercised

Code Review: PR #182 — `test(core): add unit tests for isToolBuilder and ensureBuilt`

Bug / Coverage Gap: `ensureBuilt` name-inference path is not exercised

Minor: no assertion on `name` in the existing `ensureBuilt` test

Minor: missing type-narrowing assertion for `isToolBuilder`

Not an issue: deep import of `../../src/builder/ensure-built`

Code Review: PR #182 — `test(core): add unit tests for isToolBuilder and ensureBuilt`

Bug / Coverage Gap: `ensureBuilt` name-inference path is not exercised

Minor: no assertion on `name` in the existing `ensureBuilt` test

Minor: missing type-narrowing assertion for `isToolBuilder`

Not an issue: deep import of `../../src/builder/ensure-built`