diff --git a/extensions/gentle-agents.ts b/extensions/gentle-agents.ts index 870d2bfc0..b527c4243 100644 --- a/extensions/gentle-agents.ts +++ b/extensions/gentle-agents.ts @@ -1558,7 +1558,7 @@ export default function gentleAgents(pi: ExtensionAPI, env: NodeJS.ProcessEnv = if (retiredSddAgent(agent.name)) throw new Error("Retired SDD agents cannot be dispatched."); if (![AGENT_MODE.TASK, AGENT_MODE.BACKGROUND].includes(mode)) throw new Error("Subagent mode must be task or background."); if (isSingleShotMode(ctx.mode) && mode === AGENT_MODE.BACKGROUND) throw new Error(SINGLE_SHOT_BACKGROUND_ERROR); - const scopeDenied = rejectUnscopedBoundedWriterDispatch({ agent: agent.name, task: prompt, context }); + const scopeDenied = rejectUnscopedBoundedWriterDispatch({ agent: agent.name, task: prompt, context, workspace_root: workspaceRoot, repository_root: repositoryRoot }, ctx.sessionManager.getCwd()); if (scopeDenied) throw new Error(scopeDenied.reason); if (signal?.aborted) throw new Error("Subagent launch aborted before authorization."); const registry = registryFor(ctx); diff --git a/lib/bounded-writer-admission.ts b/lib/bounded-writer-admission.ts index a9a070366..d42b8a34f 100644 --- a/lib/bounded-writer-admission.ts +++ b/lib/bounded-writer-admission.ts @@ -21,11 +21,130 @@ function isTaskScopedRepositoryRelativePath(value: string, backticked: boolean): return !/[?*\[\]{}]/.test(path.split("/")[0]); } +export function tryNormalizeInRepoAbsolutePath(candidate: string, targetRoot: string): string | undefined { + const normalizedCandidate = candidate.replace(/\\/g, "/"); + const isWindowsDrive = /^[A-Za-z]:\//.test(normalizedCandidate); + const isPosixAbsolute = normalizedCandidate.startsWith("/"); + if (!isWindowsDrive && !isPosixAbsolute) { + return undefined; + } + + const roots = [targetRoot]; + try { + const realRoot = realpathSync(targetRoot); + if (realRoot !== targetRoot) roots.push(realRoot); + } catch { + // Target root might not exist on disk in synthetic unit tests + } + + for (const root of roots) { + const normRoot = root.replace(/\\/g, "/"); + const rootDriveMatch = normRoot.match(/^([A-Za-z]:)(\/.*)?$/); + const candDriveMatch = normalizedCandidate.match(/^([A-Za-z]:)(\/.*)?$/); + + let candPrefix = ""; + let candPath = normalizedCandidate; + if (candDriveMatch) { + candPrefix = candDriveMatch[1]!.toLowerCase(); + candPath = candDriveMatch[2] ?? "/"; + } else if (isPosixAbsolute) { + candPrefix = ""; + candPath = normalizedCandidate; + } else { + continue; + } + + let rootPrefix = ""; + let rootPath = normRoot; + if (rootDriveMatch) { + rootPrefix = rootDriveMatch[1]!.toLowerCase(); + rootPath = rootDriveMatch[2] ?? "/"; + } else if (normRoot.startsWith("/")) { + rootPrefix = ""; + rootPath = normRoot; + } else { + const resolvedRoot = resolve(normRoot).replace(/\\/g, "/"); + const resDrive = resolvedRoot.match(/^([A-Za-z]:)(\/.*)?$/); + if (resDrive) { + rootPrefix = resDrive[1]!.toLowerCase(); + rootPath = resDrive[2] ?? "/"; + } else { + rootPrefix = ""; + rootPath = resolvedRoot; + } + } + + if (candPrefix !== rootPrefix) continue; + + const candSegments = candPath.split("/").filter((s) => s.length > 0 && s !== "."); + const collapsedCand: string[] = []; + for (const seg of candSegments) { + if (seg === "..") { + collapsedCand.pop(); + } else { + collapsedCand.push(seg); + } + } + const collapsedCandPath = "/" + collapsedCand.join("/"); + + const rootSegments = rootPath.split("/").filter((s) => s.length > 0 && s !== "."); + if (rootSegments.length === 0) continue; + + const collapsedRoot: string[] = []; + for (const seg of rootSegments) { + if (seg === "..") { + collapsedRoot.pop(); + } else { + collapsedRoot.push(seg); + } + } + if (collapsedRoot.length === 0) continue; + const collapsedRootPath = "/" + collapsedRoot.join("/"); + + const prefixWithSlash = collapsedRootPath === "/" ? "/" : collapsedRootPath + "/"; + const isMatch = rootDriveMatch + ? collapsedCandPath.toLowerCase().startsWith(prefixWithSlash.toLowerCase()) + : collapsedCandPath.startsWith(prefixWithSlash); + + if (isMatch) { + const rel = collapsedCandPath.slice(prefixWithSlash.length); + if (rel.length > 0 && rel !== "." && !rel.startsWith("/")) { + try { + const realCandidate = realpathSync(candidate).replace(/\\/g, "/"); + const realRoots = roots.map((r) => { + try { return realpathSync(r).replace(/\\/g, "/"); } catch { return r.replace(/\\/g, "/"); } + }); + const escapes = !realRoots.some((r) => { + const rWithSlash = r.endsWith("/") ? r : r + "/"; + return realCandidate.startsWith(rWithSlash) || realCandidate.toLowerCase().startsWith(rWithSlash.toLowerCase()); + }); + if (escapes) return undefined; + } catch { + // File does not exist yet (normal for planned edits) + } + return rel; + } + } + } + + return undefined; +} + +export function resolveTargetRepositoryRoot(input: Record, fallbackRoot?: string): string { + if (typeof input.repository_root === "string" && input.repository_root.trim().length > 0) { + return input.repository_root.trim(); + } + if (typeof input.workspace_root === "string" && input.workspace_root.trim().length > 0) { + return input.workspace_root.trim(); + } + return fallbackRoot ?? process.cwd(); +} + // This is the same canonical parser used by both the tool hook and executor. // Prose cannot close the section, and repeated sections must agree exactly. // A rejection carries the concrete problem so the caller repairs only the // section instead of re-summarizing the whole task (gentle-shell#1713). -function parseAllowedEditSurfaces(values: readonly unknown[]): { paths: string[] } | { problem: string } { +function parseAllowedEditSurfaces(values: readonly unknown[], targetRoot?: string): { paths: string[] } | { problem: string } { let expected: string[] | undefined; for (const value of values) { if (typeof value !== "string") continue; @@ -49,11 +168,17 @@ function parseAllowedEditSurfaces(values: readonly unknown[]): { paths: string[] const quotable = prose && !quoted && !unlisted.includes("`") && !/\p{Cc}|\p{Zl}|\p{Zp}/u.test(source) && isTaskScopedRepositoryRelativePath(unlisted, true); const shown = (prose ? source.trim() : path).replace(/\p{Cc}|\p{Zl}|\p{Zp}/gu, " "); const bounded = shown.length > 120 ? `${shown.slice(0, 120)}...` : shown; - return { problem: quotable - ? `Line "${bounded}" is not a valid surface entry; if it is a path, wrap the whole entry in backticks, otherwise move it under a following Markdown heading.` - : prose - ? `Line "${bounded}" is not a valid surface entry; move prose under a following Markdown heading.` - : `Entry "${bounded}" is not a narrow repository-relative path; remove absolute paths, \`..\` segments, root globs, and stray backticks.` }; + if (quotable) { + return { problem: `Line "${bounded}" is not a valid surface entry; if it is a path, wrap the whole entry in backticks, otherwise move it under a following Markdown heading.` }; + } + if (prose) { + return { problem: `Line "${bounded}" is not a valid surface entry; move prose under a following Markdown heading.` }; + } + const suggestedRelative = targetRoot ? tryNormalizeInRepoAbsolutePath(path, targetRoot) : undefined; + if (suggestedRelative && isTaskScopedRepositoryRelativePath(suggestedRelative, !!quoted)) { + return { problem: `Entry "${bounded}" is not a narrow repository-relative path (use "${suggestedRelative}" instead); remove absolute paths, \`..\` segments, root globs, and stray backticks.` }; + } + return { problem: `Entry "${bounded}" is not a narrow repository-relative path; remove absolute paths, \`..\` segments, root globs, and stray backticks.` }; } paths.push(path); } @@ -70,11 +195,12 @@ export function allowedEditSurfaces(...values: unknown[]): string[] | undefined return "paths" in parsed ? parsed.paths : undefined; } -export function rejectUnscopedBoundedWriterDispatch(input: unknown): { block: true; reason: string } | undefined { +export function rejectUnscopedBoundedWriterDispatch(input: unknown, defaultRoot?: string): { block: true; reason: string } | undefined { if (!input || typeof input !== "object" || Array.isArray(input)) return undefined; const record = input as Record; if (typeof record.agent !== "string" || !WRITER_NAMES.includes(record.agent)) return undefined; - const parsed = parseAllowedEditSurfaces([record.task, record.context]); + const targetRoot = resolveTargetRepositoryRoot(record, defaultRoot); + const parsed = parseAllowedEditSurfaces([record.task, record.context], targetRoot); if ("paths" in parsed) return undefined; return { block: true, reason: `${WRITER_EDIT_SURFACE_REJECTION} ${parsed.problem} Resend the same task text unchanged except for that section; never shorten or re-summarize it.` }; } diff --git a/tests/writer-edit-surface-scope.test.ts b/tests/writer-edit-surface-scope.test.ts index 74925b621..3e602c956 100644 --- a/tests/writer-edit-surface-scope.test.ts +++ b/tests/writer-edit-surface-scope.test.ts @@ -66,7 +66,7 @@ async function assertAccepted(input: Record, message: string) { } const RESEND = "Resend the same task text unchanged except for that section; never shorten or re-summarize it."; -const PROBLEM = /^(?:No `## Allowed edit surfaces` heading was found\.|The section has no entries\.|Repeated sections list different surfaces\.|Line ".+" is not a valid surface entry; move prose under a following Markdown heading\.|Line ".+" is not a valid surface entry; if it is a path, wrap the whole entry in backticks, otherwise move it under a following Markdown heading\.|Entry ".+" is not a narrow repository-relative path; remove absolute paths, `\.\.` segments, root globs, and stray backticks\.)$/; +const PROBLEM = /^(?:No `## Allowed edit surfaces` heading was found\.|The section has no entries\.|Repeated sections list different surfaces\.|Line ".+" is not a valid surface entry; move prose under a following Markdown heading\.|Line ".+" is not a valid surface entry; if it is a path, wrap the whole entry in backticks, otherwise move it under a following Markdown heading\.|Entry ".+" is not a narrow repository-relative path(?: \(use ".+" instead\))?; remove absolute paths, `\.\.` segments, root globs, and stray backticks\.)$/; // Every rejection is the canonical text, exactly one concrete problem, and the // resend instruction; nothing else (review R3-005). @@ -421,3 +421,88 @@ test("agents outside the bounded writer set are not scope-guarded", async () => task: "Map the decoder call sites.", }, "a read-only explorer needs no edit surfaces"); }); + +test("in-repository absolute paths are rejected with actionable suggestion and task is never mutated", async () => { + const repoRoot = "/workspace/my-project"; + const originalTask = [ + "Update the status parser.", + "", + "## Allowed edit surfaces", + `- \`${repoRoot}/lib/sdd-status.ts\``, + `- ${repoRoot}/tests/sdd-status.test.ts`, + "", + "### Validation", + "npm test", + ].join("\n"); + const input: Record = { + agent: "gentle-ai-worker", + mode: "task", + workspace_root: repoRoot, + task: originalTask, + }; + + const result = await dispatchWriter(input); + assert.equal(result?.block, true, "in-repository absolute paths under workspace_root are rejected"); + assert.ok( + result?.reason.includes('(use "lib/sdd-status.ts" instead)'), + `rejection reason must include exact actionable hint: ${result?.reason}`, + ); + assert.equal(input.task, originalTask, "input.task remains completely unmutated"); + + const winRoot = "C:\\Users\\Dev\\project"; + const winOriginalTask = [ + "## Allowed edit surfaces", + `- \`${winRoot}\\src\\index.ts\``, + `- ${winRoot}\\tests\\index.test.ts`, + ].join("\n"); + const winInput: Record = { + agent: "gentle-ai-worker", + mode: "task", + repository_root: winRoot, + task: winOriginalTask, + }; + const winResult = await dispatchWriter(winInput); + assert.equal(winResult?.block, true, "in-repository Windows drive paths under repository_root are rejected"); + assert.ok( + winResult?.reason.includes('(use "src/index.ts" instead)'), + `Windows path rejection must include exact actionable hint: ${winResult?.reason}`, + ); + assert.equal(winInput.task, winOriginalTask, "winInput.task remains completely unmutated"); + + const outsidePosixResult = await dispatchWriter({ + agent: "gentle-ai-worker", + mode: "task", + workspace_root: repoRoot, + task: ["## Allowed edit surfaces", "- `/etc/passwd`"].join("\n"), + }); + assert.equal(outsidePosixResult?.block, true); + assert.ok( + !outsidePosixResult?.reason.includes("(use "), + `out-of-repository POSIX absolute path is rejected without false suggestion: ${outsidePosixResult?.reason}`, + ); + + const outsideWinResult = await dispatchWriter({ + agent: "gentle-ai-worker", + mode: "task", + repository_root: winRoot, + task: ["## Allowed edit surfaces", `- \`C:\\outside.ts\``].join("\n"), + }); + assert.equal(outsideWinResult?.block, true); + assert.ok( + !outsideWinResult?.reason.includes("(use "), + `out-of-repository Windows path is rejected without false suggestion: ${outsideWinResult?.reason}`, + ); + + const rootTargetResult = await dispatchWriter({ + agent: "gentle-ai-worker", + mode: "task", + workspace_root: repoRoot, + task: ["## Allowed edit surfaces", `- \`${repoRoot}\``].join("\n"), + }); + assert.equal(rootTargetResult?.block, true); + assert.ok( + !rootTargetResult?.reason.includes("(use "), + `targeting the repository root itself is rejected without false suggestion: ${rootTargetResult?.reason}`, + ); +}); +