Simplify codebase (#195)

Net **-650 LOC** by removing duplication and dead noise. All tests pass.

### Duplication & helpers
- Extracted shared slim helpers (`UserLogin`, `UserLogins`, `LabelNames`, `BodyWithAttachments`, `UserDetail`, `Repo`/`Repos`, `Label`/`Labels`) into `pkg/slim`. Deleted the 4 copies that lived in `issue/`, `pull/`, `search/`, `repo/` (plus duplicate `slimUserDetail`/`slimRepo`/`slimLabels` across packages).
- Added `params.GetOptionalBoolPtr` and `params.GetOptionalStringPtr`. Replaced 18 awkward `new(localVar); if !ok { = nil }` patterns across `repo/`, `pull/`, `issue/`, `label/`, `milestone/`, `search/`.
- Extracted `pullRequestReviewerFn` for the 99%-identical `createPullRequestReviewerFn`/`deletePullRequestReviewerFn` pair.

### Dead code & noise
- Deleted **122** `log.Debugf("Called X")` narration lines (`zap.AddCaller` already records the caller) and pruned 19 unused `log` imports.
- Removed the unused `log.Logger` wrapper; the mcp-go server now uses `log.Default().Sugar()` directly (matches `util.Logger`).
- Deleted dead `s.DeleteTools("")` — confirmed no-op in mcp-go.
- Stripped WHAT-narration comments per project guidance.

### Correctness & consistency
- Fixed `log.Errorf(err.Error())` format-string bug in `pkg/to/to.go` — a `%` in the error would have been interpreted as a directive.
- Standardized `to.TextResult`/`to.ErrorResult` usage; `release.go`, `tag.go`, `branch.go` were bypassing the helpers in 9 sites (skipping the wrapper's debug/error logging).
- Made `params.GetString` reject empty strings; dropped 21 redundant `err != nil || x == ""` checks in `operation/actions/`.
- Replaced raw `args["org"].(string)` in `ListOrgReposFn` with `params.GetString` to match the rest of the codebase.

### Performance
- **Cached `*gitea.Client` by host+token via `sync.Map`** + shared `*http.Transport` via `sync.Once` for both SDK and raw REST paths. Eliminates the SDK's `/api/v1/version` preflight on every tool call and enables connection keep-alive across requests.
- Gated `to.TextResult` debug log behind `flag.Debug` to skip the `string(bytes)` allocation when debug is off.
- Hoisted `8192` and `60s` magic numbers in `pkg/gitea/rest.go` into named constants.

---
This PR was written with the help of Claude Opus 4.7

---------

Co-authored-by: silverwind <silv3rwind@gmail.com>
Reviewed-on: https://gitea.com/gitea/gitea-mcp/pulls/195
Reviewed-by: Lunny Xiao <xiaolunwen@gmail.com>
Co-authored-by: silverwind <me@silverwind.io>
Co-committed-by: silverwind <me@silverwind.io>
This commit is contained in:
silverwind
2026-05-19 08:25:36 +00:00
committed by silverwind
parent e36137f5a1
commit 371a06403a
40 changed files with 640 additions and 1007 deletions
-4
View File
@@ -1,21 +1,17 @@
// Package annotation provides shared MCP tool annotation helpers.
package annotation
import "github.com/mark3labs/mcp-go/mcp"
// ReadOnly returns a ToolAnnotation for read-only tools.
func ReadOnly(title string) mcp.ToolAnnotation {
t := true
return mcp.ToolAnnotation{Title: title, ReadOnlyHint: &t}
}
// Write returns a ToolAnnotation for write tools.
func Write(title string) mcp.ToolAnnotation {
f := false
return mcp.ToolAnnotation{Title: title, ReadOnlyHint: &f}
}
// Destructive returns a ToolAnnotation for destructive write tools.
func Destructive(title string) mcp.ToolAnnotation {
f, t := false, true
return mcp.ToolAnnotation{Title: title, ReadOnlyHint: &f, DestructiveHint: &t}
+31 -12
View File
@@ -6,6 +6,7 @@ import (
"errors"
"fmt"
"net/http"
"sync"
mcpContext "gitea.com/gitea/gitea-mcp/pkg/context"
"gitea.com/gitea/gitea-mcp/pkg/flag"
@@ -13,21 +14,39 @@ import (
"code.gitea.io/sdk/gitea"
)
var (
clientCache sync.Map // token -> *gitea.Client
sharedTransOnce sync.Once
sharedTrans *http.Transport
)
func sharedTransport() *http.Transport {
sharedTransOnce.Do(func() {
sharedTrans = http.DefaultTransport.(*http.Transport).Clone()
if flag.Insecure {
sharedTrans.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} //nolint:gosec // user-requested insecure mode
}
})
return sharedTrans
}
// NewClient returns a cached *gitea.Client keyed by host+token. The SDK's per-client
// version cache and the shared transport let us reuse keep-alive connections
// and avoid the SDK's /api/v1/version preflight on every tool call.
func NewClient(token string) (*gitea.Client, error) {
httpClient := &http.Client{
Transport: http.DefaultTransport,
CheckRedirect: checkRedirect,
key := flag.Host + "\x00" + token
if v, ok := clientCache.Load(key); ok {
return v.(*gitea.Client), nil
}
httpClient := &http.Client{
Transport: sharedTransport(),
CheckRedirect: checkRedirect,
}
opts := []gitea.ClientOption{
gitea.SetToken(token),
gitea.SetHTTPClient(httpClient),
}
if flag.Insecure {
httpClient.Transport.(*http.Transport).TLSClientConfig = &tls.Config{
InsecureSkipVerify: true,
}
}
opts = append(opts, gitea.SetHTTPClient(httpClient))
if flag.Debug {
opts = append(opts, gitea.SetDebugMode())
}
@@ -35,10 +54,10 @@ func NewClient(token string) (*gitea.Client, error) {
if err != nil {
return nil, fmt.Errorf("create gitea client err: %w", err)
}
// Set user agent for the client
client.SetUserAgent("gitea-mcp-server/" + flag.Version)
return client, nil
actual, _ := clientCache.LoadOrStore(key, client)
return actual.(*gitea.Client), nil
}
// checkRedirect prevents Go from silently changing mutating requests (POST, PATCH, etc.)
+25 -16
View File
@@ -3,7 +3,6 @@ package gitea
import (
"bytes"
"context"
"crypto/tls"
"encoding/json"
"errors"
"fmt"
@@ -11,12 +10,18 @@ import (
"net/http"
"net/url"
"strings"
"sync"
"time"
mcpContext "gitea.com/gitea/gitea-mcp/pkg/context"
"gitea.com/gitea/gitea-mcp/pkg/flag"
)
const (
httpClientTimeout = 60 * time.Second
errBodySnippetSize = 8192
)
type HTTPError struct {
StatusCode int
Body string
@@ -38,16 +43,20 @@ func tokenFromContext(ctx context.Context) string {
return flag.Token
}
func newRESTHTTPClient() *http.Client {
transport := http.DefaultTransport.(*http.Transport).Clone()
if flag.Insecure {
transport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} //nolint:gosec // user-requested insecure mode
}
return &http.Client{
Transport: transport,
Timeout: 60 * time.Second,
CheckRedirect: checkRedirect,
}
var (
restClientOnce sync.Once
restClient *http.Client
)
func restHTTPClient() *http.Client {
restClientOnce.Do(func() {
restClient = &http.Client{
Transport: sharedTransport(),
Timeout: httpClientTimeout,
CheckRedirect: checkRedirect,
}
})
return restClient
}
func buildAPIURL(path string, query url.Values) (string, error) {
@@ -96,7 +105,7 @@ func DoJSON(ctx context.Context, method, path string, query url.Values, body, re
req.Header.Set("Content-Type", "application/json")
}
client := newRESTHTTPClient()
client := restHTTPClient()
resp, err := client.Do(req)
if err != nil {
return 0, fmt.Errorf("do request: %w", err)
@@ -104,7 +113,7 @@ func DoJSON(ctx context.Context, method, path string, query url.Values, body, re
defer resp.Body.Close()
if resp.StatusCode < 200 || resp.StatusCode >= 300 {
bodySnippet, _ := io.ReadAll(io.LimitReader(resp.Body, 8192))
bodySnippet, _ := io.ReadAll(io.LimitReader(resp.Body, errBodySnippetSize))
return resp.StatusCode, &HTTPError{StatusCode: resp.StatusCode, Body: strings.TrimSpace(string(bodySnippet))}
}
@@ -151,7 +160,7 @@ func DoBytes(ctx context.Context, method, path string, query url.Values, body an
req.Header.Set("Content-Type", "application/json")
}
client := newRESTHTTPClient()
client := restHTTPClient()
resp, err := client.Do(req)
if err != nil {
return nil, 0, fmt.Errorf("do request: %w", err)
@@ -165,8 +174,8 @@ func DoBytes(ctx context.Context, method, path string, query url.Values, body an
if resp.StatusCode < 200 || resp.StatusCode >= 300 {
bodySnippet := respBytes
if len(bodySnippet) > 8192 {
bodySnippet = bodySnippet[:8192]
if len(bodySnippet) > errBodySnippetSize {
bodySnippet = bodySnippet[:errBodySnippetSize]
}
return nil, resp.StatusCode, &HTTPError{StatusCode: resp.StatusCode, Body: strings.TrimSpace(string(bodySnippet))}
}
-18
View File
@@ -79,24 +79,6 @@ func SetDefault(logger *zap.Logger) {
}
}
func New() *Logger {
return &Logger{
defaultLogger: Default(),
}
}
type Logger struct {
defaultLogger *zap.Logger
}
func (l *Logger) Infof(msg string, args ...any) {
l.defaultLogger.Sugar().Infof(msg, args...)
}
func (l *Logger) Errorf(msg string, args ...any) {
l.defaultLogger.Sugar().Errorf(msg, args...)
}
func Debug(msg string, fields ...zap.Field) {
Default().Debug(msg, fields...)
}
+33 -16
View File
@@ -16,16 +16,15 @@ const (
PaginationDesc = "results per page"
)
// GetString extracts a required string parameter from MCP tool arguments.
// GetString extracts a required string parameter. Empty strings are treated as missing.
func GetString(args map[string]any, key string) (string, error) {
val, ok := args[key].(string)
if !ok {
if !ok || val == "" {
return "", fmt.Errorf("%s is required", key)
}
return val, nil
}
// GetOptionalString extracts an optional string parameter with a default value.
func GetOptionalString(args map[string]any, key, defaultVal string) string {
if val, ok := args[key].(string); ok {
return val
@@ -33,7 +32,6 @@ func GetOptionalString(args map[string]any, key, defaultVal string) string {
return defaultVal
}
// GetStringSlice extracts an optional string slice parameter from MCP tool arguments.
func GetStringSlice(args map[string]any, key string) []string {
val, ok := args[key]
if !ok {
@@ -52,13 +50,11 @@ func GetStringSlice(args map[string]any, key string) []string {
return out
}
// GetPagination extracts page and per_page parameters, returning them as ints.
func GetPagination(args map[string]any, defaultPageSize int64) (page, pageSize int) {
return int(GetOptionalInt(args, "page", 1)), int(GetOptionalInt(args, "per_page", defaultPageSize))
}
// ToInt64 converts a value to int64, accepting both float64 (JSON number) and
// string representations. Returns false if the value cannot be converted.
// ToInt64 accepts float64 (JSON number) and string representations.
func ToInt64(val any) (int64, bool) {
switch v := val.(type) {
case float64:
@@ -74,10 +70,8 @@ func ToInt64(val any) (int64, bool) {
}
}
// GetIndex extracts a required integer parameter from MCP tool arguments.
// It accepts both numeric (float64 from JSON) and string representations.
// This provides better UX for LLM callers that may naturally use strings
// for identifiers like issue/PR numbers.
// GetIndex extracts a required integer. Accepts numeric or string forms — LLM callers
// often pass identifiers like issue/PR numbers as strings.
func GetIndex(args map[string]any, key string) (int64, error) {
val, exists := args[key]
if !exists {
@@ -95,7 +89,6 @@ func GetIndex(args map[string]any, key string) (int64, error) {
return 0, fmt.Errorf("%s must be a number or numeric string", key)
}
// GetInt64Slice extracts a required int64 slice parameter from MCP tool arguments.
func GetInt64Slice(args map[string]any, key string) ([]int64, error) {
raw, ok := args[key].([]any)
if !ok {
@@ -112,7 +105,7 @@ func GetInt64Slice(args map[string]any, key string) ([]int64, error) {
return out, nil
}
// GetOptionalTime extracts an optional RFC3339 timestamp parameter, returning nil if missing or unparseable.
// GetOptionalTime parses RFC3339, returning nil if missing or unparseable.
func GetOptionalTime(args map[string]any, key string) *time.Time {
val, ok := args[key].(string)
if !ok {
@@ -124,9 +117,6 @@ func GetOptionalTime(args map[string]any, key string) *time.Time {
return nil
}
// GetOptionalInt extracts an optional integer parameter from MCP tool arguments.
// Returns defaultVal if the key is missing or the value cannot be parsed.
// Accepts both float64 (JSON number) and string representations.
func GetOptionalInt(args map[string]any, key string, defaultVal int64) int64 {
val, exists := args[key]
if !exists {
@@ -137,3 +127,30 @@ func GetOptionalInt(args map[string]any, key string, defaultVal int64) int64 {
}
return defaultVal
}
// GetOptionalBoolPtr is for SDK fields where nil/false/true are distinct (e.g. "no change" vs "set to false").
func GetOptionalBoolPtr(args map[string]any, key string) *bool {
if v, ok := args[key].(bool); ok {
return &v
}
return nil
}
// GetOptionalStringPtr returns nil when the key is missing OR the value is an empty string.
// Use this for create/fork-style fields where "" is meaningless (e.g. fork target name).
func GetOptionalStringPtr(args map[string]any, key string) *string {
if v, ok := args[key].(string); ok && v != "" {
return &v
}
return nil
}
// GetPresentStringPtr returns &v whenever the key is present as a string, including "".
// Use this for PATCH-style fields where the SDK distinguishes "no change" (nil) from
// "set to empty" (&""), e.g. clearing an issue body or label description.
func GetPresentStringPtr(args map[string]any, key string) *string {
if v, ok := args[key].(string); ok {
return &v
}
return nil
}
+36
View File
@@ -73,6 +73,42 @@ func TestGetOptionalInt(t *testing.T) {
}
}
func TestGetOptionalStringPtr(t *testing.T) {
if p := GetOptionalStringPtr(map[string]any{}, "k"); p != nil {
t.Errorf("missing key: got %v, want nil", p)
}
if p := GetOptionalStringPtr(map[string]any{"k": ""}, "k"); p != nil {
t.Errorf("empty string: got %v, want nil", p)
}
if p := GetOptionalStringPtr(map[string]any{"k": 42}, "k"); p != nil {
t.Errorf("non-string: got %v, want nil", p)
}
if p := GetOptionalStringPtr(map[string]any{"k": nil}, "k"); p != nil {
t.Errorf("nil value (JSON null): got %v, want nil", p)
}
if p := GetOptionalStringPtr(map[string]any{"k": "x"}, "k"); p == nil || *p != "x" {
t.Errorf("non-empty: got %v, want &\"x\"", p)
}
}
func TestGetPresentStringPtr(t *testing.T) {
if p := GetPresentStringPtr(map[string]any{}, "k"); p != nil {
t.Errorf("missing key: got %v, want nil", p)
}
if p := GetPresentStringPtr(map[string]any{"k": 42}, "k"); p != nil {
t.Errorf("non-string: got %v, want nil", p)
}
if p := GetPresentStringPtr(map[string]any{"k": nil}, "k"); p != nil {
t.Errorf("nil value (JSON null): got %v, want nil", p)
}
if p := GetPresentStringPtr(map[string]any{"k": ""}, "k"); p == nil || *p != "" {
t.Errorf("empty string: got %v, want &\"\"", p)
}
if p := GetPresentStringPtr(map[string]any{"k": "x"}, "k"); p == nil || *p != "x" {
t.Errorf("non-empty: got %v, want &\"x\"", p)
}
}
func TestGetIndex(t *testing.T) {
tests := []struct {
name string
+135
View File
@@ -0,0 +1,135 @@
package slim
import (
"fmt"
"strings"
gitea_sdk "code.gitea.io/sdk/gitea"
)
func UserLogin(u *gitea_sdk.User) string {
if u == nil {
return ""
}
return u.UserName
}
func UserLogins(users []*gitea_sdk.User) []string {
if len(users) == 0 {
return nil
}
out := make([]string, 0, len(users))
for _, u := range users {
if u != nil {
out = append(out, u.UserName)
}
}
return out
}
func LabelNames(labels []*gitea_sdk.Label) []string {
if len(labels) == 0 {
return nil
}
out := make([]string, 0, len(labels))
for _, l := range labels {
if l != nil {
out = append(out, l.Name)
}
}
return out
}
func BodyWithAttachments(body string, atts []*gitea_sdk.Attachment) string {
links := make([]string, 0, len(atts))
for _, a := range atts {
if a == nil || a.DownloadURL == "" {
continue
}
links = append(links, fmt.Sprintf("[%s](%s)", a.Name, a.DownloadURL))
}
if len(links) == 0 {
return body
}
joined := strings.Join(links, "\n")
if body == "" {
return joined
}
return body + "\n\n" + joined
}
func UserDetail(u *gitea_sdk.User) map[string]any {
if u == nil {
return nil
}
return map[string]any{
"id": u.ID,
"login": u.UserName,
"full_name": u.FullName,
"email": u.Email,
"avatar_url": u.AvatarURL,
"html_url": u.HTMLURL,
"is_admin": u.IsAdmin,
}
}
func Repo(r *gitea_sdk.Repository) map[string]any {
if r == nil {
return nil
}
m := map[string]any{
"id": r.ID,
"full_name": r.FullName,
"description": r.Description,
"html_url": r.HTMLURL,
"clone_url": r.CloneURL,
"ssh_url": r.SSHURL,
"default_branch": r.DefaultBranch,
"private": r.Private,
"fork": r.Fork,
"archived": r.Archived,
"language": r.Language,
"stars_count": r.Stars,
"forks_count": r.Forks,
"open_issues_count": r.OpenIssues,
"open_pr_counter": r.OpenPulls,
"created_at": r.Created,
"updated_at": r.Updated,
}
if r.Owner != nil {
m["owner"] = r.Owner.UserName
}
if len(r.Topics) > 0 {
m["topics"] = r.Topics
}
return m
}
func Repos(repos []*gitea_sdk.Repository) []map[string]any {
out := make([]map[string]any, 0, len(repos))
for _, r := range repos {
out = append(out, Repo(r))
}
return out
}
func Label(l *gitea_sdk.Label) map[string]any {
if l == nil {
return nil
}
return map[string]any{
"id": l.ID,
"name": l.Name,
"color": l.Color,
"description": l.Description,
"exclusive": l.Exclusive,
}
}
func Labels(labels []*gitea_sdk.Label) []map[string]any {
out := make([]map[string]any, 0, len(labels))
for _, l := range labels {
out = append(out, Label(l))
}
return out
}
+110
View File
@@ -0,0 +1,110 @@
package slim
import (
"testing"
gitea_sdk "code.gitea.io/sdk/gitea"
)
func TestUserDetail(t *testing.T) {
u := &gitea_sdk.User{
ID: 42,
UserName: "alice",
FullName: "Alice Smith",
Email: "alice@example.com",
AvatarURL: "https://gitea.com/avatars/42",
HTMLURL: "https://gitea.com/alice",
IsAdmin: true,
}
m := UserDetail(u)
if m["id"] != int64(42) {
t.Errorf("expected id 42, got %v", m["id"])
}
if m["login"] != "alice" {
t.Errorf("expected login alice, got %v", m["login"])
}
if m["full_name"] != "Alice Smith" {
t.Errorf("expected full_name Alice Smith, got %v", m["full_name"])
}
if m["is_admin"] != true {
t.Errorf("expected is_admin true, got %v", m["is_admin"])
}
}
func TestUserDetail_Nil(t *testing.T) {
if m := UserDetail(nil); m != nil {
t.Errorf("expected nil for nil user, got %v", m)
}
}
func TestLabel(t *testing.T) {
l := &gitea_sdk.Label{
ID: 1,
Name: "bug",
Color: "#d73a4a",
Description: "Something isn't working",
Exclusive: false,
}
m := Label(l)
if m["name"] != "bug" {
t.Errorf("expected name bug, got %v", m["name"])
}
if m["color"] != "#d73a4a" {
t.Errorf("expected color, got %v", m["color"])
}
}
func TestRepo(t *testing.T) {
r := &gitea_sdk.Repository{
ID: 1,
FullName: "org/repo",
Description: "A test repo",
HTMLURL: "https://gitea.com/org/repo",
CloneURL: "https://gitea.com/org/repo.git",
SSHURL: "git@gitea.com:org/repo.git",
DefaultBranch: "main",
Language: "Go",
Stars: 10,
Forks: 2,
Owner: &gitea_sdk.User{UserName: "org"},
Topics: []string{"mcp", "gitea"},
}
m := Repo(r)
if m["full_name"] != "org/repo" {
t.Errorf("expected full_name org/repo, got %v", m["full_name"])
}
if m["owner"] != "org" {
t.Errorf("expected owner org, got %v", m["owner"])
}
topics := m["topics"].([]string)
if len(topics) != 2 {
t.Errorf("expected 2 topics, got %d", len(topics))
}
}
func TestBodyWithAttachments(t *testing.T) {
atts := []*gitea_sdk.Attachment{
{Name: "shot.png", DownloadURL: "https://example/shot.png"},
{Name: "log.txt", DownloadURL: "https://example/log.txt"},
}
got := BodyWithAttachments("see attached", atts)
want := "see attached\n\n[shot.png](https://example/shot.png)\n[log.txt](https://example/log.txt)"
if got != want {
t.Errorf("got %q, want %q", got, want)
}
if got := BodyWithAttachments("only body", nil); got != "only body" {
t.Errorf("nil attachments should return body unchanged, got %q", got)
}
if got := BodyWithAttachments("", atts); got != "[shot.png](https://example/shot.png)\n[log.txt](https://example/log.txt)" {
t.Errorf("empty body should drop separator, got %q", got)
}
skipped := []*gitea_sdk.Attachment{nil, {Name: "noop", DownloadURL: ""}}
if got := BodyWithAttachments("body", skipped); got != "body" {
t.Errorf("nil/empty-URL attachments should be skipped, got %q", got)
}
}
+5 -2
View File
@@ -4,6 +4,7 @@ import (
"encoding/json"
"fmt"
"gitea.com/gitea/gitea-mcp/pkg/flag"
"gitea.com/gitea/gitea-mcp/pkg/log"
"github.com/mark3labs/mcp-go/mcp"
@@ -14,11 +15,13 @@ func TextResult(v any) (*mcp.CallToolResult, error) {
if err != nil {
return nil, fmt.Errorf("marshal result err: %v", err)
}
log.Debugf("Text Result: %s", string(resultBytes))
if flag.Debug {
log.Debugf("Text Result: %s", string(resultBytes))
}
return mcp.NewToolResultText(string(resultBytes)), nil
}
func ErrorResult(err error) (*mcp.CallToolResult, error) {
log.Errorf(err.Error())
log.Errorf("%s", err.Error())
return nil, err
}