From 9cb8e3301beb3a260286b201cbe616f05ed72ab8 Mon Sep 17 00:00:00 2001 From: Amolith Date: Sat, 7 Feb 2026 04:11:16 +0000 Subject: [PATCH] =?UTF-8?q?fix:=20review=20round=202=20=E2=80=94=20credent?= =?UTF-8?q?ial=20error=20types,=20test=20gaps,=20git=5Frefs=20truncation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Use ConfigError (not ToolInputError) for missing Kagi/Tabstack credentials in web command — these are config/env errors - Fix web_search test that silently passed when no error was thrown - Truncate git_refs output consistent with other git tools - Add expandHomePath unit tests - Add "test" script to package.json - Update AGENTS.md to reflect test suite existence - Minor import formatting cleanup in cli/index.ts Co-authored-by: Shelley --- AGENTS.md | 2 +- package.json | 1 + src/agent/tools/git/refs.ts | 29 ++++++++++++---------- src/cli/commands/web.ts | 6 ++--- src/cli/index.ts | 1 - test/expand-home-path.test.ts | 44 ++++++++++++++++++++++++++++++++++ test/web-search.test.ts | 19 ++++++++------- test/workspace-cleanup.test.ts | 4 ++-- 8 files changed, 77 insertions(+), 29 deletions(-) create mode 100644 test/expand-home-path.test.ts diff --git a/AGENTS.md b/AGENTS.md index 921d5e41097a64de6fa0952d4221057275e7f677..d4f92971bb3eff6ba8ca968f017a9bc403aee059 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -10,7 +10,7 @@ bun run build # Build to dist/ bun run typecheck # TypeScript check (also: bun run lint) ``` -No test suite currently exists (`test/` is empty). +Run `bun test` to execute the test suite. ## Architecture diff --git a/package.json b/package.json index 1bc72bb965a0910193bc9dc59cf1aa88556b9b20..0509c84c2c4d91caadf8a414fd0092619bf0aebb 100644 --- a/package.json +++ b/package.json @@ -11,6 +11,7 @@ "build": "bun build src/cli/index.ts --outdir dist --target=node", "start": "bun dist/cli/index.js", "lint": "bun run --silent typecheck", + "test": "bun test", "typecheck": "tsc --noEmit" }, "dependencies": { diff --git a/src/agent/tools/git/refs.ts b/src/agent/tools/git/refs.ts index 2c87551a4e1fe2f1e7e56a6f5b21b0382e8ccbb1..d8e46b29c0ad2c6f3397a96fd21d329bb272da55 100644 --- a/src/agent/tools/git/refs.ts +++ b/src/agent/tools/git/refs.ts @@ -1,6 +1,7 @@ import { Type } from "@sinclair/typebox"; import type { AgentTool } from "@mariozechner/pi-ai"; import simpleGit from "simple-git"; +import { formatSize, truncateHead } from "../../../util/truncate.js"; const RefsSchema = Type.Object({ type: Type.Union([ @@ -18,26 +19,28 @@ export const createGitRefsTool = (workspacePath: string): AgentTool => ({ execute: async (_toolCallId: string, params: any) => { const git = simpleGit(workspacePath); + let raw: string; + let baseDetails: Record = {}; + if (params.type === "tags") { const tags = await git.tags(); - return { - content: [{ type: "text", text: tags.all.join("\n") }], - details: { count: tags.all.length }, - }; + raw = tags.all.join("\n"); + baseDetails = { count: tags.all.length }; + } else if (params.type === "remotes") { + raw = await git.raw(["branch", "-r"]); + } else { + raw = await git.raw(["branch", "-a"]); } - if (params.type === "remotes") { - const raw = await git.raw(["branch", "-r"]); - return { - content: [{ type: "text", text: raw }], - details: {}, - }; + const truncation = truncateHead(raw); + let text = truncation.content; + if (truncation.truncated) { + text += `\n\n[truncated: showing ${truncation.outputLines} of ${truncation.totalLines} lines (${formatSize(truncation.outputBytes)} of ${formatSize(truncation.totalBytes)})]`; } - const raw = await git.raw(["branch", "-a"]); return { - content: [{ type: "text", text: raw }], - details: {}, + content: [{ type: "text", text }], + details: { ...baseDetails, ...(truncation.truncated ? { truncation } : {}) }, }; }, }); diff --git a/src/cli/commands/web.ts b/src/cli/commands/web.ts index 0725dc936dfe94e8d91a476016b5cd5faa1dff70..ed0b60fd6738a64e52fd4ae31f2ff2f1043ef5f7 100644 --- a/src/cli/commands/web.ts +++ b/src/cli/commands/web.ts @@ -13,7 +13,7 @@ import { createWebSearchTool } from "../../agent/tools/web-search.js"; import { runAgent } from "../../agent/runner.js"; import { WEB_SYSTEM_PROMPT } from "../../agent/prompts/web.js"; import { createEventLogger, printUsageSummary } from "../output.js"; -import { FetchError, ToolInputError } from "../../util/errors.js"; +import { ConfigError, FetchError } from "../../util/errors.js"; const INJECT_THRESHOLD = 50 * 1024; @@ -45,10 +45,10 @@ export async function runWebCommand(options: WebCommandOptions): Promise { process.env["TABSTACK_API_KEY"]; if (!kagiSession) { - throw new ToolInputError("Missing Kagi session token (set KAGI_SESSION_TOKEN or config)"); + throw new ConfigError("Missing Kagi session token (set KAGI_SESSION_TOKEN or config)"); } if (!tabstackKey) { - throw new ToolInputError("Missing Tabstack API key (set TABSTACK_API_KEY or config)"); + throw new ConfigError("Missing Tabstack API key (set TABSTACK_API_KEY or config)"); } let systemPrompt = WEB_SYSTEM_PROMPT; diff --git a/src/cli/index.ts b/src/cli/index.ts index 6cd2c935fd1dd30a564c757f8b4917af8ace4dc2..378f0a2d722aa8a3ef7cd6bd096ce48e584f4b01 100755 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -2,7 +2,6 @@ import { runWebCommand } from "./commands/web.js"; import { runRepoCommand } from "./commands/repo.js"; import { RumiloError } from "../util/errors.js"; - import { parseArgs } from "./parse-args.js"; async function main() { diff --git a/test/expand-home-path.test.ts b/test/expand-home-path.test.ts new file mode 100644 index 0000000000000000000000000000000000000000..4de54fb170446347ac756535a707d6651929e2ed --- /dev/null +++ b/test/expand-home-path.test.ts @@ -0,0 +1,44 @@ +import { describe, test, expect, beforeEach, afterEach } from "bun:test"; +import { expandHomePath } from "../src/util/path.js"; + +describe("expandHomePath", () => { + let savedHome: string | undefined; + + beforeEach(() => { + savedHome = process.env["HOME"]; + }); + + afterEach(() => { + if (savedHome === undefined) { + delete process.env["HOME"]; + } else { + process.env["HOME"] = savedHome; + } + }); + + test("returns path unchanged when HOME is unset", () => { + delete process.env["HOME"]; + expect(expandHomePath("~/foo/bar")).toBe("~/foo/bar"); + }); + + test("bare ~ returns HOME", () => { + process.env["HOME"] = "/Users/alice"; + expect(expandHomePath("~")).toBe("/Users/alice"); + }); + + test("~/foo/bar expands to $HOME/foo/bar", () => { + process.env["HOME"] = "/Users/alice"; + expect(expandHomePath("~/foo/bar")).toBe("/Users/alice/foo/bar"); + }); + + test("paths without tilde are returned unchanged", () => { + process.env["HOME"] = "/Users/alice"; + expect(expandHomePath("/absolute/path")).toBe("/absolute/path"); + expect(expandHomePath("relative/path")).toBe("relative/path"); + }); + + test("~user/foo is returned unchanged (not our expansion)", () => { + process.env["HOME"] = "/Users/alice"; + expect(expandHomePath("~user/foo")).toBe("~user/foo"); + }); +}); diff --git a/test/web-search.test.ts b/test/web-search.test.ts index 814ee714ea97f0485e548638788258727bdaece8..2451f844622c6b80f05b9ed99044102682531d69 100644 --- a/test/web-search.test.ts +++ b/test/web-search.test.ts @@ -1,5 +1,13 @@ -import { describe, test, expect } from "bun:test"; +import { describe, test, expect, mock } from "bun:test"; import { FetchError, ToolInputError } from "../src/util/errors.js"; + +// Mock kagi-ken so the search function throws, exercising the FetchError wrapping +mock.module("kagi-ken", () => ({ + search: async () => { + throw new Error("Unauthorized"); + }, +})); + import { createWebSearchTool } from "../src/agent/tools/web-search.js"; describe("web_search error handling (issue #11)", () => { @@ -9,14 +17,7 @@ describe("web_search error handling (issue #11)", () => { }); test("search API failure is wrapped as FetchError", async () => { - // Use a bogus token so the kagi API call fails const tool = createWebSearchTool("invalid-token-xxx"); - try { - await tool.execute("id", { query: "test query" }); - // If it somehow succeeds (unlikely), that's fine - } catch (e: any) { - // After fix, errors from the search API should be wrapped as FetchError - expect(e).toBeInstanceOf(FetchError); - } + await expect(tool.execute("id", { query: "test query" })).rejects.toThrow(FetchError); }); }); diff --git a/test/workspace-cleanup.test.ts b/test/workspace-cleanup.test.ts index 0e9c4c6c7616f983c77c912cb1b0cfcf6c5c12cd..954c0f70f144224c44d3755ab09574a83a5738da 100644 --- a/test/workspace-cleanup.test.ts +++ b/test/workspace-cleanup.test.ts @@ -4,7 +4,7 @@ import { rm } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { execSync } from "node:child_process"; -import { ToolInputError } from "../src/util/errors.js"; +import { ConfigError } from "../src/util/errors.js"; /** * Snapshot rumilo-* dirs in tmpdir so we can detect leaks. @@ -50,7 +50,7 @@ describe("web command – workspace cleanup on early failure", () => { cleanup: true, }); } catch (e: any) { - expect(e).toBeInstanceOf(ToolInputError); + expect(e).toBeInstanceOf(ConfigError); } const after = rumiloTmpDirs();