diff --git a/src/orchestrator/agent-loader.ts b/src/orchestrator/agent-loader.ts index 96130b7..147c185 100644 --- a/src/orchestrator/agent-loader.ts +++ b/src/orchestrator/agent-loader.ts @@ -210,7 +210,7 @@ export async function assembleSystemPrompt( const env = await detectEnvironment(projectRoot); const toolNames = ROLE_TOOLS[role as keyof typeof ROLE_TOOLS] ?? []; - return `${decorated}\n\n${formatEnvironmentContext(env, [...toolNames])}`; + return `${decorated}\n\n${formatEnvironmentContext(env, [...toolNames], projectRoot)}`; } catch { // Environment detection is best-effort — never block agent spawn. return decorated; diff --git a/src/orchestrator/environment.test.ts b/src/orchestrator/environment.test.ts new file mode 100644 index 0000000..ce2435e --- /dev/null +++ b/src/orchestrator/environment.test.ts @@ -0,0 +1,53 @@ +/** + * Unit tests for formatEnvironmentContext — the markdown block injected into + * agent system prompts. Focus on the project-root + relative-path guidance + * added to stop non-Claude models inventing absolute paths. + */ + +import { describe, it, expect } from "vitest"; + +import { formatEnvironmentContext, type HostEnvironment } from "./environment.js"; + +const ENV: HostEnvironment = { + platform: "darwin", + osName: "macOS", + osRelease: "25.2.0", + arch: "arm64", + shell: "/bin/zsh", + nodeVersion: "v22.14.0", + packageManager: "npm", + installedTools: ["git", "node", "npm"], +}; + +const ROOT = "/Users/bober4ik/agent-bober-workspace/agent-bober-ide"; + +describe("formatEnvironmentContext", () => { + it("surfaces the absolute project root when provided", () => { + const out = formatEnvironmentContext(ENV, ["read_file", "glob"], ROOT); + expect(out).toContain(`- Project root (absolute): ${ROOT}`); + }); + + it("omits the project-root line when not provided", () => { + const out = formatEnvironmentContext(ENV, ["read_file", "glob"]); + expect(out).not.toContain("Project root (absolute)"); + }); + + it("adds relative-path guidance when a path-bearing tool is present", () => { + const out = formatEnvironmentContext(ENV, ["glob", "grep"], ROOT); + expect(out).toContain("pass paths RELATIVE to the project root"); + expect(out).toContain("Do NOT construct absolute"); + }); + + it("omits relative-path guidance when no path tool is available", () => { + const out = formatEnvironmentContext(ENV, ["report"], ROOT); + expect(out).not.toContain("pass paths RELATIVE to the project root"); + }); + + it("still lists the exact tools and host OS", () => { + const out = formatEnvironmentContext(ENV, ["read_file", "glob"], ROOT); + expect(out).toContain("# Host Environment"); + expect(out).toContain("- OS: macOS (darwin 25.2.0, arm64)"); + expect(out).toContain("# Your Tools"); + expect(out).toContain("You have EXACTLY these tools: read_file, glob."); + }); +}); diff --git a/src/orchestrator/environment.ts b/src/orchestrator/environment.ts index afcfa72..a3f3ede 100644 --- a/src/orchestrator/environment.ts +++ b/src/orchestrator/environment.ts @@ -139,14 +139,18 @@ export function resetEnvironmentCache(): void { * Render the environment + the role's exact harness tools as a markdown block * for injection into an agent's system prompt. * - * @param env Detected host environment. - * @param toolNames The exact harness tool names available to this role (e.g. - * ["read_file","glob","grep"]). Used to stop models inventing - * tools (e.g. a non-existent "bash" for read-only roles). + * @param env Detected host environment. + * @param toolNames The exact harness tool names available to this role (e.g. + * ["read_file","glob","grep"]). Used to stop models inventing + * tools (e.g. a non-existent "bash" for read-only roles). + * @param projectRoot Absolute path to the project root. Surfaced so models stop + * guessing it — non-Claude models otherwise invent an absolute + * path with the wrong home dir, which the path sandbox rejects. */ export function formatEnvironmentContext( env: HostEnvironment, toolNames: string[], + projectRoot?: string, ): string { const lines: string[] = []; lines.push("# Host Environment"); @@ -157,6 +161,7 @@ export function formatEnvironmentContext( lines.push(`- Node: ${env.nodeVersion}`); if (env.packageManager) lines.push(`- Package manager: ${env.packageManager}`); + if (projectRoot) lines.push(`- Project root (absolute): ${projectRoot}`); if (env.installedTools.length > 0) { lines.push(`- Installed CLIs on PATH: ${env.installedTools.join(", ")}`); } @@ -180,5 +185,18 @@ export function formatEnvironmentContext( `state that in your output rather than inventing a tool.`, ); + const hasPathTool = toolNames.some((t) => + ["read_file", "write_file", "edit_file", "glob", "grep"].includes(t), + ); + if (hasPathTool) { + lines.push( + "", + "For file/path tool arguments, pass paths RELATIVE to the project root " + + "(e.g. `src`, `src/index.ts`, `src/**/*.ts`). Do NOT construct absolute " + + "paths — you do not know the real home directory and a wrong guess is " + + "rejected by the path sandbox.", + ); + } + return lines.join("\n"); } diff --git a/src/orchestrator/tools/handlers.test.ts b/src/orchestrator/tools/handlers.test.ts new file mode 100644 index 0000000..eb4327a --- /dev/null +++ b/src/orchestrator/tools/handlers.test.ts @@ -0,0 +1,76 @@ +/** + * Unit tests for sandboxPath — the path-sandboxing guard shared by the + * read_file / write_file / edit_file / glob / grep tool handlers. + * + * Focus: the re-anchoring recovery for absolute paths invented with the wrong + * home directory (the DeepSeek `/Users/boberik/...` failure mode), without + * widening the sandbox for genuinely-foreign paths. + */ + +import { describe, it, expect } from "vitest"; +import { resolve } from "node:path"; + +import { sandboxPath } from "./handlers.js"; + +const ROOT = "/Users/bober4ik/agent-bober-workspace/agent-bober-ide"; + +describe("sandboxPath", () => { + it("accepts a relative path and resolves it under the root", () => { + expect(sandboxPath(ROOT, "src")).toBe(resolve(ROOT, "src")); + expect(sandboxPath(ROOT, "src/index.ts")).toBe( + resolve(ROOT, "src/index.ts"), + ); + }); + + it("accepts the root itself (empty relative path)", () => { + expect(sandboxPath(ROOT, ".")).toBe(resolve(ROOT)); + expect(sandboxPath(ROOT, ROOT)).toBe(resolve(ROOT)); + }); + + it("accepts a correct absolute path inside the root", () => { + const abs = `${ROOT}/src/app.ts`; + expect(sandboxPath(ROOT, abs)).toBe(resolve(abs)); + }); + + it("re-anchors an absolute path invented with the wrong home dir", () => { + // DeepSeek hallucinated `boberik` instead of the real `bober4ik`. + const bogus = + "/Users/boberik/agent-bober-workspace/agent-bober-ide/src"; + expect(sandboxPath(ROOT, bogus)).toBe(resolve(ROOT, "src")); + }); + + it("re-anchors a deep wrong-home absolute path to the right suffix", () => { + const bogus = + "/home/ci/agent-bober-ide/src/components/Button.tsx"; + expect(sandboxPath(ROOT, bogus)).toBe( + resolve(ROOT, "src/components/Button.tsx"), + ); + }); + + it("re-anchors a wrong-home path that points at the root itself", () => { + const bogus = "/Users/boberik/agent-bober-workspace/agent-bober-ide"; + expect(sandboxPath(ROOT, bogus)).toBe(resolve(ROOT)); + }); + + it("still blocks a relative traversal escaping the root", () => { + expect(() => sandboxPath(ROOT, "../../etc/passwd")).toThrow( + /outside the project root/, + ); + }); + + it("still blocks a genuinely-foreign absolute path (no root basename)", () => { + expect(() => sandboxPath(ROOT, "/etc/passwd")).toThrow( + /outside the project root/, + ); + }); + + it("re-anchoring never widens the sandbox: suffix stays inside root", () => { + // Even a malicious suffix after the root name cannot escape, because the + // re-anchored path is re-validated against the root. + const sneaky = + "/Users/attacker/agent-bober-ide/../../../../etc/passwd"; + expect(() => sandboxPath(ROOT, sneaky)).toThrow( + /outside the project root/, + ); + }); +}); diff --git a/src/orchestrator/tools/handlers.ts b/src/orchestrator/tools/handlers.ts index 93ca67b..9da6134 100644 --- a/src/orchestrator/tools/handlers.ts +++ b/src/orchestrator/tools/handlers.ts @@ -1,5 +1,5 @@ import { readFile, writeFile, mkdir } from "node:fs/promises"; -import { resolve, relative, isAbsolute } from "node:path"; +import { resolve, relative, isAbsolute, basename, sep } from "node:path"; import { execa } from "execa"; // ── Constants ────────────────────────────────────────────────────── @@ -27,14 +27,41 @@ export type ToolHandler = ( /** * Resolve and validate a file path, ensuring it stays within projectRoot. * Returns the absolute path or throws if the path escapes the sandbox. + * + * Re-anchoring recovery: non-Claude models (e.g. DeepSeek) sometimes ignore the + * "relative to project root" contract and invent an ABSOLUTE path with the wrong + * home dir — e.g. `/Users/boberik/.../agent-bober-ide/src` when the real root is + * `/Users/bober4ik/.../agent-bober-ide`. When such a path lands outside the root + * but still contains the root's basename, we re-anchor the suffix after it + * (`src`) relative to the real root and retry. This NEVER widens the sandbox: the + * re-anchored path is `resolve(projectRoot, suffix)` and is re-validated to be + * inside the root before use, so a genuinely-foreign path still fails closed. */ -function sandboxPath(projectRoot: string, inputPath: string): string { - const abs = isAbsolute(inputPath) +export function sandboxPath(projectRoot: string, inputPath: string): string { + let abs = isAbsolute(inputPath) ? resolve(inputPath) : resolve(projectRoot, inputPath); + let rel = relative(projectRoot, abs); + + if ((rel.startsWith("..") || isAbsolute(rel)) && isAbsolute(inputPath)) { + const rootName = basename(projectRoot); + if (rootName) { + const segments = abs.split(sep); + const idx = segments.lastIndexOf(rootName); + if (idx !== -1) { + const suffix = segments.slice(idx + 1).join(sep); + const reAnchored = resolve(projectRoot, suffix); + const reRel = relative(projectRoot, reAnchored); + if (!reRel.startsWith("..") && !isAbsolute(reRel)) { + abs = reAnchored; + rel = reRel; + } + } + } + } + // Ensure the resolved path is within the project root - const rel = relative(projectRoot, abs); if (rel.startsWith("..") || isAbsolute(rel)) { throw new Error( `Path "${inputPath}" resolves outside the project root. Access denied.`, diff --git a/src/orchestrator/tools/schemas.ts b/src/orchestrator/tools/schemas.ts index bc808ea..1dfe6c0 100644 --- a/src/orchestrator/tools/schemas.ts +++ b/src/orchestrator/tools/schemas.ts @@ -37,7 +37,7 @@ export const readFileTool: ToolDef = { file_path: { type: "string", description: - "Path to the file, relative to the project root or absolute.", + "Path to the file, relative to the project root (e.g. `src/index.ts`). Pass a relative path, not an absolute one.", }, offset: { type: "number", @@ -62,7 +62,7 @@ export const writeFileTool: ToolDef = { file_path: { type: "string", description: - "Path to the file, relative to the project root or absolute.", + "Path to the file, relative to the project root (e.g. `src/index.ts`). Pass a relative path, not an absolute one.", }, content: { type: "string", @@ -83,7 +83,7 @@ export const editFileTool: ToolDef = { file_path: { type: "string", description: - "Path to the file, relative to the project root or absolute.", + "Path to the file, relative to the project root (e.g. `src/index.ts`). Pass a relative path, not an absolute one.", }, old_text: { type: "string", @@ -113,7 +113,7 @@ export const globTool: ToolDef = { path: { type: "string", description: - "Directory to search in, relative to project root. Defaults to project root.", + "Directory to search in, relative to project root (e.g. `src`). Pass a relative path, not an absolute one. Defaults to project root.", }, }, required: ["pattern"], @@ -134,7 +134,7 @@ export const grepTool: ToolDef = { path: { type: "string", description: - "File or directory to search in, relative to project root. Defaults to project root.", + "File or directory to search in, relative to project root (e.g. `src`). Pass a relative path, not an absolute one. Defaults to project root.", }, glob: { type: "string",