From 9f12db3d94f37a1263b14cb53cd741acd2bc45c4 Mon Sep 17 00:00:00 2001 From: Mathias Date: Fri, 3 Jul 2026 21:58:05 +0200 Subject: [PATCH] fix(pr_merge): advertise canonical `number`, demote `index` to alias (#45) pr_merge was the only tool still advertising `index` as its id field while every other issue/PR tool uses the canonical `number` (#38). Flipped its schema property + required + struct field/tag `index` -> `number`; the existing `index`->`number` shim keeps legacy `index` callers working, so no shim change was needed (no canonical-`index` tool remains). Now the id arg is `number` uniformly across all per-issue/PR tools. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/tools/pr_merge.go | 22 +++++++++++----------- internal/tools/pr_merge_test.go | 24 ++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 11 deletions(-) diff --git a/internal/tools/pr_merge.go b/internal/tools/pr_merge.go index f6b0e0d..533c52f 100644 --- a/internal/tools/pr_merge.go +++ b/internal/tools/pr_merge.go @@ -28,23 +28,23 @@ func (t *PRMerge) Descriptor() registry.ToolDescriptor { "properties":{ "owner":{"type":"string"}, "repo":{"type":"string"}, - "index":{"type":"integer","minimum":1}, + "number":{"type":"integer","minimum":1}, "style":{"type":"string","enum":["merge","squash","rebase"]}, "merge_message_title":{"type":"string"}, "merge_message_field":{"type":"string"} }, - "required":["owner","repo","index"] + "required":["owner","repo","number"] }`), } } type prMergeArgs struct { - Owner string `json:"owner"` - Repo string `json:"repo"` - Index int `json:"index"` - Style string `json:"style"` - Title string `json:"merge_message_title"` - Body string `json:"merge_message_field"` + Owner string `json:"owner"` + Repo string `json:"repo"` + Number int `json:"number"` + Style string `json:"style"` + Title string `json:"merge_message_title"` + Body string `json:"merge_message_field"` } func (t *PRMerge) Call(ctx context.Context, raw json.RawMessage) (json.RawMessage, error) { @@ -55,8 +55,8 @@ func (t *PRMerge) Call(ctx context.Context, raw json.RawMessage) (json.RawMessag if err := t.a.Check(args.Owner); err != nil { return nil, err } - if args.Index < 1 { - return nil, fmt.Errorf("index must be >= 1: %w", gitea.ErrValidation) + if args.Number < 1 { + return nil, fmt.Errorf("number must be >= 1: %w", gitea.ErrValidation) } style := args.Style @@ -64,7 +64,7 @@ func (t *PRMerge) Call(ctx context.Context, raw json.RawMessage) (json.RawMessag style = "merge" } - if err := t.c.MergePullRequest(ctx, args.Owner, args.Repo, args.Index, gitea.MergePRArgs{ + if err := t.c.MergePullRequest(ctx, args.Owner, args.Repo, args.Number, gitea.MergePRArgs{ Do: style, Title: args.Title, Body: args.Body, diff --git a/internal/tools/pr_merge_test.go b/internal/tools/pr_merge_test.go index 3b586c9..3034433 100644 --- a/internal/tools/pr_merge_test.go +++ b/internal/tools/pr_merge_test.go @@ -63,6 +63,30 @@ func TestPRMergeConflictReturnsError(t *testing.T) { assert.ErrorIs(t, err, gitea.ErrConflict) } +// #45: pr_merge advertises the canonical `number` (was `index`); `index` stays +// an accepted alias via the shim. +func TestPRMergeNumberCanonical(t *testing.T) { + sch := string(tools.NewPRMerge(gitea.NewClient("http://unused", ""), allowlist.New([]string{"owner"})).Descriptor().InputSchema) + assert.Contains(t, sch, `"number":`, "pr_merge must advertise number") + assert.NotContains(t, sch, `"index":`, "pr_merge must not advertise index") + + for _, args := range []string{ + `{"owner":"owner","repo":"repo","number":7}`, + `{"owner":"owner","repo":"repo","index":7}`, + } { + var gotPath string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotPath = r.URL.Path + w.WriteHeader(http.StatusNoContent) + })) + tool := tools.NewPRMerge(gitea.NewClient(srv.URL, "tok"), allowlist.New([]string{"owner"})) + _, err := tool.Call(context.Background(), json.RawMessage(args)) + require.NoError(t, err, args) + assert.Equal(t, "/api/v1/repos/owner/repo/pulls/7/merge", gotPath, args) + srv.Close() + } +} + func TestPRMergeAllowlistRejects(t *testing.T) { tool := tools.NewPRMerge(gitea.NewClient("http://unused", ""), allowlist.New([]string{"allowed"})) _, err := tool.Call(context.Background(), json.RawMessage(`{"owner":"evil","name":"repo","index":1}`))