From 7a9125b95360004ded3ac8e3b81fe52eed1793c7 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Thu, 28 May 2026 14:54:17 +0300 Subject: [PATCH] fix(agent/agentfiles): merge duplicate file paths instead of rejecting (#25767) When a caller sends multiple entries for the same literal path, merge their edits into a single entry rather than returning 400. Symlink aliases (different paths, same real file) are still rejected. --- agent/agentfiles/files.go | 27 +++++++++------ agent/agentfiles/files_test.go | 61 ++++++++++++++++++++++++++-------- 2 files changed, 64 insertions(+), 24 deletions(-) diff --git a/agent/agentfiles/files.go b/agent/agentfiles/files.go index 4f92a8f7c9..1ee83e7371 100644 --- a/agent/agentfiles/files.go +++ b/agent/agentfiles/files.go @@ -387,17 +387,17 @@ func (api *API) HandleEditFiles(rw http.ResponseWriter, r *http.Request) { return } - // Duplicate entries both read the same file and race to write; - // the first entry's edits are silently lost. Resolve symlinks - // before comparing so two paths that alias the same real file - // (e.g. one via a symlink, one direct) don't slip past as - // distinct keys. prepareFileEdit resolves the path again for - // its own use; the double lstat cost is cheap compared to the - // data-loss risk of silent aliasing. + // Merge duplicate entries that refer to the same literal path + // so callers don't have to pre-coalesce. Two different paths + // that resolve to the same real file via symlinks are still + // rejected: silently merging edits the caller addressed to + // different paths would hide accidental aliasing. type seenEntry struct { caller string + index int // position in merged slice } seenPaths := make(map[string]seenEntry, len(req.Files)) + var merged []workspacesdk.FileEdits for _, f := range req.Files { // On resolve error, use the raw path; phase 1 surfaces // the error with its proper status code. @@ -406,17 +406,22 @@ func (api *API) HandleEditFiles(rw http.ResponseWriter, r *http.Request) { key = resolved } if prev, dup := seenPaths[key]; dup { - msg := fmt.Sprintf("duplicate file path %q: combine edits into a single entry's \"edits\" list", f.Path) - if prev.caller != f.Path { - msg = fmt.Sprintf("duplicate file path %q aliases %q (same real file): combine edits into a single entry's \"edits\" list", f.Path, prev.caller) + // Same literal path: merge edits. + if filepath.Clean(prev.caller) == filepath.Clean(f.Path) { + merged[prev.index].Edits = append(merged[prev.index].Edits, f.Edits...) + continue } + // Different paths, same real file (symlink alias). + msg := fmt.Sprintf("duplicate file path %q aliases %q (same real file): combine edits into a single entry's \"edits\" list", f.Path, prev.caller) httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: msg, }) return } - seenPaths[key] = seenEntry{caller: f.Path} + seenPaths[key] = seenEntry{caller: f.Path, index: len(merged)} + merged = append(merged, f) } + req.Files = merged // Phase 1: compute all edits in memory. If any file fails // (bad path, search miss, permission error), bail before diff --git a/agent/agentfiles/files_test.go b/agent/agentfiles/files_test.go index 8f8572b74c..8fcdaba810 100644 --- a/agent/agentfiles/files_test.go +++ b/agent/agentfiles/files_test.go @@ -2622,11 +2622,10 @@ func TestFuzzyReplace_Rejects(t *testing.T) { } } -// TestEditFiles_DuplicatePath_Rejects pins that duplicate paths in -// one request are rejected with 400 and the file on disk is -// unchanged. The pre-fix behavior silently dropped the first -// entry's edits while reporting success (last write wins). -func TestEditFiles_DuplicatePath_Rejects(t *testing.T) { +// TestEditFiles_DuplicatePath_Merges verifies that duplicate paths in +// one request are merged: edits from all entries for the same path are +// concatenated and applied in order. +func TestEditFiles_DuplicatePath_Merges(t *testing.T) { t.Parallel() tmpdir := os.TempDir() @@ -2637,10 +2636,12 @@ func TestEditFiles_DuplicatePath_Rejects(t *testing.T) { original := "one\ntwo\nthree\n" require.NoError(t, afero.WriteFile(fs, path, []byte(original), 0o644)) + // Entry 2 searches for the output of entry 1, proving edits + // are applied in the order they appear across entries. req := workspacesdk.FileEditRequest{ Files: []workspacesdk.FileEdits{ - {Path: path, Edits: []workspacesdk.FileEdit{{Search: "one", Replace: "ONE"}}}, - {Path: path, Edits: []workspacesdk.FileEdit{{Search: "three", Replace: "THREE"}}}, + {Path: path, Edits: []workspacesdk.FileEdit{{Search: "one", Replace: "CHANGED"}}}, + {Path: path, Edits: []workspacesdk.FileEdit{{Search: "CHANGED", Replace: "FINAL"}}}, }, } @@ -2653,15 +2654,49 @@ func TestEditFiles_DuplicatePath_Rejects(t *testing.T) { r := httptest.NewRequestWithContext(ctx, http.MethodPost, "/edit-files", buf) api.Routes().ServeHTTP(w, r) - require.Equal(t, http.StatusBadRequest, w.Code, "body: %s", w.Body.String()) - got := &codersdk.Error{} - require.NoError(t, json.NewDecoder(w.Body).Decode(got)) - require.ErrorContains(t, got, "duplicate file path") + require.Equal(t, http.StatusOK, w.Code, "body: %s", w.Body.String()) - // File on disk must be untouched: no partial edits. data, err := afero.ReadFile(fs, path) require.NoError(t, err) - require.Equal(t, original, string(data)) + require.Equal(t, "FINAL\ntwo\nthree\n", string(data)) +} + +// TestEditFiles_DuplicatePath_NonCanonicalMerges verifies that +// non-canonical paths normalizing to the same file are merged, +// not rejected as symlink aliases. +func TestEditFiles_DuplicatePath_NonCanonicalMerges(t *testing.T) { + t.Parallel() + + tmpdir := os.TempDir() + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug) + fs := afero.NewMemMapFs() + api := agentfiles.NewAPI(logger, fs, nil) + canonical := filepath.Join(tmpdir, "noncanon") + nonCanonical := canonical[:len(tmpdir)] + "/./noncanon" + original := "one\ntwo\nthree\n" + require.NoError(t, afero.WriteFile(fs, canonical, []byte(original), 0o644)) + + req := workspacesdk.FileEditRequest{ + Files: []workspacesdk.FileEdits{ + {Path: canonical, Edits: []workspacesdk.FileEdit{{Search: "one", Replace: "ONE"}}}, + {Path: nonCanonical, Edits: []workspacesdk.FileEdit{{Search: "three", Replace: "THREE"}}}, + }, + } + + ctx := testutil.Context(t, testutil.WaitShort) + buf := bytes.NewBuffer(nil) + enc := json.NewEncoder(buf) + enc.SetEscapeHTML(false) + require.NoError(t, enc.Encode(req)) + w := httptest.NewRecorder() + r := httptest.NewRequestWithContext(ctx, http.MethodPost, "/edit-files", buf) + api.Routes().ServeHTTP(w, r) + + require.Equal(t, http.StatusOK, w.Code, "body: %s", w.Body.String()) + + data, err := afero.ReadFile(fs, canonical) + require.NoError(t, err) + require.Equal(t, "ONE\ntwo\nTHREE\n", string(data)) } // TestEditFiles_DuplicatePath_SymlinkAliasRejects pins that two