From ac337955e74faed47edf248f92d93d61962ddf0f Mon Sep 17 00:00:00 2001 From: Mathias Date: Sat, 4 Jul 2026 13:57:05 +0200 Subject: [PATCH] fix(issue_label): schema wrongly required labels, blocking label_ids-only callers (#52 review finding) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Independent adversarial review of #52 (v0.8.0) caught a schema/implementation mismatch: the advertised InputSchema marked "labels" as required, but Call already treated labels/label_ids as either-or. An MCP client that validates arguments against the advertised schema before dispatch would reject a label_ids-only call as invalid even though the code was written to serve it — and that path had zero test coverage either way. Dropped "labels" from the required array (owner/repo/number remain required); runtime validation already correctly requires at least one of labels/label_ids. Added TestIssueLabelAppliesByIDOnly (asserts ListLabels is never called when IDs are already known) and a schema-lock test for the fixed contract. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/tools/issue_label.go | 6 ++-- internal/tools/issue_label_test.go | 46 ++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 3 deletions(-) diff --git a/internal/tools/issue_label.go b/internal/tools/issue_label.go index 9ce6eaa..c778d46 100644 --- a/internal/tools/issue_label.go +++ b/internal/tools/issue_label.go @@ -29,10 +29,10 @@ func (t *IssueLabel) Descriptor() registry.ToolDescriptor { "owner":{"type":"string"}, "repo":{"type":"string"}, "number":{"type":"integer","minimum":1}, - "labels":{"type":"array","items":{"type":"string"}}, - "label_ids":{"type":"array","items":{"type":"integer"}} + "labels":{"type":"array","items":{"type":"string"},"description":"Label names to resolve and apply. Either labels or label_ids is required."}, + "label_ids":{"type":"array","items":{"type":"integer"},"description":"Label IDs to apply directly, skipping name resolution. Either labels or label_ids is required."} }, - "required":["owner","repo","number","labels"] + "required":["owner","repo","number"] }`), } } diff --git a/internal/tools/issue_label_test.go b/internal/tools/issue_label_test.go index 1957262..654f31e 100644 --- a/internal/tools/issue_label_test.go +++ b/internal/tools/issue_label_test.go @@ -53,6 +53,52 @@ func TestIssueLabelAppliesByName(t *testing.T) { assert.Contains(t, string(out), `"name":"enhancement"`) } +// label_ids alone (no labels) must work end-to-end without hitting ListLabels +// at all — this is the schema-level "either labels or label_ids" contract, and +// it must never require a GET to the label list when the caller already has IDs. +func TestIssueLabelAppliesByIDOnly(t *testing.T) { + var captured []byte + var listCalled bool + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodGet && r.URL.Path == "/api/v1/repos/o/r/labels": + listCalled = true + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(labelListFixture)) + case r.Method == http.MethodPost && r.URL.Path == "/api/v1/repos/o/r/issues/42/labels": + var err error + captured, err = io.ReadAll(r.Body) + require.NoError(t, err) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(labelListFixture)) + default: + t.Fatalf("unexpected request: %s %s", r.Method, r.URL.Path) + } + })) + defer srv.Close() + + tool := tools.NewIssueLabel(gitea.NewClient(srv.URL, "tok"), allowlist.New([]string{"o"})) + out, err := tool.Call(context.Background(), json.RawMessage(`{"owner":"o","repo":"r","number":42,"label_ids":[1,2]}`)) + require.NoError(t, err) + + assert.False(t, listCalled, "label_ids-only must not call ListLabels") + var payload map[string]any + require.NoError(t, json.Unmarshal(captured, &payload)) + ids, ok := payload["labels"].([]any) + require.True(t, ok) + assert.ElementsMatch(t, []any{float64(1), float64(2)}, ids) + assert.Contains(t, string(out), `"name":"bug"`) +} + +// #52 review finding: the advertised schema wrongly required "labels", making +// label_ids-only calls fail JSON-Schema validation before reaching Call at all. +// Lock the fixed contract: neither is individually required. +func TestIssueLabelSchema_NeitherLabelsNorLabelIDsRequired(t *testing.T) { + sch := string(tools.NewIssueLabel(gitea.NewClient("http://unused", ""), allowlist.New([]string{"o"})).Descriptor().InputSchema) + assert.NotContains(t, sch, `"required":["owner","repo","number","labels"]`) + assert.Contains(t, sch, `"required":["owner","repo","number"]`) +} + func TestIssueLabelUnknownNameNamesTheMissingLabel(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json")