fix(issue_label): schema wrongly required labels, blocking label_ids-only callers (#52 review finding)
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) <noreply@anthropic.com>
This commit is contained in:
@@ -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"]
|
||||
}`),
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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")
|
||||
|
||||
Reference in New Issue
Block a user