From 5695021db5812068ae0a053e8129f687f2cbfcc5 Mon Sep 17 00:00:00 2001 From: Amolith Date: Sat, 7 Feb 2026 03:01:00 +0000 Subject: [PATCH] =?UTF-8?q?fix:=20review=20cleanup=20=E2=80=94=20dedup=20c?= =?UTF-8?q?ontainment=20check,=20remove=20dead=20imports,=20fix=20-u=20edg?= =?UTF-8?q?e=20case?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Deduplicate ensureContained() in content.ts: import ensureWorkspacePath from path-utils.ts instead of maintaining an identical private copy - Remove unused DEFAULT_MAX_BYTES/DEFAULT_MAX_LINES imports from git blame, diff, and show tools (truncateHead uses them internally) - Fix -u without value silently setting options["u"] = true: now leaves uri unset so command handlers can validate properly - Extract expandHomePath() utility for system_prompt_path tilde expansion, replacing fragile inline regex that missed bare ~ and empty HOME - Remove unnecessary parseArgs re-export from cli/index.ts (tests import directly from parse-args.ts) - Add test for -u at end of args Co-authored-by: Shelley --- src/agent/tools/git/blame.ts | 2 +- src/agent/tools/git/diff.ts | 2 +- src/agent/tools/git/show.ts | 2 +- src/cli/commands/repo.ts | 4 ++-- src/cli/commands/web.ts | 4 ++-- src/cli/index.ts | 1 - src/cli/parse-args.ts | 9 ++++++--- src/util/path.ts | 16 ++++++++++++++++ src/workspace/content.ts | 15 +++------------ test/cli-parser.test.ts | 12 +++++++++--- 10 files changed, 41 insertions(+), 26 deletions(-) create mode 100644 src/util/path.ts diff --git a/src/agent/tools/git/blame.ts b/src/agent/tools/git/blame.ts index b12605a81cefed72c544386a16df2c9ce53712f9..0ee022e292ebe3e60cf9d9ce095e0135f21a1cf3 100644 --- a/src/agent/tools/git/blame.ts +++ b/src/agent/tools/git/blame.ts @@ -2,7 +2,7 @@ import { Type } from "@sinclair/typebox"; import type { AgentTool } from "@mariozechner/pi-ai"; import simpleGit from "simple-git"; import { ToolInputError } from "../../../util/errors.js"; -import { DEFAULT_MAX_BYTES, DEFAULT_MAX_LINES, formatSize, truncateHead } from "../../../util/truncate.js"; +import { formatSize, truncateHead } from "../../../util/truncate.js"; const BlameSchema = Type.Object({ path: Type.String({ description: "File path relative to repo root" }), diff --git a/src/agent/tools/git/diff.ts b/src/agent/tools/git/diff.ts index 08454d90ee9599ed308465aa11d8f578aa318771..fecc7b402e5bf9bfb7e7810f1db11d922ec2b208 100644 --- a/src/agent/tools/git/diff.ts +++ b/src/agent/tools/git/diff.ts @@ -2,7 +2,7 @@ import { Type } from "@sinclair/typebox"; import type { AgentTool } from "@mariozechner/pi-ai"; import simpleGit from "simple-git"; import { ToolInputError } from "../../../util/errors.js"; -import { DEFAULT_MAX_BYTES, DEFAULT_MAX_LINES, formatSize, truncateHead } from "../../../util/truncate.js"; +import { formatSize, truncateHead } from "../../../util/truncate.js"; const DiffSchema = Type.Object({ ref: Type.Optional(Type.String({ description: "Base ref (optional)" })), diff --git a/src/agent/tools/git/show.ts b/src/agent/tools/git/show.ts index 9c0a5394ea88b6c1cc7f5c61a2b3b9019801ac92..8b5e26d859378b5fae169b6481fc73935e9b7c79 100644 --- a/src/agent/tools/git/show.ts +++ b/src/agent/tools/git/show.ts @@ -2,7 +2,7 @@ import { Type } from "@sinclair/typebox"; import type { AgentTool } from "@mariozechner/pi-ai"; import simpleGit from "simple-git"; import { ToolInputError } from "../../../util/errors.js"; -import { DEFAULT_MAX_BYTES, DEFAULT_MAX_LINES, formatSize, truncateHead } from "../../../util/truncate.js"; +import { formatSize, truncateHead } from "../../../util/truncate.js"; const ShowSchema = Type.Object({ ref: Type.String({ description: "Commit hash or ref" }), diff --git a/src/cli/commands/repo.ts b/src/cli/commands/repo.ts index 7f14cdf977afcabe6a4817cbd028be9f0e1be11f..9cc13ea7ea09d5b708767c5c4a893268c7cf2919 100644 --- a/src/cli/commands/repo.ts +++ b/src/cli/commands/repo.ts @@ -1,4 +1,5 @@ import { readFile } from "node:fs/promises"; +import { expandHomePath } from "../../util/path.js"; import { applyConfigOverrides, loadConfig } from "../../config/loader.js"; import { createWorkspace } from "../../workspace/manager.js"; import { createGrepTool } from "../../agent/tools/grep.js"; @@ -42,8 +43,7 @@ export async function runRepoCommand(options: RepoCommandOptions): Promise let systemPrompt = REPO_SYSTEM_PROMPT; const promptPath = overrides.repo.system_prompt_path; if (promptPath) { - const home = process.env["HOME"] ?? ""; - systemPrompt = await readFile(promptPath.replace(/^~\//, `${home}/`), "utf8"); + systemPrompt = await readFile(expandHomePath(promptPath), "utf8"); } const git = simpleGit(); diff --git a/src/cli/commands/web.ts b/src/cli/commands/web.ts index ea887ab6fa55ee6f1c0a607cee393406f31fc684..0725dc936dfe94e8d91a476016b5cd5faa1dff70 100644 --- a/src/cli/commands/web.ts +++ b/src/cli/commands/web.ts @@ -1,5 +1,6 @@ import { readFile } from "node:fs/promises"; import { basename } from "node:path"; +import { expandHomePath } from "../../util/path.js"; import { applyConfigOverrides, loadConfig } from "../../config/loader.js"; import { createWorkspace } from "../../workspace/manager.js"; import { writeWorkspaceFile } from "../../workspace/content.js"; @@ -53,8 +54,7 @@ export async function runWebCommand(options: WebCommandOptions): Promise { let systemPrompt = WEB_SYSTEM_PROMPT; const promptPath = overrides.web.system_prompt_path; if (promptPath) { - const home = process.env["HOME"] ?? ""; - systemPrompt = await readFile(promptPath.replace(/^~\//, `${home}/`), "utf8"); + systemPrompt = await readFile(expandHomePath(promptPath), "utf8"); } const tools = [ diff --git a/src/cli/index.ts b/src/cli/index.ts index dc69d2687413503ac9a30f848a17f07ee8b3de6c..6cd2c935fd1dd30a564c757f8b4917af8ace4dc2 100755 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -4,7 +4,6 @@ import { runRepoCommand } from "./commands/repo.js"; import { RumiloError } from "../util/errors.js"; import { parseArgs } from "./parse-args.js"; -export { parseArgs }; async function main() { const { command, options, positional } = parseArgs(process.argv); diff --git a/src/cli/parse-args.ts b/src/cli/parse-args.ts index b35bc2d44af0f4f3c83eecf90ecf4413f66f57b1..76eb674276605123caaeb4ec3b9cbec0b67a837b 100644 --- a/src/cli/parse-args.ts +++ b/src/cli/parse-args.ts @@ -34,9 +34,12 @@ export function parseArgs(args: string[]): ParsedArgs { } } else if (arg.startsWith("-")) { const short = arg.slice(1); - if (short === "u" && rest[i + 1] && !rest[i + 1]!.startsWith("-")) { - options["uri"] = rest[i + 1] as string; - i += 1; + if (short === "u") { + if (rest[i + 1] && !rest[i + 1]!.startsWith("-")) { + options["uri"] = rest[i + 1] as string; + i += 1; + } + // else: -u with no value — uri stays unset, command handler validates } else if (short === "f") { options["full"] = true; } else { diff --git a/src/util/path.ts b/src/util/path.ts new file mode 100644 index 0000000000000000000000000000000000000000..861c165d0994485758f6b80cf314e45ee0a025b3 --- /dev/null +++ b/src/util/path.ts @@ -0,0 +1,16 @@ +import { resolve } from "node:path"; + +/** + * Expand a leading ~ in a file path to the user's home directory. + * Use for paths outside the workspace (e.g. system_prompt_path). + * Workspace-sandboxed paths should NOT use this. + */ +export function expandHomePath(filePath: string): string { + const home = process.env["HOME"]; + if (!home) return filePath; + + if (filePath === "~") return home; + if (filePath.startsWith("~/")) return resolve(home, filePath.slice(2)); + + return filePath; +} diff --git a/src/workspace/content.ts b/src/workspace/content.ts index 0af8e87b101c96c4815c1c359fe45329e94a8cd3..ab533a64f1980981361eed4c73ccec616656c0cb 100644 --- a/src/workspace/content.ts +++ b/src/workspace/content.ts @@ -1,6 +1,6 @@ import { mkdir, writeFile } from "node:fs/promises"; -import { dirname, join, resolve, sep } from "node:path"; -import { ToolInputError } from "../util/errors.js"; +import { dirname, join } from "node:path"; +import { ensureWorkspacePath } from "../agent/tools/path-utils.js"; export interface WorkspaceContent { filePath: string; @@ -8,22 +8,13 @@ export interface WorkspaceContent { bytes: number; } -function ensureContained(workspacePath: string, targetPath: string): void { - const resolved = resolve(workspacePath, targetPath); - const root = workspacePath.endsWith(sep) ? workspacePath : `${workspacePath}${sep}`; - - if (resolved !== workspacePath && !resolved.startsWith(root)) { - throw new ToolInputError(`Path escapes workspace: ${targetPath}`); - } -} - export async function writeWorkspaceFile( workspacePath: string, relativePath: string, content: string, ): Promise { const filePath = join(workspacePath, relativePath); - ensureContained(workspacePath, filePath); + ensureWorkspacePath(workspacePath, filePath); await mkdir(dirname(filePath), { recursive: true }); await writeFile(filePath, content, "utf8"); diff --git a/test/cli-parser.test.ts b/test/cli-parser.test.ts index 36b1e64db9d8c1ec722d54b98fab656aec80498d..aaa8209a94eff0bb4f10523cbf6dc99db4997716 100644 --- a/test/cli-parser.test.ts +++ b/test/cli-parser.test.ts @@ -26,7 +26,7 @@ describe("CLI --key=value parsing (issue #6)", () => { describe("CLI -u short flag (issue #7)", () => { test("-u does not swallow a following flag as its value", () => { const result = parseArgs(["node", "script", "web", "-u", "--verbose"]); - expect(result.options["uri"]).not.toBe("--verbose"); + expect(result.options["uri"]).toBeUndefined(); expect(result.options["verbose"]).toBe(true); }); @@ -37,8 +37,14 @@ describe("CLI -u short flag (issue #7)", () => { test("-u swallowing -f as value is prevented", () => { const result = parseArgs(["node", "script", "repo", "-u", "-f"]); - expect(result.options["uri"]).not.toBe("-f"); - // -u becomes boolean-like (true), -f should be parsed as full flag + expect(result.options["uri"]).toBeUndefined(); + // -u with no valid value is a no-op, -f should be parsed as full flag expect(result.options["full"]).toBe(true); }); + + test("-u at end of args leaves uri unset (no stray option)", () => { + const result = parseArgs(["node", "script", "web", "-u"]); + expect(result.options["uri"]).toBeUndefined(); + expect(result.options["u"]).toBeUndefined(); + }); });