refactor: move the transport onto the official Gitea SDK

The transport was hand-rolled net/http against the REST API. The payload shapes were ours, in internal/wire, which meant every field Gitea learned was a field somebody here had to notice; and "does this instance have issue dependencies?" had to be guessed from a status code, because a 404 from a missing route and a 404 from a missing issue look alike. The SDK settles both. The shapes are maintained by the people who maintain the server, and the client negotiates the server's version when it is built, so the dependency endpoint is now gated on `>= 1.20.0` — verified against the release where the route appears, not assumed. Below the gate nothing is requested at all. internal/wire keeps what the SDK has no answer for: addressing. The SDK takes an owner, a name and an int64 and never parses, while `42`, `#42`, `owner/repo#42` and an issue URL are four spellings of one address, all four are what somebody has in hand, and Key is what the ledger is keyed by. The payload structs go. Four things that had to survive the move, and did: - request bodies still land in .kettle/payload/, now via an http.RoundTripper on the client the SDK is given — which is better than before, because it files every request rather than the ones a call site remembered to name; - errors still carry the HTTP status AND the response body, and a decode failure on a 2xx is deliberately not an APIError, so the dependency probe cannot read a bad decode as "feature missing"; - the number -> slug ledger is untouched, entries still outlive the files they name; - Client.For(repo) still re-points at another repository for one call. What it cost, written down in AGENTS.md where it happened. internal/mapping's layering test was a fact about the import graph — nothing in its closure could open a socket — and the SDK ships its types and its client in one package, so the test now asserts what is still true: the bridge performs no I/O, checked on direct imports plus a grep for time.Now. A run makes one extra request before it does anything. Gitea's issue edit endpoint carries no labels, so a push whose labels changed needs a second call; push makes it and says so. go.mod requires go 1.26, which the SDK sets and which is now the floor for building this binary. internal/issue and internal/project are byte-identical. The domain did not notice, which is the whole argument for the layering. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-11 19:29:38 +05:00
parent 9480e48312
commit 1239fdee70
292 changed files with 51205 additions and 864 deletions
@@ -9,6 +9,12 @@ package gitea_test
 // — and a request dump would land in the developer's own project. Nothing here
 // reads a login file either, but KETTLE_CONFIG_HOME is redirected all the same,
 // so a run can neither read nor overwrite the developer's own tokens.
+//
+// EVERY FAKE ANSWERS /api/v1/version, because building a client is now a
+// request: the SDK asks the instance what it is before it hands one back, and
+// that answer is what the dependency gate is decided on later. A fake that did
+// not answer it would be a fake no client can be built against — see
+// versionRoute.

 import (
 	"encoding/json"
@@ -19,15 +25,22 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
+	"reflect"
 	"strconv"
 	"strings"
 	"testing"

+	sdk "code.gitea.io/sdk/gitea"
+
 	"git.noodles.cam/claude-skills/marketplace/cli/internal/config"
 	"git.noodles.cam/claude-skills/marketplace/cli/internal/gitea"
 	"git.noodles.cam/claude-skills/marketplace/cli/internal/wire"
 )

+// modernGitea is what a fake says it is: new enough for everything this
+// transport asks for, dependency endpoints included.
+const modernGitea = "1.26.1"
+
 // newProject makes an initialized project and points the walk at it. Returns
 // the project root.
 func newProject(t *testing.T) string {
@@ -41,6 +54,31 @@ func newProject(t *testing.T) string {
 	return dir
 }

+// versionRoute answers the SDK's version handshake and reports whether it did,
+// so every other handler can be written as though the request were not there.
+func versionRoute(w http.ResponseWriter, r *http.Request, version string) bool {
+	if r.URL.Path != "/api/v1/version" {
+		return false
+	}
+	w.Header().Set("Content-Type", "application/json")
+	_, _ = io.WriteString(w, `{"version":"`+version+`"}`)
+	return true
+}
+
+// serve is an httptest server that speaks the handshake and hands everything
+// else to next.
+func serve(t *testing.T, version string, next http.HandlerFunc) *httptest.Server {
+	t.Helper()
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if versionRoute(w, r, version) {
+			return
+		}
+		next(w, r)
+	}))
+	t.Cleanup(srv.Close)
+	return srv
+}
+
 func newClient(t *testing.T, url string) *gitea.Client {
 	t.Helper()
 	c, err := gitea.New(&config.Resolved{URL: url, Token: "s3cret", Owner: "acme", Repo: "widgets"})
@@ -65,7 +103,7 @@ func TestPaginationFollowsToTheLastPage(t *testing.T) {

 	var asked []string
 	var auth string
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+	srv := serve(t, modernGitea, func(w http.ResponseWriter, r *http.Request) {
 		asked = append(asked, r.URL.RequestURI())
 		auth = r.Header.Get("Authorization")

@@ -82,8 +120,7 @@ func TestPaginationFollowsToTheLastPage(t *testing.T) {
 			out = append(out, map[string]any{"id": (page-1)*limit + i + 1, "body": "hello"})
 		}
 		writeJSON(t, w, out)
-	}))
-	defer srv.Close()
+	})

 	got, err := newClient(t, srv.URL).ListComments(42)
 	if err != nil {
@@ -102,7 +139,7 @@ func TestPaginationFollowsToTheLastPage(t *testing.T) {
 	if auth != "token s3cret" {
 		t.Errorf("Authorization was %q, want %q", auth, "token s3cret")
 	}
-	want := "/api/v1/repos/acme/widgets/issues/42/comments?page=1&limit=50"
+	want := "/api/v1/repos/acme/widgets/issues/42/comments?limit=50&page=1"
 	if asked[0] != want {
 		t.Errorf("first request was %s, want %s", asked[0], want)
 	}
@@ -113,11 +150,10 @@ func TestPaginationFollowsToTheLastPage(t *testing.T) {
 func TestErrorNamesTheStatusAndTheBody(t *testing.T) {
 	newProject(t)

-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+	srv := serve(t, modernGitea, func(w http.ResponseWriter, r *http.Request) {
 		w.WriteHeader(http.StatusUnprocessableEntity)
 		io.WriteString(w, `{"message":"label already exists","url":"https://example.test/docs"}`)
-	}))
-	defer srv.Close()
+	})

 	_, err := newClient(t, srv.URL).GetIssue(7)
 	if err == nil {
@@ -152,21 +188,20 @@ func TestPayloadLandsBesideTheStoreAndNeverInIt(t *testing.T) {
 	root := newProject(t)

 	var sent []byte
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+	srv := serve(t, modernGitea, func(w http.ResponseWriter, r *http.Request) {
 		sent, _ = io.ReadAll(r.Body)
 		writeJSON(t, w, map[string]any{
 			"number": 42, "id": 5, "html_url": "https://example.test/acme/widgets/issues/42"})
-	}))
-	defer srv.Close()
+	})

 	body := "<!-- kettle:id wire-sqlc --> a & b"
 	got, err := newClient(t, srv.URL).CreateIssue(
-		wire.IssueRequest{Title: wire.Set("wire sqlc"), Body: wire.Set(body)}, "issue-wire-sqlc")
+		sdk.CreateIssueOption{Title: "wire sqlc", Body: body}, "issue-wire-sqlc")
 	if err != nil {
 		t.Fatalf("CreateIssue: %v", err)
 	}
-	if got.Number != 42 {
-		t.Errorf("got issue #%d, want #42", got.Number)
+	if got.Index != 42 {
+		t.Errorf("got issue #%d, want #42", got.Index)
 	}

 	path := filepath.Join(root, ".kettle", "payload", "issue-wire-sqlc.json")
@@ -174,19 +209,33 @@ func TestPayloadLandsBesideTheStoreAndNeverInIt(t *testing.T) {
 	if err != nil {
 		t.Fatalf("the request body was not filed at %s: %v", path, err)
 	}
-	if string(raw) != string(sent) {
+	// The same JSON VALUE as went out, and not the same bytes: the SDK marshals
+	// with encoding/json's defaults, so what is on the wire has its markup
+	// escaped and no indentation. A dump nobody can read is a dump nobody
+	// reads, and a re-POST of this file sends what this call sent.
+	var filed, onTheWire any
+	if err := json.Unmarshal(raw, &filed); err != nil {
+		t.Fatalf("the filed body is not JSON: %v\n%s", err, raw)
+	}
+	if err := json.Unmarshal(sent, &onTheWire); err != nil {
+		t.Fatalf("what was sent is not JSON: %v\n%s", err, sent)
+	}
+	if !reflect.DeepEqual(filed, onTheWire) {
 		t.Errorf("the filed body is not the body that was sent:\nfiled: %s\nsent:  %s", raw, sent)
 	}
 	// A dump escaped to \u003c is unreadable exactly when it is being read.
 	if !strings.Contains(string(raw), "<!-- kettle:id wire-sqlc --> a & b") {
 		t.Errorf("the dump escaped the markup it was meant to preserve:\n%s", raw)
 	}
+	if !strings.Contains(string(raw), "\n  \"") {
+		t.Errorf("the dump is not indented:\n%s", raw)
+	}
 	// The whole reason the scratchpad is a sibling.
 	if _, err := os.Stat(filepath.Join(root, ".kettle", "issues")); !os.IsNotExist(err) {
 		t.Errorf("writing a request body materialized the issue store (%v)", err)
 	}
 	// A namespaced name must not climb out of the scratchpad.
-	if _, err := newClient(t, srv.URL).CreateLabel(wire.LabelRequest{Name: "type/bug", Color: "#ee0701"}); err != nil {
+	if _, err := newClient(t, srv.URL).CreateLabel(sdk.CreateLabelOption{Name: "type/bug", Color: "#ee0701"}); err != nil {
 		t.Fatalf("CreateLabel: %v", err)
 	}
 	if _, err := os.Stat(filepath.Join(root, ".kettle", "payload", "label-type-bug.json")); err != nil {
@@ -198,14 +247,14 @@ func TestPayloadLandsBesideTheStoreAndNeverInIt(t *testing.T) {
 }

 // A run that sends no body leaves no directory behind — the scratchpad is
-// created by the first write and only then.
+// created by the first write and only then. The version handshake every client
+// opens with is a read, so building one is not "a run that sent something".
 func TestAReadOnlyCallCreatesNoScratchpad(t *testing.T) {
 	root := newProject(t)

-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+	srv := serve(t, modernGitea, func(w http.ResponseWriter, r *http.Request) {
 		writeJSON(t, w, map[string]any{"number": 42})
-	}))
-	defer srv.Close()
+	})

 	if _, err := newClient(t, srv.URL).GetIssue(42); err != nil {
 		t.Fatalf("GetIssue: %v", err)
@@ -221,7 +270,7 @@ func TestAReadOnlyCallCreatesNoScratchpad(t *testing.T) {
 func TestListIssuesRechecksWhatTheServerIgnored(t *testing.T) {
 	newProject(t)

-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+	srv := serve(t, modernGitea, func(w http.ResponseWriter, r *http.Request) {
 		if strings.Contains(r.URL.Path, "/milestones") {
 			writeJSON(t, w, []map[string]any{{"id": 3, "title": "v1"}, {"id": 9, "title": "later"}})
 			return
@@ -237,8 +286,7 @@ func TestListIssuesRechecksWhatTheServerIgnored(t *testing.T) {
 			{"number": 4, "title": "a pull request", "milestone": map[string]any{"id": 3, "title": "v1"},
 				"pull_request": map[string]any{"merged": false}},
 		})
-	}))
-	defer srv.Close()
+	})

 	got, err := newClient(t, srv.URL).ListIssues(gitea.IssueFilter{Milestone: "v1", Limit: 50})
 	if err != nil {
@@ -247,7 +295,7 @@ func TestListIssuesRechecksWhatTheServerIgnored(t *testing.T) {
 	if got.Milestone != "v1" {
 		t.Errorf("resolved milestone is %q, want v1", got.Milestone)
 	}
-	if len(got.Issues) != 1 || got.Issues[0].Number != 1 {
+	if len(got.Issues) != 1 || got.Issues[0].Index != 1 {
 		t.Fatalf("got %d issue(s) %v, want only #1 — the backlog was not re-filtered", len(got.Issues), got.Issues)
 	}

@@ -265,7 +313,7 @@ func TestListIssuesStopsAtThePageBudget(t *testing.T) {
 	newProject(t)

 	pages := 0
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+	srv := serve(t, modernGitea, func(w http.ResponseWriter, r *http.Request) {
 		pages++
 		limit, _ := strconv.Atoi(r.URL.Query().Get("limit"))
 		out := []map[string]any{}
@@ -273,12 +321,11 @@ func TestListIssuesStopsAtThePageBudget(t *testing.T) {
 			out = append(out, map[string]any{"number": pages*100 + i, "state": "closed"})
 		}
 		writeJSON(t, w, out)
-	}))
-	defer srv.Close()
+	})

 	got, err := newClient(t, srv.URL).ListIssues(gitea.IssueFilter{
 		Limit: 2,
-		Keep:  func(i *wire.Issue) bool { return i.State == "open" },
+		Keep:  func(i *sdk.Issue) bool { return i.State == sdk.StateOpen },
 	})
 	if err != nil {
 		t.Fatalf("ListIssues: %v", err)
@@ -302,7 +349,7 @@ func TestListIssuesStopsAtTheLimit(t *testing.T) {
 	newProject(t)

 	pages := 0
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+	srv := serve(t, modernGitea, func(w http.ResponseWriter, r *http.Request) {
 		pages++
 		limit, _ := strconv.Atoi(r.URL.Query().Get("limit"))
 		out := []map[string]any{}
@@ -310,12 +357,11 @@ func TestListIssuesStopsAtTheLimit(t *testing.T) {
 			out = append(out, map[string]any{"number": pages*100 + i, "state": "open"})
 		}
 		writeJSON(t, w, out)
-	}))
-	defer srv.Close()
+	})

 	got, err := newClient(t, srv.URL).ListIssues(gitea.IssueFilter{
 		Limit: 2,
-		Keep:  func(i *wire.Issue) bool { return i.State == "open" },
+		Keep:  func(i *sdk.Issue) bool { return i.State == sdk.StateOpen },
 	})
 	if err != nil {
 		t.Fatalf("ListIssues: %v", err)
@@ -328,29 +374,122 @@ func TestListIssuesStopsAtTheLimit(t *testing.T) {
 	}
 }

-// A dependency endpoint the instance does not have is "no dependencies", not a
-// failed pull. A dead connection still is one.
+// A dependency endpoint the instance has but this repository does not is "no
+// dependencies", not a failed pull. A dead connection still is one.
 func TestDependenciesToleratesAnInstanceWithoutThem(t *testing.T) {
 	newProject(t)

-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+	srv := serve(t, modernGitea, func(w http.ResponseWriter, r *http.Request) {
 		http.Error(w, "not implemented", http.StatusNotImplemented)
-	}))
-	defer srv.Close()
+	})

-	got, err := newClient(t, srv.URL).DependencyKeys(42)
+	// Both clients are built while the server is up, because building one is
+	// itself a request now — and the question this asks is what a call does
+	// when the connection dies UNDER it, which is a different failure from a
+	// tracker that was never there.
+	live := newClient(t, srv.URL)
+	dead := newClient(t, srv.URL)
+
+	got, err := live.DependencyKeys(42)
 	if err != nil || len(got) != 0 {
 		t.Errorf("DependencyKeys = %v, %v; want no keys and no error", got, err)
 	}

 	srv.Close()
-	if _, err := newClient(t, srv.URL).Dependencies(42); err == nil {
+	if _, err := dead.Dependencies(42); err == nil {
 		t.Error("a dead connection was reported as an instance without dependency support")
 	}
 }

+// The gate the SDK made possible: an instance too old to have the endpoint at
+// all is answered from its version, without a request.
+//
+// It matters because the alternative is guessing from a status code. An old
+// Gitea answers a route it does not have with the same 404 it answers for an
+// issue that does not exist, and a pull that read the second as "no blockers"
+// would quietly drop half the unit of work.
+func TestDependenciesAreNotAskedForOnAnInstanceTooOldToHaveThem(t *testing.T) {
+	newProject(t)
+
+	asked := 0
+	srv := serve(t, "1.19.4", func(w http.ResponseWriter, r *http.Request) {
+		asked++
+		writeJSON(t, w, []map[string]any{{"number": 7}})
+	})
+
+	c := newClient(t, srv.URL)
+	got, err := c.Dependencies(42)
+	if err != nil || len(got) != 0 {
+		t.Errorf("Dependencies = %v, %v; want none and no error", got, err)
+	}
+	if asked != 0 {
+		t.Errorf("%d request(s) went out — the version had already answered", asked)
+	}
+
+	// Writing one says so out loud instead: push reports it beside the issue it
+	// could not link, and a warning naming the version is something an operator
+	// can act on.
+	err = c.AddDependency(42, wire.Key{Repo: wire.Repo{Owner: "acme", Name: "widgets"}, Number: 7})
+	if err == nil {
+		t.Fatal("a link was attempted against an instance that has no endpoint for it")
+	}
+	if !strings.Contains(err.Error(), "1.20.0") {
+		t.Errorf("the refusal does not name the version that would work: %v", err)
+	}
+	if asked != 0 {
+		t.Errorf("%d request(s) went out for a write the instance cannot take", asked)
+	}
+}
+
+// And the other side of the same gate: a modern instance is asked, and the
+// answer comes back as cross-repo keys — a dependency is allowed to live
+// somewhere else, and a bare number would not say where.
+func TestDependenciesAreAskedForOnAnInstanceThatHasThem(t *testing.T) {
+	newProject(t)
+
+	var body string
+	srv := serve(t, modernGitea, func(w http.ResponseWriter, r *http.Request) {
+		if r.Method == http.MethodPost {
+			raw, _ := io.ReadAll(r.Body)
+			body = string(raw)
+			w.WriteHeader(http.StatusCreated)
+			return
+		}
+		writeJSON(t, w, []map[string]any{
+			{"number": 7},
+			{"number": 3, "repository": map[string]any{"full_name": "other/repo"}},
+		})
+	})
+
+	c := newClient(t, srv.URL)
+	got, err := c.DependencyKeys(42)
+	if err != nil {
+		t.Fatalf("DependencyKeys: %v", err)
+	}
+	want := []wire.Key{
+		{Repo: wire.Repo{Owner: "acme", Name: "widgets"}, Number: 7},
+		{Repo: wire.Repo{Owner: "other", Name: "repo"}, Number: 3},
+	}
+	if !reflect.DeepEqual(got, want) {
+		t.Errorf("keys = %v, want %v", got, want)
+	}
+
+	// The one endpoint left with a hand-rolled request, because the SDK's
+	// IssueMeta carries an index and nothing else: a link to another repository
+	// needs the owner and the name with it.
+	if err := c.AddDependency(42, want[1]); err != nil {
+		t.Fatalf("AddDependency: %v", err)
+	}
+	for _, part := range []string{`"index":3`, `"owner":"other"`, `"repo":"repo"`} {
+		if !strings.Contains(body, part) {
+			t.Errorf("the link body does not carry %s: %s", part, body)
+		}
+	}
+}
+
 // A half-filled configuration is refused here rather than at the first 401,
-// because a 401 names nothing an operator can act on.
+// because a 401 names nothing an operator can act on — and before the client is
+// built at all, because building one dials.
 func TestNewRefusesAHalfFilledConfiguration(t *testing.T) {
 	for _, tc := range []struct {
 		what string
@@ -372,14 +511,54 @@ func TestNewRefusesAHalfFilledConfiguration(t *testing.T) {
 	}
 }

+// An instance nobody can reach is named as one. The handshake is the first
+// request of every run, so this is the failure an operator meets when the URL
+// is wrong or the tracker is down, and it has to say which instance.
+func TestNewSaysWhichInstanceItCouldNotReach(t *testing.T) {
+	newProject(t)
+
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}))
+	srv.Close()
+
+	_, err := gitea.New(&config.Resolved{URL: srv.URL, Token: "t", Owner: "a", Repo: "b"})
+	if err == nil {
+		t.Fatal("a client was built against a tracker that is not there")
+	}
+	if !strings.Contains(err.Error(), srv.URL) {
+		t.Errorf("the error does not name the instance: %v", err)
+	}
+}
+
+// A version nobody can parse is refused at the handshake, not carried.
+//
+// The SDK hands back a client that has silently decided the server is 1.11 and
+// then rewrites issue URLs from a field a modern payload need not carry, which
+// is a nil dereference on the first issue read. A refusal that names the
+// instance is the answer an operator can act on.
+func TestNewRefusesAnInstanceWhoseVersionIsNotOne(t *testing.T) {
+	newProject(t)
+
+	srv := serve(t, "not-a-version", func(w http.ResponseWriter, r *http.Request) {
+		t.Errorf("a call went out to %s after the handshake had already failed", r.URL.Path)
+	})
+
+	_, err := gitea.New(&config.Resolved{URL: srv.URL, Token: "t", Owner: "a", Repo: "b"})
+	if err == nil {
+		t.Fatal("an unreadable version was accepted — the first issue read would panic inside the SDK")
+	}
+	if !strings.Contains(err.Error(), srv.URL) || !strings.Contains(err.Error(), config.EnvURL) {
+		t.Errorf("the refusal names neither the instance nor the setting that points at it: %v", err)
+	}
+}
+
 // The layering rule, from this side. The transport knows numbers, logins, HTTP
 // and JSON; the domain knows none of those, and neither may reach the other.
 //
 // The bridge is out too, and for a reason of its own: it is the layer that
 // translates between the two, so it sits ABOVE both. A transport that imported
 // it would be a transport that knows what an issue is, one indirection later —
-// and the protocol both of them share, internal/wire, exists precisely so that
-// neither has to reach for the other to name a payload.
+// and the vocabulary both of them share, the SDK's payloads, exists precisely
+// so that neither has to reach for the other to name one.
 func TestTransportDoesNotImportTheDomain(t *testing.T) {
 	out, err := exec.Command("go", "list", "-deps", ".").Output()
 	if err != nil {