Skip to content

Commit 9483000

Browse files
kevincodex1claude
andcommitted
fix(sandbox): share the structured-patch classifier with apply_patch
Review follow-ups for #956: - The tool accepted decorated structured markers ("***Begin Patch") that the sandbox's applyPatchPaths did not recognise, so for that spelling the boundary scanned the patch as a unified diff, extracted no targets and validated nothing. The marker classifier now lives in the sandbox package (StructuredPatchMarker / IsStructuredPatch) and the tool delegates to it, so both sides accept exactly the same set; tests cover inside, traversal and outside targets for the no-space spelling at the boundary. - Document the containment model at the apply site: structured patches are applied through an opened os.Root (descriptor-relative, no-follow); unified diffs keep the pre-existing git-apply pathname flow, which the absolute-path rewrite does not widen (they become the same root-relative form relative paths always used). - OpenAI addendum: the one-file-per-call rule names read_file, edit_file and write_file explicitly and allows a single multi-file apply_patch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012JqvAf7zsP1qYFVZayKXHq
1 parent ff9b0de commit 9483000

5 files changed

Lines changed: 88 additions & 27 deletions

File tree

‎internal/agent/system_prompt_models.go‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -61,9 +61,11 @@ const openAIPromptAddendum = `<model_guidance>
6161
commands, and symbols.
6262
- Strongly prefer the native file tools (read_file, list_directory, grep, glob,
6363
edit_file, apply_patch, write_file) over shelling out to cat/sed/awk/python
64-
for file work. Make one tool call per file; do not batch file writes into a
65-
script. Independent tool calls (several reads, several edit_file calls to
66-
different files) belong in the same turn.
64+
for file work. read_file, edit_file and write_file take one file per call;
65+
a single apply_patch may span several files when they form one coherent
66+
change. Do not batch file writes into a script. Independent tool calls
67+
(several reads, several edit_file calls to different files) belong in the
68+
same turn.
6769
- Persist until the task is fully handled this turn: gather context, implement,
6870
run the validators, and report — do not stop at a partial result.
6971
</model_guidance>`

‎internal/sandbox/apply_patch_paths_test.go‎

Lines changed: 38 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -16,21 +16,47 @@ func TestApplyPatchPathBlockOnlyRejectsRelativeTraversal(t *testing.T) {
1616
"absolute inside workspace": structured(inside),
1717
"relative": structured("main.js"),
1818
"decorated markers": "*** Begin Patch ***\n*** Update File: main.js\n@@\n-a\n+b\n*** End Patch ***",
19+
"no-space marker": "***Begin Patch\n*** Update File: main.js\n@@\n-a\n+b\n***End Patch",
1920
} {
2021
request := Request{ToolName: "apply_patch", WorkspaceRoot: root, SideEffect: SideEffectWrite, Args: map[string]any{"patch": patch}}
2122
if block := applyPatchPathBlock(request); block != nil {
2223
t.Fatalf("%s: unexpected block %+v", name, block)
2324
}
2425
}
2526
for _, path := range []string{"../escape.js", ".."} {
26-
request := Request{ToolName: "apply_patch", WorkspaceRoot: root, SideEffect: SideEffectWrite, Args: map[string]any{"patch": structured(path)}}
27-
block := applyPatchPathBlock(request)
28-
if block == nil || block.Code != BlockOutsideWorkspace {
29-
t.Fatalf("%q must be blocked as traversal, got %+v", path, block)
27+
for name, patch := range map[string]string{"canonical": structured(path), "no-space": "***Begin Patch\n*** Update File: " + path + "\n@@\n-a\n+b\n***End Patch"} {
28+
request := Request{ToolName: "apply_patch", WorkspaceRoot: root, SideEffect: SideEffectWrite, Args: map[string]any{"patch": patch}}
29+
block := applyPatchPathBlock(request)
30+
if block == nil || block.Code != BlockOutsideWorkspace {
31+
t.Fatalf("%s %q must be blocked as traversal, got %+v", name, path, block)
32+
}
3033
}
3134
}
3235
}
3336

37+
// Every marker spelling the tool applies must be classified as structured at
38+
// the sandbox boundary too; otherwise the boundary scans the patch as a unified
39+
// diff, extracts no targets, and validates nothing (fail-open).
40+
func TestStructuredPatchClassifierMatchesToolSpellings(t *testing.T) {
41+
for _, header := range []string{"*** Begin Patch", "*** Begin Patch ***", "***Begin Patch", " *** Begin Patch ", "\ufeff*** Begin Patch"} {
42+
patch := header + "\n*** Update File: main.js\n@@\n-a\n+b\n*** End Patch"
43+
if !IsStructuredPatch(patch) {
44+
t.Fatalf("%q must classify as a structured patch", header)
45+
}
46+
if paths := applyPatchPaths(patch); len(paths) != 1 || paths[0] != "main.js" {
47+
t.Fatalf("%q: sandbox must extract the structured target, got %v", header, paths)
48+
}
49+
}
50+
for _, header := range []string{"--- a/x", "Begin Patch", "*** Begin Patchwork", "*** Update File: x"} {
51+
if IsStructuredPatch(header + "\n-a\n+b") {
52+
t.Fatalf("%q must not classify as a structured patch", header)
53+
}
54+
}
55+
if StructuredPatchMarker("*** End Patch ***") != "end" || StructuredPatchMarker("***End Patch") != "end" {
56+
t.Fatal("decorated end markers must classify as end")
57+
}
58+
}
59+
3460
func TestApplyPatchRequestPathsCarryAbsolutePathsToScopeValidation(t *testing.T) {
3561
root := t.TempDir()
3662
// NewScope also grants the system temp dir, so a sibling t.TempDir() is
@@ -39,10 +65,14 @@ func TestApplyPatchRequestPathsCarryAbsolutePathsToScopeValidation(t *testing.T)
3965
if err != nil {
4066
t.Fatal(err)
4167
}
42-
patch := strings.Join([]string{"*** Begin Patch", "*** Update File: " + outside, "@@", "-a", "+b", "*** End Patch"}, "\n")
43-
paths := applyPatchRequestPaths(map[string]any{"patch": patch})
44-
if len(paths) != 1 || paths[0] != outside {
45-
t.Fatalf("absolute patch path must reach scope validation unchanged, got %v", paths)
68+
for name, patch := range map[string]string{
69+
"canonical": strings.Join([]string{"*** Begin Patch", "*** Update File: " + outside, "@@", "-a", "+b", "*** End Patch"}, "\n"),
70+
"no-space": strings.Join([]string{"***Begin Patch", "*** Update File: " + outside, "@@", "-a", "+b", "***End Patch"}, "\n"),
71+
} {
72+
paths := applyPatchRequestPaths(map[string]any{"patch": patch})
73+
if len(paths) != 1 || paths[0] != outside {
74+
t.Fatalf("%s: absolute patch path must reach scope validation unchanged, got %v", name, paths)
75+
}
4676
}
4777
scope, err := NewScope(root, nil)
4878
if err != nil {

‎internal/sandbox/risk.go‎

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -300,8 +300,35 @@ func applyPatchPathBlock(request Request) *pathBlock {
300300
return nil
301301
}
302302

303+
// structuredPatchMarkerPattern is the single classifier for structured-patch
304+
// markers, shared by the sandbox boundary and the apply_patch tool (which
305+
// imports this package). It accepts the canonical "*** Begin Patch" /
306+
// "*** End Patch" and the decorated spellings models emit ("*** Begin Patch ***",
307+
// "***Begin Patch", trailing whitespace). Both sides must agree: a spelling the
308+
// tool would apply but the sandbox did not recognise would make the sandbox
309+
// scan the patch as a unified diff, extract no targets, and validate nothing.
310+
var structuredPatchMarkerPattern = regexp.MustCompile(`^\*{3}\s*(Begin|End) Patch\s*\**\s*$`)
311+
312+
// StructuredPatchMarker classifies a line as the "begin" or "end" marker of a
313+
// structured patch, or "" when it is neither.
314+
func StructuredPatchMarker(line string) string {
315+
match := structuredPatchMarkerPattern.FindStringSubmatch(strings.TrimSpace(line))
316+
if match == nil {
317+
return ""
318+
}
319+
return strings.ToLower(match[1])
320+
}
321+
322+
// IsStructuredPatch reports whether patch opens with a structured begin marker.
323+
// The tool applies exactly the patches this returns true for, so the sandbox
324+
// extracts structured header paths for exactly the same set.
325+
func IsStructuredPatch(patch string) bool {
326+
first, _, _ := strings.Cut(strings.TrimSpace(strings.TrimPrefix(patch, "\ufeff")), "\n")
327+
return StructuredPatchMarker(first) == "begin"
328+
}
329+
303330
func applyPatchPaths(patch string) []string {
304-
if strings.HasPrefix(strings.TrimSpace(strings.TrimPrefix(patch, "\ufeff")), "*** Begin Patch") {
331+
if IsStructuredPatch(patch) {
305332
return structuredPatchHeaderPaths(patch)
306333
}
307334
return patchHeaderPaths(patch)

‎internal/tools/apply_patch.go‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,8 +59,18 @@ func (tool applyPatchTool) RunWithOptions(ctx context.Context, args map[string]a
5959
return errorResult("Error applying patch: " + err.Error())
6060
}
6161
if isStructuredPatch(patch) {
62+
// Structured patches (the format models use) are applied by Zero itself
63+
// through an opened workspace root (os.Root): every read, create and
64+
// write below is descriptor-relative and no-follow, so there is no
65+
// pathname check-to-use window.
6266
return tool.runStructuredPatch(applyRoot, relativeRoot, patch, options)
6367
}
68+
// Unified diffs are handed to git apply, which opens targets by name after
69+
// the pathname checks below; that pre-existing check-to-use window is the
70+
// same one the relative-path flow always had. Absolute paths are only ever
71+
// rewritten to the same root-relative form relative paths already use, so
72+
// this does not widen it. Moving unified diffs onto the os.Root path is a
73+
// separate change.
6474
patch = relativizeUnifiedPatchPaths(applyRoot, patch)
6575
if err := validatePatchPaths(applyRoot, patch); err != nil {
6676
return errorResult("Error applying patch: " + err.Error())

‎internal/tools/structured_patch.go‎

Lines changed: 7 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"strings"
1111

1212
"github.com/Gitlawb/zero/internal/pathjail"
13+
"github.com/Gitlawb/zero/internal/sandbox"
1314
)
1415

1516
const (
@@ -64,13 +65,6 @@ type structuredPatchChange struct {
6465
mode os.FileMode
6566
}
6667

67-
// structuredPatchMarkerPattern accepts the canonical "*** Begin Patch" /
68-
// "*** End Patch" markers plus the decorated spellings models commonly emit
69-
// ("*** Begin Patch ***", "***Begin Patch", trailing whitespace). A strict
70-
// byte-equal match here made every patch from some models fail on line 1 and
71-
// pushed them into whole-file rewrites.
72-
var structuredPatchMarkerPattern = regexp.MustCompile(`^\*{3}\s*(Begin|End) Patch\s*\**\s*$`)
73-
7468
// unifiedHunkRangePattern recognises a unified-diff range header ("-12,4 +12,6",
7569
// optionally followed by "@@ heading") written inside a structured hunk marker.
7670
// The line numbers carry no information for the context-anchored grammar, so
@@ -80,13 +74,12 @@ var unifiedHunkRangePattern = regexp.MustCompile(`^-\d+(?:,\d+)?\s+\+\d+(?:,\d+)
8074
const structuredPatchFormatHint = ` (format: "*** Begin Patch", then "*** Update File: path" / "*** Add File: path" / "*** Delete File: path" sections whose hunks start with "@@" and use " " context, "-" removed and "+" added lines, then "*** End Patch")`
8175

8276
// structuredPatchMarker classifies a line as the "begin" or "end" marker of a
83-
// structured patch, or "" when it is neither.
77+
// structured patch, or "" when it is neither. The classifier is owned by the
78+
// sandbox package so the boundary check and the tool accept exactly the same
79+
// spellings; a strict byte-equal match here once made every patch from some
80+
// models fail on line 1 and pushed them into whole-file rewrites.
8481
func structuredPatchMarker(line string) string {
85-
match := structuredPatchMarkerPattern.FindStringSubmatch(strings.TrimSpace(line))
86-
if match == nil {
87-
return ""
88-
}
89-
return strings.ToLower(match[1])
82+
return sandbox.StructuredPatchMarker(line)
9083
}
9184

9285
// structuredHunkAnchor normalises the text after a hunk's "@@ " marker. A
@@ -101,8 +94,7 @@ func structuredHunkAnchor(context string) string {
10194
}
10295

10396
func isStructuredPatch(patch string) bool {
104-
first, _, _ := strings.Cut(strings.TrimSpace(strings.TrimPrefix(patch, "\ufeff")), "\n")
105-
return structuredPatchMarker(first) == "begin"
97+
return sandbox.IsStructuredPatch(patch)
10698
}
10799

108100
func (tool applyPatchTool) runStructuredPatch(applyRoot, relativeRoot, patch string, options RunOptions) Result {

0 commit comments

Comments
 (0)