diff --git a/README.md b/README.md index f3fb335..1250f17 100644 --- a/README.md +++ b/README.md @@ -170,6 +170,9 @@ sting --author mfacenet --scope repos --repos skaphos/sting --window 7d --diffs ``` Run `sting --help` (or `sting --help`) for the full flag list. +Queries return at most 100 commits by default to avoid routine rate-limit +pressure; pass `--max-commits 0` only when you intentionally want an exhaustive +scan. ### Evidence depth @@ -192,7 +195,9 @@ flags when you want an agent to explain the actual code changes: GitHub fetches this evidence from per-commit detail calls. GitLab uses `with_stats` for line stats and commit diff calls for file evidence. Keep full diffs explicit because they cost extra API calls and can be token-heavy for an -LLM context. +LLM context. The MCP `get_commits` tool therefore treats an omitted +`include_diffs` argument as `false`, even when the server config enables diffs; +callers must explicitly pass `include_diffs: true` to request patch text. ### Scopes @@ -272,7 +277,7 @@ directory, or pointed at explicitly with `--config path.yaml`. | `gitlab_token` | `STING_GITLAB_TOKEN` | `--gitlab-token` | — | dedicated GitLab PAT | | `gitlab_base_url` | `STING_GITLAB_BASE_URL` | `--gitlab-base-url` | GitLab.com | GitLab API v4 root | | `per_page` | `STING_PER_PAGE` | `--per-page` | `100` | API page size (1–100) | -| `max_commits` | `STING_MAX_COMMITS` | `--max-commits` | `0` | cap on returned commits (0 = unlimited) | +| `max_commits` | `STING_MAX_COMMITS` | `--max-commits` | `100` | cap on returned commits (0 = unlimited) | | `default_scope` | `STING_DEFAULT_SCOPE` | (`--scope`) | `search` | scope when unspecified | | `default_window` | `STING_DEFAULT_WINDOW` | (`--window`) | `7d` | look-back when `since` unspecified | | `default_repos` | `STING_DEFAULT_REPOS` | (`--repos`) | — | repo/project list for `repos` scope | diff --git a/config.example.yaml b/config.example.yaml index 73ffe40..7269a8f 100644 --- a/config.example.yaml +++ b/config.example.yaml @@ -34,7 +34,7 @@ default_org: "" # org/group used by scope=org default_format: markdown # markdown | json per_page: 100 # API page size (1-100) -max_commits: 0 # cap on returned commits (0 = unlimited) +max_commits: 100 # cap on returned commits (0 = unlimited) include_stats: false # fetch per-commit additions/deletions include_files: false # fetch per-file change summaries include_diffs: false # fetch bounded patch text; implies include_files diff --git a/config/config.go b/config/config.go index c6fe3bb..b051ea8 100644 --- a/config/config.go +++ b/config/config.go @@ -14,6 +14,10 @@ import ( "github.com/skaphos/sting/model" ) +// DefaultMaxCommits bounds routine queries to the providers' maximum page size. +// Users can still set max_commits to 0 for an exhaustive scan. +const DefaultMaxCommits = 100 + // Config holds all tunable settings. The mapstructure keys are the canonical // configuration keys: they are the YAML/JSON config-file keys, the viper keys // bound to flags, and (uppercased, STING_-prefixed) the environment variables. @@ -77,7 +81,7 @@ func Default() Config { DefaultRepos: []string{}, DefaultFormat: "markdown", PerPage: 100, - MaxCommits: 0, + MaxCommits: DefaultMaxCommits, IncludeStats: false, IncludeFiles: false, IncludeDiffs: false, @@ -126,6 +130,9 @@ func (cfg Config) Validate() error { if cfg.PerPage < 1 || cfg.PerPage > 100 { return fmt.Errorf("per_page must be 1-100, got %d", cfg.PerPage) } + if cfg.MaxCommits < 0 { + return fmt.Errorf("max_commits must be >= 0, got %d", cfg.MaxCommits) + } if cfg.MaxDiffBytes < 0 { return fmt.Errorf("max_diff_bytes must be >= 0, got %d", cfg.MaxDiffBytes) } diff --git a/config/config_test.go b/config/config_test.go index cb118b4..0ca75b7 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -116,3 +116,11 @@ func TestValidateGitLabSearchScopeIncompatible(t *testing.T) { t.Errorf("Validate: unexpected error for provider=gitlab, default_scope=repos: %v", err) } } + +func TestValidateMaxCommits(t *testing.T) { + cfg := Default() + cfg.MaxCommits = -1 + if err := cfg.Validate(); err == nil { + t.Fatal("Validate: want error for negative max commits") + } +} diff --git a/config/resolve.go b/config/resolve.go index 9deb727..cc91387 100644 --- a/config/resolve.go +++ b/config/resolve.go @@ -46,6 +46,8 @@ type Request struct { IncludeDiffs *bool // MaxDiffBytes overrides the default when non-nil. MaxDiffBytes *int + // MaxCommits overrides the default when non-nil. + MaxCommits *int // IncludePullRequests overrides the default when non-nil. IncludePullRequests *bool } @@ -156,6 +158,13 @@ func (cfg Config) Resolve(req Request, now time.Time) (model.Query, error) { if maxDiffBytes < 0 { return model.Query{}, fmt.Errorf("max_diff_bytes must be >= 0, got %d", maxDiffBytes) } + maxCommits := cfg.MaxCommits + if req.MaxCommits != nil { + maxCommits = *req.MaxCommits + } + if maxCommits < 0 { + return model.Query{}, fmt.Errorf("max_commits must be >= 0, got %d", maxCommits) + } includePRs := cfg.IncludePullRequests if req.IncludePullRequests != nil { @@ -174,7 +183,7 @@ func (cfg Config) Resolve(req Request, now time.Time) (model.Query, error) { IncludeFiles: includeFiles, IncludeDiffs: includeDiffs, MaxDiffBytes: maxDiffBytes, - MaxCommits: cfg.MaxCommits, + MaxCommits: maxCommits, IncludePullRequests: includePRs, }, nil } diff --git a/config/resolve_test.go b/config/resolve_test.go index 698dd7e..5aad694 100644 --- a/config/resolve_test.go +++ b/config/resolve_test.go @@ -25,6 +25,9 @@ func TestResolveDefaults(t *testing.T) { if q.Scope != model.ScopeSearch { t.Errorf("scope = %q, want default search", q.Scope) } + if q.MaxCommits != DefaultMaxCommits { + t.Errorf("MaxCommits = %d, want default %d", q.MaxCommits, DefaultMaxCommits) + } if !q.Until.Equal(now) { t.Errorf("until = %v, want now %v", q.Until, now) } @@ -215,3 +218,20 @@ func TestResolveDiffsImplyFiles(t *testing.T) { t.Errorf("MaxDiffBytes = %d, want 1234", q.MaxDiffBytes) } } + +func TestResolveMaxCommitsOverride(t *testing.T) { + cfg := Default() + max := 0 + q, err := cfg.Resolve(Request{Author: "x", MaxCommits: &max}, time.Now()) + if err != nil { + t.Fatalf("Resolve: %v", err) + } + if q.MaxCommits != 0 { + t.Errorf("MaxCommits = %d, want explicit unlimited 0", q.MaxCommits) + } + + negative := -1 + if _, err := cfg.Resolve(Request{Author: "x", MaxCommits: &negative}, time.Now()); err == nil { + t.Fatal("Resolve: want error for negative max commits") + } +} diff --git a/ghclient/collect_test.go b/ghclient/collect_test.go index d6d76d6..a09e9e1 100644 --- a/ghclient/collect_test.go +++ b/ghclient/collect_test.go @@ -207,6 +207,48 @@ func TestCollectScopeOrg(t *testing.T) { } } +func TestCollectScopeOrgStopsRepoPaginationAtMaxCommits(t *testing.T) { + const firstPage = `[{"full_name":"skaphos/sting"}]` + const twoCommits = `[ + {"sha":"one","commit":{"message":"first","author":{"date":"2026-05-21T11:00:00Z"}}}, + {"sha":"two","commit":{"message":"second","author":{"date":"2026-05-21T12:00:00Z"}}} + ]` + mux := http.NewServeMux() + mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + switch { + case strings.Contains(r.URL.Path, "/orgs/skaphos/repos"): + if r.URL.Query().Get("page") == "2" { + t.Errorf("org repo pagination should stop once MaxCommits is reached") + } + w.Header().Set("Link", `; rel="next"`) + _, _ = w.Write([]byte(firstPage)) + case strings.Contains(r.URL.Path, "/repos/skaphos/sting/commits"): + _, _ = w.Write([]byte(twoCommits)) + default: + t.Errorf("unexpected path %q", r.URL.Path) + } + }) + srv := httptest.NewServer(mux) + defer srv.Close() + + c := newTestClient(t, srv.URL, 50) + res, err := c.Collect(context.Background(), model.Query{ + Author: "octocat", + Scope: model.ScopeOrg, + Org: "skaphos", + MaxCommits: 1, + }) + if err != nil { + t.Fatalf("Collect: %v", err) + } + if res.Count != 1 { + t.Fatalf("Count = %d, want 1", res.Count) + } + if !res.Truncated { + t.Error("Truncated = false, want true") + } +} + // TestCollectScopeOrgSkipsBadRepo verifies that an org scan does not abort when // one repo cannot be listed (here an empty repo returning 409): the bad repo is // recorded in Result.Skipped and enumeration continues to the healthy repo. diff --git a/ghclient/ghclient.go b/ghclient/ghclient.go index 66f7ac5..b8c62c8 100644 --- a/ghclient/ghclient.go +++ b/ghclient/ghclient.go @@ -226,7 +226,7 @@ func (c *Client) searchByAuthor(ctx context.Context, q model.Query) ([]model.Com opts := &github.SearchOptions{ Sort: "author-date", Order: "desc", - ListOptions: github.ListOptions{PerPage: c.perPage}, + ListOptions: github.ListOptions{PerPage: c.resultPageSize(q.MaxCommits)}, } var out []model.Commit for page := 1; ; page++ { @@ -265,7 +265,11 @@ func (c *Client) listRepos(ctx context.Context, q model.Query) ([]model.Commit, if !ok { return nil, fmt.Errorf("invalid repo %q (want owner/repo)", target) } - commits, err := c.collectRepo(ctx, owner, repo, q) + repoQuery := remainingQuery(q, len(out)) + if repoQuery.MaxCommits == 0 && q.MaxCommits > 0 { + return out, nil + } + commits, err := c.collectRepo(ctx, owner, repo, repoQuery) if err != nil { return nil, err } @@ -287,31 +291,48 @@ func (c *Client) listOrg(ctx context.Context, q model.Query) ([]model.Commit, [] if q.Org == "" { return nil, nil, fmt.Errorf("scope %q requires an org", model.ScopeOrg) } - repos, err := c.orgRepos(ctx, q.Org) - if err != nil { - return nil, nil, err + opts := &github.RepositoryListByOrgOptions{ + ListOptions: github.ListOptions{PerPage: c.perPage}, } var ( out []model.Commit skipped []model.SkippedRepo ) - for _, full := range repos { - owner, repo, ok := splitRepo(full) - if !ok { - continue + for page := 1; ; page++ { + if page > maxPages { + return nil, nil, fmt.Errorf("list org repos %s: exceeded max pages (%d)", q.Org, maxPages) } - commits, err := c.collectRepo(ctx, owner, repo, q) + repos, resp, err := c.gh.Repositories.ListByOrg(ctx, q.Org, opts) if err != nil { - if reason, skip := skipRepoReason(err); skip { - skipped = append(skipped, model.SkippedRepo{Repo: full, Reason: reason}) + return nil, nil, apiError("list org repos "+q.Org, err) + } + for _, r := range repos { + full := r.GetFullName() + owner, repo, ok := splitRepo(full) + if !ok { continue } - return nil, nil, err + repoQuery := remainingQuery(q, len(out)) + if repoQuery.MaxCommits == 0 && q.MaxCommits > 0 { + return out, skipped, nil + } + commits, err := c.collectRepo(ctx, owner, repo, repoQuery) + if err != nil { + if reason, skip := skipRepoReason(err); skip { + skipped = append(skipped, model.SkippedRepo{Repo: full, Reason: reason}) + continue + } + return nil, nil, err + } + out = append(out, commits...) + if q.MaxCommits > 0 && len(out) >= q.MaxCommits { + return out, skipped, nil + } } - out = append(out, commits...) - if q.MaxCommits > 0 && len(out) >= q.MaxCommits { + if resp.NextPage == 0 { break } + opts.Page = resp.NextPage } return out, skipped, nil } @@ -358,7 +379,7 @@ func (c *Client) listRepoCommits(ctx context.Context, owner, repo string, q mode Author: q.Author, Since: q.Since, Until: q.Until, - ListOptions: github.ListOptions{PerPage: c.perPage}, + ListOptions: github.ListOptions{PerPage: c.resultPageSize(q.MaxCommits)}, } full := owner + "/" + repo var out []model.Commit @@ -431,7 +452,7 @@ func (c *Client) pullRequestCommits(ctx context.Context, owner, repo string, q m func (c *Client) prBranchCommits(ctx context.Context, owner, repo string, number int, q model.Query, seen map[string]bool) ([]model.Commit, error) { full := owner + "/" + repo source := fmt.Sprintf("pull/%d", number) - opts := &github.ListOptions{PerPage: c.perPage} + opts := &github.ListOptions{PerPage: c.resultPageSize(q.MaxCommits)} var out []model.Commit for page := 1; ; page++ { if page > maxPages { @@ -504,32 +525,27 @@ func inWindow(t, since, until time.Time) bool { return true } -func (c *Client) orgRepos(ctx context.Context, org string) ([]string, error) { - opts := &github.RepositoryListByOrgOptions{ - ListOptions: github.ListOptions{PerPage: c.perPage}, - } - var out []string - for page := 1; ; page++ { - if page > maxPages { - return nil, fmt.Errorf("list org repos %s: exceeded max pages (%d)", org, maxPages) - } - repos, resp, err := c.gh.Repositories.ListByOrg(ctx, org, opts) - if err != nil { - return nil, apiError("list org repos "+org, err) - } - for _, r := range repos { - out = append(out, r.GetFullName()) - } - if resp.NextPage == 0 { - break - } - opts.Page = resp.NextPage +func needsDetail(q model.Query) bool { + return q.IncludeStats || q.IncludeFiles || q.IncludeDiffs +} + +func (c *Client) resultPageSize(maxCommits int) int { + if maxCommits > 0 && maxCommits < c.perPage { + return maxCommits } - return out, nil + return c.perPage } -func needsDetail(q model.Query) bool { - return q.IncludeStats || q.IncludeFiles || q.IncludeDiffs +func remainingQuery(q model.Query, have int) model.Query { + if q.MaxCommits <= 0 { + return q + } + remaining := q.MaxCommits - have + if remaining < 0 { + remaining = 0 + } + q.MaxCommits = remaining + return q } // enrichDetails fills each commit's stats/files/diffs concurrently, bounded by diff --git a/gitlabclient/gitlabclient.go b/gitlabclient/gitlabclient.go index ebe17d1..ba87782 100644 --- a/gitlabclient/gitlabclient.go +++ b/gitlabclient/gitlabclient.go @@ -159,7 +159,11 @@ func (c *Client) listRepos(ctx context.Context, q model.Query) ([]model.Commit, if project == "" { return nil, fmt.Errorf("invalid repo %q", target) } - commits, err := c.listProjectCommits(ctx, project, project, q) + projectQuery := remainingQuery(q, len(out)) + if projectQuery.MaxCommits == 0 && q.MaxCommits > 0 { + return out, nil + } + commits, err := c.listProjectCommits(ctx, project, project, projectQuery) if err != nil { return nil, err } @@ -180,30 +184,50 @@ func (c *Client) listGroup(ctx context.Context, q model.Query) ([]model.Commit, if strings.TrimSpace(q.Org) == "" { return nil, nil, fmt.Errorf("scope %q requires an org", model.ScopeOrg) } - projects, err := c.groupProjects(ctx, q.Org) - if err != nil { - return nil, nil, err - } var ( out []model.Commit skipped []model.SkippedRepo ) - for _, project := range projects { - target := strconv.FormatInt(project.ID, 10) - label := project.PathWithNamespace - if label == "" { - label = target + values := url.Values{} + values.Set("include_subgroups", "true") + values.Set("simple", "true") + values.Set("per_page", strconv.Itoa(c.perPage)) + + endpoint := "groups/" + url.PathEscape(strings.TrimSpace(q.Org)) + "/projects" + for page := 1; ; page++ { + if page > maxPages { + return nil, nil, fmt.Errorf("list group projects %s: exceeded max pages (%d)", q.Org, maxPages) } - commits, err := c.listProjectCommits(ctx, target, label, q) + values.Set("page", strconv.Itoa(page)) + var projects []gitlabProject + next, err := c.get(ctx, "list group projects "+q.Org, endpoint, values, &projects) if err != nil { - if reason, skip := skipProjectReason(err); skip { - skipped = append(skipped, model.SkippedRepo{Repo: label, Reason: reason}) - continue - } return nil, nil, err } - out = append(out, commits...) - if q.MaxCommits > 0 && len(out) >= q.MaxCommits { + for _, project := range projects { + target := strconv.FormatInt(project.ID, 10) + label := project.PathWithNamespace + if label == "" { + label = target + } + projectQuery := remainingQuery(q, len(out)) + if projectQuery.MaxCommits == 0 && q.MaxCommits > 0 { + return out, skipped, nil + } + commits, err := c.listProjectCommits(ctx, target, label, projectQuery) + if err != nil { + if reason, skip := skipProjectReason(err); skip { + skipped = append(skipped, model.SkippedRepo{Repo: label, Reason: reason}) + continue + } + return nil, nil, err + } + out = append(out, commits...) + if q.MaxCommits > 0 && len(out) >= q.MaxCommits { + return out, skipped, nil + } + } + if next == "" { break } } @@ -221,7 +245,7 @@ func (c *Client) listProjectCommits(ctx context.Context, project, repoLabel stri values.Set("author", q.Author) values.Set("since", q.Since.UTC().Format(time.RFC3339)) values.Set("until", q.Until.UTC().Format(time.RFC3339)) - values.Set("per_page", strconv.Itoa(c.perPage)) + values.Set("per_page", strconv.Itoa(c.resultPageSize(q.MaxCommits))) if q.IncludeStats { values.Set("with_stats", "true") } @@ -251,32 +275,6 @@ func (c *Client) listProjectCommits(ctx context.Context, project, repoLabel stri return out, nil } -func (c *Client) groupProjects(ctx context.Context, group string) ([]gitlabProject, error) { - values := url.Values{} - values.Set("include_subgroups", "true") - values.Set("simple", "true") - values.Set("per_page", strconv.Itoa(c.perPage)) - - endpoint := "groups/" + url.PathEscape(strings.TrimSpace(group)) + "/projects" - var out []gitlabProject - for page := 1; ; page++ { - if page > maxPages { - return nil, fmt.Errorf("list group projects %s: exceeded max pages (%d)", group, maxPages) - } - values.Set("page", strconv.Itoa(page)) - var projects []gitlabProject - next, err := c.get(ctx, "list group projects "+group, endpoint, values, &projects) - if err != nil { - return nil, err - } - out = append(out, projects...) - if next == "" { - break - } - } - return out, nil -} - func (c *Client) get(ctx context.Context, op, endpoint string, values url.Values, dest any) (string, error) { u := c.baseURL + endpoint if len(values) > 0 { @@ -319,6 +317,25 @@ func (c *Client) get(ctx context.Context, op, endpoint string, values url.Values return resp.Header.Get("X-Next-Page"), nil } +func (c *Client) resultPageSize(maxCommits int) int { + if maxCommits > 0 && maxCommits < c.perPage { + return maxCommits + } + return c.perPage +} + +func remainingQuery(q model.Query, have int) model.Query { + if q.MaxCommits <= 0 { + return q + } + remaining := q.MaxCommits - have + if remaining < 0 { + remaining = 0 + } + q.MaxCommits = remaining + return q +} + // statusError is a non-2xx GitLab API response. Keeping the status code typed // (rather than only in the message string) lets an org scan classify per-project // failures and skip past them. Its message is unchanged from the prior inline diff --git a/gitlabclient/gitlabclient_test.go b/gitlabclient/gitlabclient_test.go index 7b043e3..eb37e2c 100644 --- a/gitlabclient/gitlabclient_test.go +++ b/gitlabclient/gitlabclient_test.go @@ -195,6 +195,48 @@ func TestCollectScopeOrg(t *testing.T) { } } +func TestCollectScopeOrgStopsProjectPaginationAtMaxCommits(t *testing.T) { + const twoCommits = `[ + {"id":"one","message":"first","authored_date":"2026-05-21T11:00:00Z"}, + {"id":"two","message":"second","authored_date":"2026-05-21T12:00:00Z"} + ]` + mux := http.NewServeMux() + mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch path := r.URL.EscapedPath(); { + case strings.Contains(path, "/groups/skaphos/projects"): + if r.URL.Query().Get("page") == "2" { + t.Errorf("group project pagination should stop once MaxCommits is reached") + } + w.Header().Set("X-Next-Page", "2") + _, _ = w.Write([]byte(`[{"id":42,"path_with_namespace":"skaphos/sting"}]`)) + case strings.Contains(path, "/projects/42/repository/commits"): + _, _ = w.Write([]byte(twoCommits)) + default: + t.Errorf("unexpected path %q", path) + } + }) + srv := httptest.NewServer(mux) + defer srv.Close() + + c := newTestClient(t, srv.URL, 50) + res, err := c.Collect(context.Background(), model.Query{ + Author: "octocat", + Scope: model.ScopeOrg, + Org: "skaphos", + MaxCommits: 1, + }) + if err != nil { + t.Fatalf("Collect: %v", err) + } + if res.Count != 1 { + t.Fatalf("Count = %d, want 1", res.Count) + } + if !res.Truncated { + t.Error("Truncated = false, want true") + } +} + // TestCollectScopeOrgSkipsBadProject verifies a group scan records a per-project // failure (here a 404) in Result.Skipped and continues to the healthy project // instead of aborting the whole scan. diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index ca31b70..88a4505 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -797,6 +797,9 @@ func TestLoadConfigValid(t *testing.T) { if cfg.PerPage != 100 { t.Errorf("PerPage = %d, want 100", cfg.PerPage) } + if cfg.MaxCommits != config.DefaultMaxCommits { + t.Errorf("MaxCommits = %d, want %d", cfg.MaxCommits, config.DefaultMaxCommits) + } } // --- Execute / initConfig / must --- diff --git a/internal/cli/root.go b/internal/cli/root.go index f8fa4a2..e95ac24 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -49,7 +49,7 @@ func init() { pf.String("gitlab-token", "", "GitLab personal access token (overrides config/env)") pf.String("gitlab-base-url", "", "GitLab API v4 base URL") pf.Int("per-page", 100, "API page size (1-100)") - pf.Int("max-commits", 0, "cap on returned commits (0 = unlimited)") + pf.Int("max-commits", config.DefaultMaxCommits, "cap on returned commits (0 = unlimited)") // Bind the config-bearing persistent flags to their viper keys so flags win // over env and file when set. diff --git a/internal/mcpserver/getcommits_test.go b/internal/mcpserver/getcommits_test.go index d12f230..e70a929 100644 --- a/internal/mcpserver/getcommits_test.go +++ b/internal/mcpserver/getcommits_test.go @@ -123,6 +123,94 @@ func TestGetCommitsSuccess(t *testing.T) { } } +func TestGetCommitsMaxCommitsOverride(t *testing.T) { + const payload = `{ + "total_count": 2, + "incomplete_results": false, + "items": [ + { + "sha": "one", + "repository": {"full_name": "skaphos/sting"}, + "commit": {"message": "first", "author": {"date": "2026-05-29T12:00:00Z"}} + }, + { + "sha": "two", + "repository": {"full_name": "skaphos/sting"}, + "commit": {"message": "second", "author": {"date": "2026-05-29T13:00:00Z"}} + } + ] + }` + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if !strings.Contains(r.URL.Path, "/search/commits") { + http.Error(w, "unexpected path "+r.URL.Path, http.StatusNotFound) + return + } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(payload)) + })) + defer srv.Close() + + h := newTestHandler(t, srv) + maxCommits := 1 + res, mr, err := h.getCommits(context.Background(), nil, GetCommitsInput{ + Author: "mfacenet", + Scope: "search", + MaxCommits: &maxCommits, + }) + if err != nil { + t.Fatalf("getCommits returned error: %v", err) + } + if res == nil || res.IsError { + t.Fatalf("expected non-error result, got %+v", res) + } + if mr.Count != 1 { + t.Fatalf("Count = %d, want 1", mr.Count) + } + if !mr.Truncated { + t.Error("Truncated = false, want true") + } +} + +func TestGetCommitsDefaultsDiffsOff(t *testing.T) { + orig := collectCommits + t.Cleanup(func() { collectCommits = orig }) + + var gotQuery model.Query + collectCommits = func(_ context.Context, _ config.Config, q model.Query) (model.Result, error) { + gotQuery = q + return model.Result{Author: q.Author}, nil + } + + cfg := config.Default() + cfg.IncludeDiffs = true + h := &handler{cfg: cfg} + + _, _, err := h.getCommits(context.Background(), nil, GetCommitsInput{ + Author: "mfacenet", + Scope: "search", + }) + if err != nil { + t.Fatalf("getCommits returned error: %v", err) + } + if gotQuery.IncludeDiffs { + t.Error("omitted include_diffs should override the server default with false") + } + + on := true + _, _, err = h.getCommits(context.Background(), nil, GetCommitsInput{ + Author: "mfacenet", + Scope: "search", + IncludeDiffs: &on, + }) + if err != nil { + t.Fatalf("getCommits returned error: %v", err) + } + if !gotQuery.IncludeDiffs || !gotQuery.IncludeFiles { + t.Errorf("include_diffs=true should enable diffs and files, got %+v", gotQuery) + } +} + // TestGetCommitsIncludePRs covers the include_prs input branch: with the flag // set on a repos-scope query, the handler discovers a commit on an open PR // branch in addition to the default-branch commit. diff --git a/internal/mcpserver/server.go b/internal/mcpserver/server.go index d8349e6..187eee5 100644 --- a/internal/mcpserver/server.go +++ b/internal/mcpserver/server.go @@ -18,10 +18,10 @@ import ( // GetCommitsInput is the argument schema for the get_commits tool. The // jsonschema descriptions are surfaced to the calling agent. // -// The Include* flags are *bool (rather than bool) so that an explicit -// "false" from the client is distinguishable from an omitted field: with a -// plain bool, both encode as the zero value and a client could never turn -// off a flag the server config enables by default. +// The Include* flags are *bool (rather than bool) so that an explicit "false" +// from the client is distinguishable from an omitted field. Most omitted flags +// inherit server config; IncludeDiffs is the deliberate exception and defaults +// to false for MCP calls because of its API and output cost. type GetCommitsInput struct { Provider string `json:"provider,omitempty" jsonschema:"source control provider: github (default) or gitlab"` Author string `json:"author" jsonschema:"provider username or author string whose commits to retrieve"` @@ -33,8 +33,9 @@ type GetCommitsInput struct { Org string `json:"org,omitempty" jsonschema:"organization or GitLab group; required for scope=org, and scopes GitHub scope=search into that org (reaches private repos the token can access)"` IncludeStats *bool `json:"include_stats,omitempty" jsonschema:"fetch per-commit line additions/deletions; GitHub uses extra API calls, GitLab uses commit-list stats"` IncludeFiles *bool `json:"include_files,omitempty" jsonschema:"fetch per-file change summaries; uses extra commit-detail API calls"` - IncludeDiffs *bool `json:"include_diffs,omitempty" jsonschema:"fetch bounded patch text for changed files; implies include_files and can be token-heavy"` + IncludeDiffs *bool `json:"include_diffs,omitempty" jsonschema:"opt in to bounded patch text for changed files; defaults to false, implies include_files, and uses extra API calls and tokens"` MaxDiffBytes int `json:"max_diff_bytes,omitempty" jsonschema:"per-commit patch byte cap when include_diffs is true; defaults to server config"` + MaxCommits *int `json:"max_commits,omitempty" jsonschema:"cap returned commits for this call; defaults to server config, set 0 only for an intentional exhaustive scan"` IncludePRs *bool `json:"include_prs,omitempty" jsonschema:"also discover commits on open pull-request branches (scope=repos or org, GitHub only); finds unmerged work that commit search and branch listing miss, at the cost of extra API calls"` } @@ -127,12 +128,19 @@ func (h *handler) getCommits(ctx context.Context, _ *mcp.CallToolRequest, in Get if in.IncludeFiles != nil { req.IncludeFiles = in.IncludeFiles } + // Diffs are intentionally opt-in for MCP calls, independent of server config, + // because they add provider requests and can substantially increase output. + includeDiffs := false if in.IncludeDiffs != nil { - req.IncludeDiffs = in.IncludeDiffs + includeDiffs = *in.IncludeDiffs } + req.IncludeDiffs = &includeDiffs if in.MaxDiffBytes != 0 { req.MaxDiffBytes = &in.MaxDiffBytes } + if in.MaxCommits != nil { + req.MaxCommits = in.MaxCommits + } if in.IncludePRs != nil { req.IncludePullRequests = in.IncludePRs }