fix: review cleanup — dedup containment check, remove dead imports, fix -u edge case

Amolith and Shelley created 1 month ago

- 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 <[e-mail]>

Change summary

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(-)

Detailed changes

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" }),

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)" })),

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" }),

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<void>
     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();

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<void> {
     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 = [

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);

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 {

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;
+}

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<WorkspaceContent> {
   const filePath = join(workspacePath, relativePath);
-  ensureContained(workspacePath, filePath);
+  ensureWorkspacePath(workspacePath, filePath);
   await mkdir(dirname(filePath), { recursive: true });
   await writeFile(filePath, content, "utf8");
 

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();
+  });
 });