Repository navigation
fix(writers): suggest in-repo relative path on absolute edit surface rejection (#2077) #2081
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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<string, unknown>, 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(); | ||
| } | ||
|
Comment on lines
+24
to
+141
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: sed -n '1,220p' lib/bounded-writer-admission.ts
sed -n '420,515p' tests/writer-edit-surface-scope.test.ts
rg -n 'tryNormalizeInRepoAbsolutePath|resolveTargetRepositoryRoot|repository_root|workspace_root|symlink|repo-other' tests/writer-edit-surface-scope.test.ts tests/bounded-writer-admission.test.tsRepository: Gentleman-Programming/gentle-shell Length of output: 15319 🏁 Script executed: set -o pipefail
printf '%s\n' '--- relevant test file outline/content ---'
sed -n '1,130p' tests/bounded-writer-admission.test.ts
sed -n '380,525p' tests/writer-edit-surface-scope.test.ts
printf '%s\n' '--- changed-file diff against merge base ---'
git diff --unified=80 43541a0d30dfafacfcb2e99ef4135ab8b1693e99 46198c380bcdfdb2fead7852a3dddafd7c25abdc -- lib/bounded-writer-admission.ts tests/bounded-writer-admission.test.ts tests/writer-edit-surface-scope.test.tsRepository: Gentleman-Programming/gentle-shell Length of output: 44183 🏁 Script executed: set -o pipefail
printf '%s\n' '--- bounded admission tests ---'
sed -n '1,180p' tests/bounded-writer-admission.test.ts
printf '%s\n' '--- dispatch/parser tests ---'
sed -n '380,525p' tests/writer-edit-surface-scope.test.ts
printf '%s\n' '--- changed-file diff against merge base ---'
git diff --unified=60 43541a0d30dfafacfcb2e99ef4135ab8b1693e99 46198c380bcdfdb2fead7852a3dddafd7c25abdc -- lib/bounded-writer-admission.ts tests/bounded-writer-admission.test.ts tests/writer-edit-surface-scope.test.tsRepository: Gentleman-Programming/gentle-shell Length of output: 41286 🏁 Script executed: set -o pipefail
sed -n '1,180p' tests/bounded-writer-admission.test.ts
sed -n '400,525p' tests/writer-edit-surface-scope.test.ts
git diff --unified=40 43541a0d30dfafacfcb2e99ef4135ab8b1693e99 46198c380bcdfdb2fead7852a3dddafd7c25abdc -- tests/bounded-writer-admission.test.ts tests/writer-edit-surface-scope.test.ts lib/bounded-writer-admission.tsRepository: Gentleman-Programming/gentle-shell Length of output: 35483 Add focused edge-case coverage for the new path helpers. The It does not cover 🤖 Prompt for AI Agents |
||
|
|
||
| // 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<string, unknown>; | ||
| 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.` }; | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The symlink escape check uses a string prefix, and a broken symlink skips it.
The fallback containment check at Line 119 compares
realCandidateagainstrWithSlashwithstartsWith, and it also compares the lowercased strings. On a case-sensitive POSIX filesystem, the case-insensitive comparison accepts a real path that differs only in letter case, such as/Repo/against/repo/. A second gap exists:realpathSync(candidate)throws for a dangling symlink, and thecatchthen returnsrel. As a result, a path through a symlinked parent directory that points outside the repository still gets a suggestion. Admission still rejects the path, so this does not bypass access control. The model still receives a wrong in-repo hint, which goes against the issue requirement that paths outside the repository keep the generic rejection.To fix this, compare paths with
path.relativeand reject any result that starts with..or is absolute. Use the case-insensitive comparison only for Windows-drive roots. When the full candidate does not exist, resolve its nearest existing ancestor.Based on learnings: "do not rely on string prefix ... compute the relative path via a proper path API (e.g. Node.js
path.relative)".🤖 Prompt for AI Agents
Source: Learnings