From e5cfcb316ae919da25971ba478ce3c3d8c08c7a1 Mon Sep 17 00:00:00 2001 From: mesh-tools Date: Wed, 21 Oct 2026 09:21:59 +0000 Subject: [PATCH] Fall back to the tool's own patch applier when the forge answers 500, with tests (issue 1604) --- modules/gitea/cmd/forge/applypatch_test.go | 132 +++++++++++++++++++++ modules/gitea/cmd/forge/commit.go | 77 +++++++++++- modules/gitea/cmd/forge/forge_test.go | 8 +- 3 files changed, 211 insertions(+), 6 deletions(-) create mode 100644 modules/gitea/cmd/forge/applypatch_test.go diff --git a/modules/gitea/cmd/forge/applypatch_test.go b/modules/gitea/cmd/forge/applypatch_test.go new file mode 100644 index 00000000..413c3e37 --- /dev/null +++ b/modules/gitea/cmd/forge/applypatch_test.go @@ -0,0 +1,132 @@ +package main + +import ( + "context" + "strings" + "testing" +) + +// novox/hq issue 1604: the forge's diffpatch answers 500 when an unchanged context line holds a non-ASCII +// character; the tool then applies the patch itself. + +func sectionFake(t *testing.T) *fakeForge { + f := newFake(t) + f.trees["c0"]["doc.md"] = "# Title\n\nSee § 4 for the rule.\nold line\nend § here\n" + f.trees["c0"]["run.sh"] = "#!/bin/sh\necho a\n" + f.patchStatus = 500 + return f +} + +const sectionPatch = "diff --git a/doc.md b/doc.md\n--- a/doc.md\n+++ b/doc.md\n@@ -2,4 +2,4 @@\n \n See § 4 for the rule.\n-old line\n+new line\n end § here\n" + +func TestAPatchWhoseContextHoldsANonASCIICharacterIsAppliedByTheToolWhenTheForgeAnswers500(t *testing.T) { + f := sectionFake(t) + got, err := commit(context.Background(), f.serve(), ask(t, map[string]any{"branch": "fix/1604", "patch": sectionPatch})) + if err != nil { + t.Fatal(err) + } + want := "# Title\n\nSee § 4 for the rule.\nnew line\nend § here\n" + if tree := f.trees[got.Commit]; tree["doc.md"] != want || tree["README.md"] != "hello\n" { + t.Fatalf("tree %v", tree) + } + if len(f.patches) != 1 || len(f.writes) != 1 || f.writes[0]["new_branch"] != "fix/1604" || f.writes[0]["branch"] != "main" { + t.Fatalf("patches %v writes %v", f.patches, f.writes) + } + if len(got.Files) != 1 || got.Files[0].Operation != "update" || got.Parent != "c0" { + t.Fatalf("%+v", got) + } + if !strings.Contains(strings.Join(got.Said, " "), "applied it") { + t.Errorf("not said: %v", got.Said) + } +} + +func TestAPatchTheForgeTakesIsStillTheForgesToApply(t *testing.T) { + f := sectionFake(t) + f.patchStatus = 0 + if _, err := commit(context.Background(), f.serve(), ask(t, map[string]any{"branch": "fix/1604", "patch": sectionPatch})); err != nil { + t.Fatal(err) + } + if len(f.writes) != 0 { + t.Fatal("the tool applied a patch the forge took") + } +} + +func TestAPatchThatCreatesDeletesAndRenamesIsApplied(t *testing.T) { + f := sectionFake(t) + patch := "diff --git a/new.txt b/new.txt\nnew file mode 100644\n--- /dev/null\n+++ b/new.txt\n@@ -0,0 +1,2 @@\n+one §\n+two\n" + + "diff --git a/docs/a.md b/docs/a.md\ndeleted file mode 100644\n--- a/docs/a.md\n+++ /dev/null\n@@ -1 +0,0 @@\n-a\n" + + "diff --git a/README.md b/README.txt\nsimilarity index 80%\nrename from README.md\nrename to README.txt\n--- a/README.md\n+++ b/README.txt\n@@ -1 +1 @@\n-hello\n+hello §\n" + got, err := commit(context.Background(), f.serve(), ask(t, map[string]any{"branch": "fix/r", "patch": patch})) + if err != nil { + t.Fatal(err) + } + tree := f.trees[got.Commit] + if tree["new.txt"] != "one §\ntwo\n" || tree["README.txt"] != "hello §\n" { + t.Fatalf("tree %v", tree) + } + if _, has := tree["README.md"]; has { + t.Error("the renamed file is still there") + } + if _, has := tree["docs/a.md"]; has { + t.Error("the deleted file is still there") + } +} + +func TestAHunkMovedByEarlierLinesIsFoundAndNoNewlineIsKept(t *testing.T) { + f := sectionFake(t) + f.trees["c0"]["x.txt"] = "pre\npre\nabc §\nd\ne" + patch := "--- a/x.txt\n+++ b/x.txt\n@@ -1,3 +1,3 @@\n abc §\n d\n-e\n\\ No newline at end of file\n+f\n\\ No newline at end of file\n" + got, err := commit(context.Background(), f.serve(), ask(t, map[string]any{"branch": "fix/n", "patch": patch})) + if err != nil { + t.Fatal(err) + } + if g := f.trees[got.Commit]["x.txt"]; g != "pre\npre\nabc §\nd\nf" { + t.Fatalf("%q", g) + } +} + +func TestAPatchThatDoesNotApplyIsRefusedNamingTheFileAndLine(t *testing.T) { + f := sectionFake(t) + patch := strings.Replace(sectionPatch, "-old line", "-other line", 1) + _, err := commit(context.Background(), f.serve(), ask(t, map[string]any{"branch": "fix/1604", "patch": patch})) + if err == nil || !strings.Contains(err.Error(), "doc.md") || !strings.Contains(err.Error(), "at line 2 does not apply") { + t.Fatalf("%v", err) + } + if len(f.writes) != 0 { + t.Fatal("something was written") + } +} + +func TestAPatchWithAnExecutableFileIsNotAppliedByTheToolAndSaysWhy(t *testing.T) { + f := sectionFake(t) + for _, patch := range []string{ + "diff --git a/s.sh b/s.sh\nnew file mode 100755\n--- /dev/null\n+++ b/s.sh\n@@ -0,0 +1 @@\n+echo §\n", + "diff --git a/run.sh b/run.sh\nold mode 100644\nnew mode 100755\n", + "diff --git a/run.sh b/run.sh\nindex 1111111..2222222 100755\n--- a/run.sh\n+++ b/run.sh\n@@ -1,2 +1,2 @@\n #!/bin/sh\n-echo a\n+echo b\n", + } { + _, err := commit(context.Background(), f.serve(), ask(t, map[string]any{"branch": "fix/x", "patch": patch})) + if err == nil || !strings.Contains(err.Error(), "the forge did not take the patch") || !strings.Contains(err.Error(), "carries no file mode") { + t.Errorf("%v", err) + } + } + if len(f.writes) != 0 { + t.Fatal("something was written") + } +} + +func TestAClientErrorOfTheForgeIsNotRetriedByTheTool(t *testing.T) { + f := sectionFake(t) + f.patchStatus = 422 + _, err := commit(context.Background(), f.serve(), ask(t, map[string]any{"branch": "fix/1604", "patch": sectionPatch})) + if err == nil || len(f.writes) != 0 { + t.Fatalf("%v", err) + } +} + +func TestADefaultBranchIsStillRefusedForAPatch(t *testing.T) { + f := sectionFake(t) + _, err := commit(context.Background(), f.serve(), ask(t, map[string]any{"branch": "main", "patch": sectionPatch})) + if err == nil || !strings.Contains(err.Error(), "ADR 0266") || len(f.patches)+len(f.writes) != 0 { + t.Fatalf("%v", err) + } +} diff --git a/modules/gitea/cmd/forge/commit.go b/modules/gitea/cmd/forge/commit.go index 66e4ced0..95cc3318 100644 --- a/modules/gitea/cmd/forge/commit.go +++ b/modules/gitea/cmd/forge/commit.go @@ -326,14 +326,42 @@ func commit(ctx context.Context, f *Forge, a CommitAsk) (*Committed, error) { var made struct { Commit madeCommit `json:"commit"` } + // A patch goes to the forge's diffpatch first. When the forge answers a server error (it does so, with an + // empty message, when an unchanged context line holds a non-ASCII character: novox/hq issue 1604) the tool + // applies the patch itself and commits the resulting files the way a files commit does. + files := a.Files + forgeApplied := false if a.Patch != "" { on["content"] = a.Patch - if err := f.SendJSON(ctx, http.MethodPost, repo+"/diffpatch", on, &made); err != nil { + err := f.SendJSON(ctx, http.MethodPost, repo+"/diffpatch", on, &made) + var status *forgehttp.StatusError + switch { + case err == nil: + forgeApplied = true + case errors.As(err, &status) && status.Status >= 500: + delete(on, "content") + read := func(p string) ([]byte, bool, error) { return f.contentAt(ctx, repo, p, ref) } + var perr error + if files, perr = patchAsFiles(a.Patch, read); perr != nil { + return nil, fmt.Errorf("the forge did not take the patch: %w; the tool could not apply it itself: %v", err, perr) + } + total := 0 + for _, fc := range files { + if fc.size > maxFileBytes { + return nil, refuse("%s would be %s, more than the %s one file of a commit may be. Nothing was sent", fc.Path, mib(fc.size), mib(maxFileBytes)) + } + total += fc.size + } + if total > maxCommitBytes { + return nil, refuse("the patch's files are %s together, more than the %s one commit carries. Nothing was sent", mib(total), mib(maxCommitBytes)) + } + out.Said = append(out.Said, fmt.Sprintf("the forge answered %d to the patch, so the tool applied it to the files at %s and committed them", status.Status, short(ref))) + default: return nil, fmt.Errorf("the forge did not take the patch: %w", err) } - } else { - files := a.Files - for _, fc := range a.Files { + } + if !forgeApplied { + for _, fc := range files { if fc.CopyFrom != nil { // Read, and judged against the limits, before anything is sent. var err error @@ -391,7 +419,7 @@ func commit(ctx context.Context, f *Forge, a CommitAsk) (*Committed, error) { moved = "base " + out.Base } how := "each file it changes was checked against " + short(read) + " by its blob, so none of them changed in between" - if a.Patch != "" { + if forgeApplied { how = "the forge applied the patch there, and it refuses a patch whose context changed" } out.Said = append(out.Said, fmt.Sprintf("the %s moved from %s to %s between the read and the commit, so the "+ @@ -478,3 +506,42 @@ func short(sha string) string { } return sha } + +// contentAt is the content of a file at a ref; ok is false when there is no file there. +func (f *Forge) contentAt(ctx context.Context, repo, path, ref string) ([]byte, bool, error) { + seg, err := segments("file path", path) + if err != nil { + return nil, false, err + } + body, err := f.Do(ctx, http.MethodGet, repo+"/contents/"+seg+"?ref="+queryEscape(ref), nil, maxBody) + if err != nil { + if errors.Is(err, errNotFound) { + return nil, false, nil + } + return nil, false, err + } + var entry struct { + Type string `json:"type"` + Size int `json:"size"` + Content string `json:"content"` + Encoding string `json:"encoding"` + } + if err := jsonInto(body, &entry); err != nil { + return nil, false, fmt.Errorf("the forge's answer about %s is not readable: %w", path, err) + } + if entry.Type != "file" { + return nil, false, refuse("%s is a %s, not a file. Nothing was sent", path, entry.Type) + } + if entry.Encoding == "base64" && (entry.Content != "" || entry.Size == 0) { + raw, err := base64.StdEncoding.DecodeString(strings.ReplaceAll(entry.Content, "\n", "")) + if err != nil { + return nil, false, fmt.Errorf("the content of %s is not base64: %w", path, err) + } + return raw, true, nil + } + raw, err := f.Do(ctx, http.MethodGet, repo+"/raw/"+seg+"?ref="+queryEscape(ref), nil, maxFileBytes+1) + if err != nil { + return nil, false, err + } + return raw, true, nil +} diff --git a/modules/gitea/cmd/forge/forge_test.go b/modules/gitea/cmd/forge/forge_test.go index 1d4eb928..92249ca4 100644 --- a/modules/gitea/cmd/forge/forge_test.go +++ b/modules/gitea/cmd/forge/forge_test.go @@ -46,6 +46,7 @@ type fakeForge struct { parentsBroken bool // and GET git/commits fails parents map[string]string // commit -> parent, as GET git/commits says it sources map[string]*sourceRepo + patchStatus int // the status diffpatch answers instead of taking the patch (issue 1604) } type move struct { @@ -128,7 +129,8 @@ func (f *fakeForge) handle(w http.ResponseWriter, r *http.Request) { http.NotFound(w, r) return } - reply(map[string]any{"type": "file", "path": p, "sha": blob(content)}) + reply(map[string]any{"type": "file", "path": p, "sha": blob(content), "size": len(content), "encoding": "base64", + "content": base64.StdEncoding.EncodeToString([]byte(content))}) case r.Method == "POST" && path == repo+"/contents": var body map[string]any _ = json.NewDecoder(r.Body).Decode(&body) @@ -176,6 +178,10 @@ func (f *fakeForge) handle(w http.ResponseWriter, r *http.Request) { var body map[string]any _ = json.NewDecoder(r.Body).Decode(&body) f.patches = append(f.patches, body) + if f.patchStatus != 0 { + http.Error(w, `{"message":""}`, f.patchStatus) + return + } f.next++ made := fmt.Sprintf("p%d", f.next) from := f.branches[body["branch"].(string)]