diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index f768ad1..f3a841b 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -207,6 +207,11 @@ jobs: ' <<<"$metadata" - name: Publish npm package run: npm publish --provenance --access public + - name: Verify npm publication is publicly discoverable + shell: bash + env: + VERSION: ${{ inputs.release_tag || github.ref_name }} + run: node scripts/verify-npm-publication.mjs "${VERSION#v}" - name: Attach npm package to GitHub release env: GH_TOKEN: ${{ github.token }} diff --git a/.release-please-manifest.json b/.release-please-manifest.json index 5fdd883..895bf0e 100644 --- a/.release-please-manifest.json +++ b/.release-please-manifest.json @@ -1,3 +1,3 @@ { - ".": "1.1.0" + ".": "2.0.0" } diff --git a/CHANGELOG.md b/CHANGELOG.md index 1ada2e5..190ca66 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,21 @@ # Changelog +## [2.0.0] (2026-08-09) + +### ⚠ BREAKING CHANGES + +* **mcp:** `github.search_threads` and `github.read_source_files` now require + `repository: {owner, repo}`. Flat `owner` and `repo` request fields are no + longer accepted. See [the v2 MCP migration guide](docs/mcp-v2-migration.md). + +### Features + +* **mcp:** add optional repository scope to authored pull-request portfolios. + +### Bug Fixes + +* **mcp:** return host-neutral native resource links for durable artifacts. + ## [1.1.0](https://github.com/morluto/gitcontribute/compare/v1.0.0...v1.1.0) (2026-08-08) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f40bc36..03602e3 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -64,6 +64,10 @@ filesystem locks, job ownership, or cancellation: make test-race ``` +The focused race lane defaults to four in-package test slots. On a constrained +machine, lower only that setting without reducing the package-level race +coverage, for example `make test-race RACE_TEST_PARALLELISM=2`. + The SQLite driver is pure Go. Keep CGO-disabled compatibility when changing storage or build dependencies. diff --git a/Makefile b/Makefile index 2b16a90..987b2b7 100644 --- a/Makefile +++ b/Makefile @@ -17,6 +17,7 @@ GOTESTSUM ?= $(shell command -v gotestsum 2>/dev/null || printf '%s/bin/gotestsu # use more than the historical four-test cap. TEST_PARALLELISM ?= 8 TEST_PACKAGE_PARALLELISM ?= 8 +RACE_TEST_PARALLELISM ?= 4 INTEGRATION_PARALLELISM ?= 4 GOTESTSUM_FLAGS ?= --rerun-fails=2 --rerun-fails-max-failures=5 @@ -79,12 +80,12 @@ test-uncached: test-race: # Keep package-level overlap for cross-package race coverage while bounding # in-process test concurrency for the CPU-heavy SQLite tests. - $(GO) test -short -race -p=4 -parallel=2 -timeout 600s ./internal/app ./internal/corpus ./internal/workspace + $(GO) test -short -race -p=4 -parallel=$(RACE_TEST_PARALLELISM) -timeout 600s ./internal/app ./internal/corpus ./internal/mcpserver ./internal/workspace test-race-full: # Keep package-level overlap for cross-package race coverage while bounding # in-process test concurrency for the CPU-heavy SQLite tests. - $(GO) test -race -p=4 -parallel=2 -count=1 -timeout 900s ./... + $(GO) test -race -p=4 -parallel=$(RACE_TEST_PARALLELISM) -count=1 -timeout 900s ./... test-verbose: $(GO) test -short -v -p=$(TEST_PACKAGE_PARALLELISM) -parallel=$(TEST_PARALLELISM) -timeout 120s ./... @@ -143,4 +144,4 @@ test-integration: check: fmt-check test lint-changed -verify: fmt-check test-uncached lint-full tidy-check generate-check docs-check +verify: fmt-check vet test-uncached lint-full tidy-check generate-check docs-check diff --git a/docs/architecture.md b/docs/architecture.md index 1cc29fe..f80c699 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -138,8 +138,9 @@ known zero merge rate remains distinct from an unknown rate. Pull-request portfolios use the ordinary repository and thread projections. `github.sync_pull_request_portfolio` is the only public portfolio producer. Its -discriminated selection is either authored discovery or an explicit bounded -set; identity lookup, authored discovery, and scalar status refresh are +discriminated selection is either authored discovery (optionally scoped to one +repository) or an explicit bounded set; identity lookup, authored discovery, +and scalar status refresh are internal phases rather than separately advertised operations. REST `pr_details` and `pr_reviews` facets are combined with typed GraphQL facets for checks, unresolved review threads, detailed merge state, merge queue, @@ -398,10 +399,11 @@ live GitHub request -> local resources/read ``` -`github.search_threads` persists the returned issue or pull-request +`github.search_threads` accepts a required nested repository reference and persists the returned issue or pull-request observations and an exact `github-thread-search.v1` result artifact. A search page never advances repository-wide thread coverage and an empty page is not -proof that no matching live thread exists. `github.read_source_files` resolves +proof that no matching live thread exists. `github.read_source_files` accepts +the same required nested repository reference, resolves one named ref to a commit, reads bounded repository-relative files in input order, and stores a `source-bundle.v1` artifact. Commit SHA is the authoritative revision; GitHub blob SHA remains a separate file identity. Source content is diff --git a/docs/mcp-composed-workflows.md b/docs/mcp-composed-workflows.md index 378723e..ffe426f 100644 --- a/docs/mcp-composed-workflows.md +++ b/docs/mcp-composed-workflows.md @@ -103,8 +103,8 @@ revision authority and are not treated as GitContribute execution results. ## Contribution collision checks ```text -github.search_threads (bounded current work) -github.sync_pull_request_portfolio(selection=authored) -> jobs.get +github.search_threads(repository={owner,repo}, bounded current work) +github.sync_pull_request_portfolio(selection=authored, repository={owner,repo}) -> jobs.get corpus.search_pull_requests | corpus.find_pull_request_overlaps workspace.check_merge_conflicts (only after explicit acquisition) ``` diff --git a/docs/mcp-scalable-workflows.md b/docs/mcp-scalable-workflows.md index 14a48f1..d07bf8b 100644 --- a/docs/mcp-scalable-workflows.md +++ b/docs/mcp-scalable-workflows.md @@ -40,6 +40,9 @@ merge details, checks, files, and other children require explicit facets. `github.read_source_files` resolves a ref once and reads up to 20 ordered repository-relative files with per-file and total-byte limits. Its immutable source-bundle resource records the resolved commit and blob provenance. +Both live repository acquisitions require `repository: {owner, repo}`. See the +[v2 migration guide](mcp-v2-migration.md) for request examples; flat owner and +repo arguments are rejected. `corpus.search_code` accepts up to 20 queries over one repository or snapshot scope. Every query uses the same offline corpus revision. It never falls back @@ -97,6 +100,8 @@ Exact PR refresh uses `github.sync_pull_request_feedback`. CI uses `github.sync_pull_request_ci`; checks and statuses are bound to the observed head SHA. Offline authored-PR reads use `corpus.search_pull_requests`, and overlap analysis uses `corpus.find_pull_request_overlaps`. +Use `repository: {owner, repo}` with authored portfolio synchronization or +offline portfolio reads when the portfolio must be constrained to one project. ## Jobs, partial results, and recovery diff --git a/docs/mcp-v2-migration.md b/docs/mcp-v2-migration.md new file mode 100644 index 0000000..99bb7db --- /dev/null +++ b/docs/mcp-v2-migration.md @@ -0,0 +1,71 @@ +# MCP v2 migration: repository-bound live acquisition + +Version 2 removes the flat `owner` and `repo` fields from the two live +repository-acquisition tools. Both now require one nested `repository` object. +There are no compatibility aliases or mixed forms: callers must update every +request, recovery replay, and saved tool call before connecting to a v2 server. + +## `github.search_threads` + +Before (v1): + +```json +{ + "owner": "acme", + "repo": "rocket", + "query": "cache eviction", + "kind": "issue" +} +``` + +After (v2): + +```json +{ + "repository": {"owner": "acme", "repo": "rocket"}, + "query": "cache eviction", + "kind": "issue" +} +``` + +## `github.read_source_files` + +Before (v1): + +```json +{ + "owner": "acme", + "repo": "rocket", + "ref": "main", + "files": [{"path": "README.md"}] +} +``` + +After (v2): + +```json +{ + "repository": {"owner": "acme", "repo": "rocket"}, + "ref": "main", + "files": [{"path": "README.md"}] +} +``` + +The tools still return an opaque artifact URI. Follow that URI only through MCP +`resources/read`; the resource reader remains local and offline. + +## Scoped authored portfolios + +`github.sync_pull_request_portfolio` accepts the same optional `repository` +scope only with `selection: "authored"`. The returned job follow-up and a +truncated `corpus.search_pull_requests` recovery retain that scope. Explicit +pull-request selections are already exact and reject `repository`. + +```json +{ + "selection": "authored", + "repository": {"owner": "acme", "repo": "rocket"}, + "state": "open", + "limit": 20 +} +``` diff --git a/docs/onboarding.md b/docs/onboarding.md index a8e1520..c342d6b 100644 --- a/docs/onboarding.md +++ b/docs/onboarding.md @@ -230,7 +230,8 @@ One tag version controls the Go binaries and npm package. Release automation: 4. verifies the package has no install lifecycle; 5. installs the tarball with `--ignore-scripts` and runs a smoke test; 6. enforces a 100 MB compressed-package ceiling; -7. publishes the npm package with provenance; +7. publishes the npm package with provenance and waits for its exact version, + `latest` tag, and fresh npx metadata invocation to agree; 8. publishes matching `server.json` metadata to the MCP Registry with GitHub OIDC; 9. creates a matching GitHub release. diff --git a/internal/acquire/acquire.go b/internal/acquire/acquire.go index f829f33..3400c06 100644 --- a/internal/acquire/acquire.go +++ b/internal/acquire/acquire.go @@ -21,6 +21,7 @@ import ( "github.com/morluto/gitcontribute/internal/buflimit" "github.com/morluto/gitcontribute/internal/domain" "github.com/morluto/gitcontribute/internal/gitremote" + "github.com/morluto/gitcontribute/internal/redaction" ) var ( @@ -135,7 +136,7 @@ func (execRunner) Run(ctx context.Context, name string, args ...string) (string, return stdout.String(), buflimit.ErrOutputLimit } if err != nil { - return "", fmt.Errorf("exec %s: %w (stderr: %s)", name, err, strings.TrimSpace(stderr.String())) + return "", fmt.Errorf("exec %s: %w (stderr: %s)", name, err, redaction.String(strings.TrimSpace(stderr.String()))) } return stdout.String(), nil } @@ -337,7 +338,7 @@ func (m *Manager) git(ctx context.Context, dir string, args ...string) (string, return m.runner.Run(ctx, "git", all...) } -func (m *Manager) cloneMirror(ctx context.Context, remote, mirrorPath string) error { +func (m *Manager) cloneMirror(ctx context.Context, remote, mirrorPath string) (resultErr error) { parent := filepath.Dir(mirrorPath) if err := os.MkdirAll(parent, 0700); err != nil { return fmt.Errorf("create mirrors dir: %w", err) @@ -347,8 +348,16 @@ func (m *Manager) cloneMirror(ctx context.Context, remote, mirrorPath string) er tmpPath := filepath.Join(parent, tmpName) defer func() { - if _, err := os.Stat(tmpPath); err == nil { - _ = os.RemoveAll(tmpPath) + _, err := os.Stat(tmpPath) + if errors.Is(err, os.ErrNotExist) { + return + } + if err != nil { + resultErr = errors.Join(resultErr, fmt.Errorf("inspect clone staging path: %w", err)) + return + } + if err := os.RemoveAll(tmpPath); err != nil { + resultErr = errors.Join(resultErr, fmt.Errorf("remove clone staging path: %w", err)) } }() diff --git a/internal/acquire/acquire_test.go b/internal/acquire/acquire_test.go index a1a9b1a..3404b15 100644 --- a/internal/acquire/acquire_test.go +++ b/internal/acquire/acquire_test.go @@ -147,6 +147,58 @@ func TestCleanupWorktreeReturnsGitRemovalFailure(t *testing.T) { } } +func TestCloneMirrorReportsFailedStagingCleanup(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("POSIX directory modes are not available on Windows") + } + parent := t.TempDir() + cloneErr := errors.New("clone failed") + runner := scriptedRunner(func(_ context.Context, _ string, args ...string) (string, error) { + for _, arg := range args { + if arg != "clone" { + continue + } + tmpPath := filepath.Join(parent, args[len(args)-1]) + if err := os.Mkdir(tmpPath, 0755); err != nil { + return "", err + } + if err := os.Chmod(parent, 0500); err != nil { + return "", err + } + return "", cloneErr + } + t.Fatalf("unexpected git invocation: %q", args) + return "", nil + }) + m := &Manager{runner: runner} + + err := m.cloneMirror(context.Background(), "https://example.test/owner/repo.git", filepath.Join(parent, "repo.git")) + if chmodErr := os.Chmod(parent, 0700); chmodErr != nil { + t.Fatal(chmodErr) + } + if !errors.Is(err, cloneErr) { + t.Fatalf("clone error = %v, want clone failure", err) + } + if err == nil || !strings.Contains(err.Error(), "remove clone staging path") { + t.Fatalf("clone error omitted staging cleanup failure: %v", err) + } + entries, readErr := os.ReadDir(parent) + if readErr != nil || len(entries) != 1 || !strings.HasPrefix(entries[0].Name(), ".clone-") { + t.Fatalf("failed clone staging directory was unexpectedly removed: entries=%v err=%v", entries, readErr) + } +} + +func TestExecRunnerRedactsCredentialLikeStderr(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("test uses a POSIX shell to produce controlled stderr") + } + secret := "github_pat_" + strings.Repeat("a", 22) + _, err := (execRunner{}).Run(context.Background(), "sh", "-c", "printf '%s\\n' \"token=$1\" >&2; exit 1", "sh", secret) + if err == nil || strings.Contains(err.Error(), secret) || !strings.Contains(err.Error(), "[REDACTED]") { + t.Fatalf("runner error exposed credential-like stderr: %v", err) + } +} + func TestAcquireRejectsCredentialRemoteBeforeSideEffects(t *testing.T) { fixtureUser := strings.Join([]string{"fixture", "user"}, "-") fixturePassword := strings.Join([]string{"fixture", "password"}, "-") diff --git a/internal/app/control.go b/internal/app/control.go index c5f1353..409e831 100644 --- a/internal/app/control.go +++ b/internal/app/control.go @@ -139,7 +139,8 @@ func (s *Service) ControlStatus(ctx context.Context) (*contracts.ControlStatusRe if err != nil { return nil, err } - stats, err := c.ControlStats(ctx, s.now()) + now := s.now() + stats, err := c.ControlStats(ctx, now) if err != nil { return nil, err } @@ -158,12 +159,17 @@ func (s *Service) ControlStatus(ctx context.Context) (*contracts.ControlStatusRe if resource == "" { resource = "unknown" } + stale := observation.ResetAt.IsZero() || !observation.ResetAt.After(now) rateLimits[i] = contracts.RateLimitState{ Resource: resource, Limit: observation.Limit, Remaining: observation.Remaining, Used: observation.Used, ResetAt: formatTime(observation.ResetAt), - StatusCode: observation.StatusCode, ObservedAt: formatTime(observation.ObservedAt), + Stale: stale, StatusCode: observation.StatusCode, ObservedAt: formatTime(observation.ObservedAt), } - if observation.Limit > 0 && observation.Remaining == 0 && observation.ResetAt.After(s.now()) { + if observation.ResetAt.IsZero() { + warnings = append(warnings, fmt.Sprintf("GitHub %s rate limit observation has no reset time; quota is unknown until the next GitHub response", resource)) + } else if stale { + warnings = append(warnings, fmt.Sprintf("GitHub %s rate limit observation expired; quota is unknown until the next GitHub response", resource)) + } else if observation.Limit > 0 && observation.Remaining == 0 && observation.ResetAt.After(now) { warnings = append(warnings, fmt.Sprintf("GitHub %s rate limit resets at %s", resource, formatTime(observation.ResetAt))) } } @@ -176,7 +182,7 @@ func (s *Service) ControlStatus(ctx context.Context) (*contracts.ControlStatusRe if stats.ActiveRuns > 0 || stats.ActiveJobs > 0 { warnings = append(warnings, "background work is active") } - if !stats.Freshest.IsZero() && s.now().Sub(stats.Freshest) > 7*24*time.Hour { + if !stats.Freshest.IsZero() && now.Sub(stats.Freshest) > 7*24*time.Hour { warnings = append(warnings, "freshest GitHub observation is older than 7 days") } return &contracts.ControlStatusResult{ diff --git a/internal/app/control_test.go b/internal/app/control_test.go index b9eee61..1a97035 100644 --- a/internal/app/control_test.go +++ b/internal/app/control_test.go @@ -233,6 +233,74 @@ func TestControlStatusUsesLocalCorpus(t *testing.T) { } } +func TestControlStatusWarnsWhenRateLimitObservationHasExpired(t *testing.T) { + paths := config.NewPaths(&config.Env{Home: t.TempDir()}) + svc, err := New(paths, "test", nil) + if err != nil { + t.Fatal(err) + } + defer svc.Close() + if _, err := svc.Init(context.Background()); err != nil { + t.Fatal(err) + } + now := time.Date(2026, time.August, 9, 0, 0, 0, 0, time.UTC) + svc.SetClock(func() time.Time { return now }) + c, err := svc.openCorpus(context.Background()) + if err != nil { + t.Fatal(err) + } + if err := c.RecordRateLimitObservation(context.Background(), corpus.RateLimitObservation{ + Attempt: 1, StatusCode: 200, Resource: "core", Limit: 5000, Remaining: 4999, + ObservedAt: now.Add(-time.Hour), ResetAt: now.Add(-time.Minute), + }); err != nil { + t.Fatal(err) + } + + result, err := svc.ControlStatus(context.Background()) + if err != nil { + t.Fatal(err) + } + if !containsString(result.Warnings, "GitHub core rate limit observation expired; quota is unknown until the next GitHub response") { + t.Fatalf("expired rate-limit observation was not marked unknown: %+v", result) + } + if len(result.RateLimits) != 1 || !result.RateLimits[0].Stale { + t.Fatalf("expired rate-limit state was not marked stale: %+v", result.RateLimits) + } +} + +func TestControlStatusWarnsWhenRateLimitObservationHasNoReset(t *testing.T) { + paths := config.NewPaths(&config.Env{Home: t.TempDir()}) + svc, err := New(paths, "test", nil) + if err != nil { + t.Fatal(err) + } + defer svc.Close() + if _, err := svc.Init(context.Background()); err != nil { + t.Fatal(err) + } + c, err := svc.openCorpus(context.Background()) + if err != nil { + t.Fatal(err) + } + if err := c.RecordRateLimitObservation(context.Background(), corpus.RateLimitObservation{ + Attempt: 1, StatusCode: 200, Resource: "core", Limit: 5000, Remaining: 4999, + ObservedAt: time.Date(2026, time.August, 9, 0, 0, 0, 0, time.UTC), + }); err != nil { + t.Fatal(err) + } + + result, err := svc.ControlStatus(context.Background()) + if err != nil { + t.Fatal(err) + } + if !containsString(result.Warnings, "GitHub core rate limit observation has no reset time; quota is unknown until the next GitHub response") { + t.Fatalf("rate-limit observation without reset was not marked unknown: %+v", result) + } + if len(result.RateLimits) != 1 || !result.RateLimits[0].Stale { + t.Fatalf("rate-limit observation without reset was not marked stale: %+v", result.RateLimits) + } +} + func TestDoctorDoesNotExposeEnvironmentToken(t *testing.T) { secret := strings.Join([]string{"github_pat", "fixture-control-value"}, "_") t.Setenv("GITCONTRIBUTE_TEST_TOKEN", secret) diff --git a/internal/app/discovery.go b/internal/app/discovery.go index a6580c9..4d6be83 100644 --- a/internal/app/discovery.go +++ b/internal/app/discovery.go @@ -253,14 +253,7 @@ func (s *Service) crawlSearchSource(ctx context.Context, c *corpus.Corpus, sourc if err != nil { return nil, err } - defer func() { - if resultErr == nil { - return - } - cleanup, cancel := context.WithTimeout(context.WithoutCancel(ctx), 5*time.Second) - defer cancel() - _ = c.FailRun(cleanup, run.ID, resultErr.Error()) - }() + defer failRunOnError(ctx, c, run.ID, &resultErr) now := s.now().UTC().Truncate(time.Second) start := now.Add(-opts.Since) @@ -353,14 +346,7 @@ func (s *Service) crawlRepoSource(ctx context.Context, c *corpus.Corpus, source if err != nil { return nil, err } - defer func() { - if resultErr == nil { - return - } - cleanup, cancel := context.WithTimeout(context.WithoutCancel(ctx), 5*time.Second) - defer cancel() - _ = c.FailRun(cleanup, run.ID, resultErr.Error()) - }() + defer failRunOnError(ctx, c, run.ID, &resultErr) now := s.now().UTC().Truncate(time.Second) processed := 0 @@ -420,14 +406,7 @@ func (s *Service) crawlGHArchiveSource(ctx context.Context, c *corpus.Corpus, so if err != nil { return nil, err } - defer func() { - if resultErr == nil { - return - } - cleanup, cancel := context.WithTimeout(context.WithoutCancel(ctx), 5*time.Second) - defer cancel() - _ = c.FailRun(cleanup, run.ID, resultErr.Error()) - }() + defer failRunOnError(ctx, c, run.ID, &resultErr) now := s.now().UTC() startHour, endHour := discovery.ArchiveHourRange(opts.Since, now) diff --git a/internal/app/job_executor.go b/internal/app/job_executor.go index af744aa..3c8449b 100644 --- a/internal/app/job_executor.go +++ b/internal/app/job_executor.go @@ -45,6 +45,7 @@ type jobExecutorConfig struct { leaseTimeout time.Duration heartbeatInterval time.Duration pollInterval time.Duration + cleanupTimeout time.Duration maxConcurrentJobs int64 maxAdmittedJobs int64 } @@ -54,6 +55,7 @@ func defaultJobExecutorConfig() jobExecutorConfig { leaseTimeout: 10 * time.Second, heartbeatInterval: 2 * time.Second, pollInterval: 200 * time.Millisecond, + cleanupTimeout: jobCleanupTimeout, maxConcurrentJobs: 4, maxAdmittedJobs: 256, } @@ -94,6 +96,9 @@ func newJobExecutorWithConfig(ctx context.Context, c jobStore, cfg jobExecutorCo if cfg.pollInterval <= 0 { cfg.pollInterval = defaultJobExecutorConfig().pollInterval } + if cfg.cleanupTimeout <= 0 { + cfg.cleanupTimeout = defaultJobExecutorConfig().cleanupTimeout + } if cfg.maxConcurrentJobs <= 0 { cfg.maxConcurrentJobs = defaultJobExecutorConfig().maxConcurrentJobs } @@ -128,7 +133,7 @@ func newJobExecutorWithConfig(ctx context.Context, c jobStore, cfg jobExecutorCo if err := c.ReconcileInterruptedJobs(ctx, cfg.leaseTimeout); err != nil { e.cancel() e.backgroundWG.Wait() - cleanupCtx, cleanupCancel := context.WithTimeout(context.WithoutCancel(ctx), jobCleanupTimeout) + cleanupCtx, cleanupCancel := context.WithTimeout(context.WithoutCancel(ctx), cfg.cleanupTimeout) defer cleanupCancel() cleanupErr := c.DeleteJobOwner(cleanupCtx, ownerID) if cleanupErr != nil { @@ -228,7 +233,7 @@ func (e *JobExecutor) Close() error { e.mu.Unlock() e.backgroundWG.Wait() - cleanupCtx, cleanupCancel := context.WithTimeout(context.WithoutCancel(e.rootCtx), jobCleanupTimeout) + cleanupCtx, cleanupCancel := e.cleanupContext(e.rootCtx) defer cleanupCancel() return e.corpus.DeleteJobOwner(cleanupCtx, e.ownerID) } @@ -242,6 +247,35 @@ func (e *JobExecutor) releaseAdmission() { e.mu.Unlock() } +func (e *JobExecutor) cleanupContext(ctx context.Context) (context.Context, context.CancelFunc) { + return context.WithTimeout(context.WithoutCancel(ctx), e.cfg.cleanupTimeout) +} + +// terminalWriteContext keeps a normal terminal write unbounded, but cancels it +// after the cleanup window once the job is cancelled or the executor closes. +func (e *JobExecutor) terminalWriteContext(jobCtx context.Context) (context.Context, context.CancelFunc) { + writeCtx, cancel := context.WithCancel(context.WithoutCancel(jobCtx)) + done := make(chan struct{}) + go func() { + select { + case <-done: + return + case <-jobCtx.Done(): + } + timer := time.NewTimer(e.cfg.cleanupTimeout) + defer timer.Stop() + select { + case <-done: + case <-timer.C: + cancel() + } + }() + return writeCtx, func() { + close(done) + cancel() + } +} + func (e *JobExecutor) heartbeat() { defer e.backgroundWG.Done() timer := time.NewTimer(e.cfg.heartbeatInterval) @@ -317,9 +351,11 @@ func (e *JobExecutor) run(jobCtx context.Context, id string, cancel context.Canc if err := e.slots.Acquire(jobCtx, 1); err != nil { if e.rootCtx.Err() != nil { + cleanupCtx, cleanupCancel := e.cleanupContext(jobCtx) + defer cleanupCancel() _ = e.corpus.TransitionJob( - context.WithoutCancel(jobCtx), id, - corpus.JobStatusQueued, corpus.JobStatusFailed, "", "executor closed before start", + cleanupCtx, id, + corpus.JobStatusQueued, corpus.JobStatusCancelled, "", "executor closed before start", ) } return @@ -329,13 +365,16 @@ func (e *JobExecutor) run(jobCtx context.Context, id string, cancel context.Canc e.mu.Lock() if e.closed { e.mu.Unlock() - _ = e.corpus.TransitionJob(context.WithoutCancel(jobCtx), id, corpus.JobStatusQueued, corpus.JobStatusFailed, "", "executor closed before start") + cleanupCtx, cleanupCancel := e.cleanupContext(jobCtx) + defer cleanupCancel() + _ = e.corpus.TransitionJob(cleanupCtx, id, corpus.JobStatusQueued, corpus.JobStatusCancelled, "", "executor closed before start") return } e.mu.Unlock() if err := e.corpus.StartJobAs(jobCtx, id, e.ownerID); err != nil { - writeCtx := context.WithoutCancel(jobCtx) + writeCtx, writeCancel := e.cleanupContext(jobCtx) + defer writeCancel() job, getErr := e.corpus.GetJob(writeCtx, id) if getErr != nil { message := errors.Join(err, fmt.Errorf("get job after start failure: %w", getErr)).Error() @@ -352,14 +391,16 @@ func (e *JobExecutor) run(jobCtx context.Context, id string, cancel context.Canc return } - _ = e.corpus.RecordJobEvent(context.WithoutCancel(jobCtx), id, "info", "job started") + startCtx, startCancel := e.cleanupContext(jobCtx) + _ = e.corpus.RecordJobEvent(startCtx, id, "info", "job started") + startCancel() result, runErr := fn(jobCtx, func(progress, statistics string) error { return e.corpus.UpdateJobProgress(jobCtx, id, progress, statistics) }) - writeCtx := context.WithoutCancel(jobCtx) - + writeCtx, writeCancel := e.terminalWriteContext(jobCtx) + defer writeCancel() job, err := e.corpus.GetJob(writeCtx, id) if err != nil { // Best effort: preserve the read error in durable job state. diff --git a/internal/app/job_executor_terminal_write_test.go b/internal/app/job_executor_terminal_write_test.go new file mode 100644 index 0000000..0d6875c --- /dev/null +++ b/internal/app/job_executor_terminal_write_test.go @@ -0,0 +1,70 @@ +package app + +import ( + "context" + "sync" + "testing" + "time" + + "github.com/morluto/gitcontribute/internal/corpus" +) + +type gatedFinishJobStore struct { + jobStore + entered chan struct{} + release chan struct{} + timedOut chan struct{} + once sync.Once +} + +func (s *gatedFinishJobStore) TransitionJob(ctx context.Context, id, from, to, result, errStr string) error { + if from != corpus.JobStatusRunning { + return s.jobStore.TransitionJob(ctx, id, from, to, result, errStr) + } + s.once.Do(func() { close(s.entered) }) + select { + case <-s.release: + return s.jobStore.TransitionJob(ctx, id, from, to, result, errStr) + case <-ctx.Done(): + close(s.timedOut) + return ctx.Err() + } +} + +func TestJobExecutorAllowsNormalTerminalWritePastCleanupTimeout(t *testing.T) { + t.Parallel() + ctx := context.Background() + svc := newJobTestService(t) + store := &gatedFinishJobStore{ + jobStore: svc.corpus, + entered: make(chan struct{}), + release: make(chan struct{}), + timedOut: make(chan struct{}), + } + jobs, err := newJobExecutorWithConfig(ctx, store, jobExecutorConfig{ + pollInterval: time.Hour, cleanupTimeout: 25 * time.Millisecond, + }) + if err != nil { + t.Fatalf("new executor: %v", err) + } + svc.jobs = jobs + + id, err := jobs.Submit(ctx, "gated-finish", nil, func(context.Context, func(string, string) error) (any, error) { + return "done", nil + }) + if err != nil { + t.Fatalf("submit: %v", err) + } + select { + case <-store.entered: + case <-time.After(time.Second): + t.Fatal("job did not begin its terminal write") + } + select { + case <-store.timedOut: + t.Fatal("normal terminal write was limited by the cleanup timeout") + case <-time.After(3 * 25 * time.Millisecond): + } + close(store.release) + waitForJobStatus(t, jobs, id, corpus.JobStatusSucceeded, time.Second) +} diff --git a/internal/app/job_executor_test.go b/internal/app/job_executor_test.go index 44de89d..ae206d8 100644 --- a/internal/app/job_executor_test.go +++ b/internal/app/job_executor_test.go @@ -22,6 +22,21 @@ type faultingJobStore struct { startErr error } +type blockingFinishJobStore struct { + jobStore + entered chan struct{} + once sync.Once +} + +func (s *blockingFinishJobStore) TransitionJob(ctx context.Context, id, from, to, result, errStr string) error { + if from == corpus.JobStatusRunning { + s.once.Do(func() { close(s.entered) }) + <-ctx.Done() + return ctx.Err() + } + return s.jobStore.TransitionJob(ctx, id, from, to, result, errStr) +} + func (s *faultingJobStore) GetJob(ctx context.Context, id string) (*corpus.Job, error) { s.mu.Lock() if s.failNextGet { @@ -199,6 +214,90 @@ func TestJobExecutorBoundsConcurrentJobs(t *testing.T) { waitForJobStatus(t, jobs, secondID, corpus.JobStatusSucceeded, 2*time.Second) } +func TestJobExecutorCloseCancelsQueuedJobs(t *testing.T) { + t.Parallel() + ctx := context.Background() + svc := newJobTestService(t) + jobs := newJobExecutorOnService(t, svc, jobExecutorConfig{pollInterval: time.Hour, maxConcurrentJobs: 1}) + + started := make(chan struct{}) + firstID, err := jobs.Submit(ctx, "first", nil, func(ctx context.Context, _ func(string, string) error) (any, error) { + close(started) + <-ctx.Done() + return nil, ctx.Err() + }) + if err != nil { + t.Fatalf("submit first: %v", err) + } + <-started + + queuedRan := make(chan struct{}, 1) + queuedID, err := jobs.Submit(ctx, "queued", nil, func(context.Context, func(string, string) error) (any, error) { + queuedRan <- struct{}{} + return nil, nil + }) + if err != nil { + t.Fatalf("submit queued: %v", err) + } + + if err := jobs.Close(); err != nil { + t.Fatalf("close jobs: %v", err) + } + select { + case <-queuedRan: + t.Fatal("queued job started during executor shutdown") + default: + } + for _, id := range []string{firstID, queuedID} { + job, err := jobs.Get(ctx, id) + if err != nil { + t.Fatalf("get %s: %v", id, err) + } + if job.Status != corpus.JobStatusCancelled { + t.Fatalf("job %s status = %q, want cancelled", id, job.Status) + } + } +} + +func TestJobExecutorCloseBoundsTerminalWriteAfterCancellation(t *testing.T) { + t.Parallel() + ctx := context.Background() + svc := newJobTestService(t) + store := &blockingFinishJobStore{jobStore: svc.corpus, entered: make(chan struct{})} + jobs, err := newJobExecutorWithConfig(ctx, store, jobExecutorConfig{ + pollInterval: time.Hour, cleanupTimeout: time.Second, + }) + if err != nil { + t.Fatalf("new executor: %v", err) + } + svc.jobs = jobs + + started := make(chan struct{}) + if _, err := jobs.Submit(ctx, "blocked-finish", nil, func(context.Context, func(string, string) error) (any, error) { + close(started) + return "done", nil + }); err != nil { + t.Fatalf("submit: %v", err) + } + <-started + select { + case <-store.entered: + case <-time.After(3 * time.Second): + t.Fatal("job did not begin its terminal write") + } + + closed := make(chan error, 1) + go func() { closed <- jobs.Close() }() + select { + case err := <-closed: + if err != nil { + t.Fatalf("close executor: %v", err) + } + case <-time.After(3 * time.Second): + t.Fatal("close waited indefinitely for a cancelled terminal write") + } +} + func TestJobExecutorRecordsReadErrorAfterExecution(t *testing.T) { t.Parallel() ctx := context.Background() diff --git a/internal/app/mcp_authored_sync.go b/internal/app/mcp_authored_sync.go index 2f05900..31626b8 100644 --- a/internal/app/mcp_authored_sync.go +++ b/internal/app/mcp_authored_sync.go @@ -14,6 +14,7 @@ import ( ) type authoredPullRequestSyncOptions struct { + Repository *mcpcontract.RepositoryRef State string UpdatedAfter string Limit int @@ -68,7 +69,12 @@ func (s *Service) syncAuthoredPullRequests(ctx context.Context, in authoredPullR } perPage := min(100, in.Limit-discovered) requests++ - result, err := searcher.SearchAuthoredPullRequests(ctx, github.AuthoredPullRequestSearchOptions{Login: identity.Login, State: in.State, UpdatedAfter: updatedAfter, PageOptions: github.PageOptions{Page: page, PerPage: perPage}}) + options := github.AuthoredPullRequestSearchOptions{Login: identity.Login, State: in.State, UpdatedAfter: updatedAfter, PageOptions: github.PageOptions{Page: page, PerPage: perPage}} + if in.Repository != nil { + options.RepositoryOwner = in.Repository.Owner + options.RepositoryName = in.Repository.Repo + } + result, err := searcher.SearchAuthoredPullRequests(ctx, options) if err != nil { return nil, err } @@ -77,6 +83,9 @@ func (s *Service) syncAuthoredPullRequests(ctx context.Context, in authoredPullR if pr.RepositoryOwner == "" || pr.RepositoryName == "" { continue } + if in.Repository != nil && (!strings.EqualFold(pr.RepositoryOwner, in.Repository.Owner) || !strings.EqualFold(pr.RepositoryName, in.Repository.Repo)) { + continue + } key := pr.RepositoryOwner + "/" + pr.RepositoryName if _, exists := byRepo[key]; !exists { order = append(order, key) diff --git a/internal/app/mcp_ensure_coverage.go b/internal/app/mcp_ensure_coverage.go index bddcabb..a055a17 100644 --- a/internal/app/mcp_ensure_coverage.go +++ b/internal/app/mcp_ensure_coverage.go @@ -38,6 +38,12 @@ func (r *MCPReader) EnsureCoverage(ctx context.Context, in mcpcontract.EnsureCov if in.LimitPerRepository < 1 || in.LimitPerRepository > 1000 { return mcpcontract.JobReference{}, errors.New("limit_per_repository must be between 1 and 1000") } + if err := validateEnsureCoverageTarget(in.Target); err != nil { + return mcpcontract.JobReference{}, err + } + if in.Target.Type == mcpcontract.CoverageTargetRepository && len(in.Facets) > 0 { + return mcpcontract.JobReference{}, errors.New("facets can be selected only for exact-thread coverage") + } allowedFacets := make(map[string]struct{}) for _, name := range facets.SelectableNames() { allowedFacets[name] = struct{}{} @@ -52,9 +58,6 @@ func (r *MCPReader) EnsureCoverage(ctx context.Context, in mcpcontract.EnsureCov } seenFacets[name] = struct{}{} } - if err := validateEnsureCoverageTarget(in.Target); err != nil { - return mcpcontract.JobReference{}, err - } id, err := r.submitJob(ctx, jobKindEnsureCoverage, in, func(ctx context.Context, report func(string, string) error) (any, error) { return r.ensureCoverage(ctx, in, report) }) diff --git a/internal/app/mcp_ensure_coverage_test.go b/internal/app/mcp_ensure_coverage_test.go new file mode 100644 index 0000000..7696ea5 --- /dev/null +++ b/internal/app/mcp_ensure_coverage_test.go @@ -0,0 +1,22 @@ +package app + +import ( + "context" + "strings" + "testing" + + "github.com/morluto/gitcontribute/internal/mcpcontract" +) + +func TestEnsureCoverageRejectsThreadFacetsForRepositoryTarget(t *testing.T) { + _, err := (&MCPReader{}).EnsureCoverage(context.Background(), mcpcontract.EnsureCoverageInput{ + Target: mcpcontract.CoverageTarget{ + Type: mcpcontract.CoverageTargetRepository, + Repository: mcpcontract.RepositoryRef{Owner: "acme", Repo: "rocket"}, + }, + Facets: []string{"issue_comments"}, + }) + if err == nil || !strings.Contains(err.Error(), "exact-thread") { + t.Fatalf("repository coverage with facets error = %v", err) + } +} diff --git a/internal/app/mcp_explain_evidence_test.go b/internal/app/mcp_explain_evidence_test.go index ca1c013..0ab14fe 100644 --- a/internal/app/mcp_explain_evidence_test.go +++ b/internal/app/mcp_explain_evidence_test.go @@ -71,3 +71,24 @@ func TestMCPReaderExplainMatchReturnsMatchingExcerpt(t *testing.T) { t.Fatalf("repository explanation omitted topic match: %q", repoOut.Snippet) } } + +func TestMCPReaderExplainMatchTreatsWhitespaceQueryAsOmitted(t *testing.T) { + t.Parallel() + ctx := context.Background() + svc := newSearchTestService(t) + repo, err := svc.corpus.UpsertRepository(ctx, corpus.Repository{Owner: "owner", Name: "repo"}, `{}`) + if err != nil { + t.Fatal(err) + } + if _, err := svc.corpus.UpsertThread(ctx, corpus.Thread{RepositoryID: repo.ID, Kind: corpus.ThreadKindIssue, Number: 1, State: "open", Title: "exact item", SourceUpdatedAt: time.Unix(1, 0).UTC()}, `{}`); err != nil { + t.Fatal(err) + } + + out, err := svc.MCPReader().ExplainMatch(ctx, mcpcontract.ExplainMatchInput{Owner: "owner", Repo: "repo", Kind: "issue", Number: 1, Query: " \t\n "}) + if err != nil { + t.Fatal(err) + } + if out.Query != "" || out.Reason != "repository present in local corpus" { + t.Fatalf("whitespace explanation = %+v", out) + } +} diff --git a/internal/app/mcp_fix_patterns.go b/internal/app/mcp_fix_patterns.go index 5981f52..baa8165 100644 --- a/internal/app/mcp_fix_patterns.go +++ b/internal/app/mcp_fix_patterns.go @@ -106,9 +106,17 @@ func (r *MCPReader) GetFixPatternReport(ctx context.Context, id string) (mcpcont return mcpcontract.FixPatternReport{}, fmt.Errorf("decode fix-pattern report identity: %w", err) } if identity.SnapshotToken == "" { + var request mcpcontract.MineRepositoryFixPatternsInput + var actions []mcpcontract.ToolCall + if err := json.Unmarshal([]byte(job.Request), &request); err == nil { + if request, err = normalizeFixPatternInput(request); err == nil { + actions = append(actions, mcpcontract.RecoveryAction(request)) + } + } return mcpcontract.FixPatternReport{}, mcpcontract.Unavailable( "legacy_artifact", "this persisted fix-pattern report predates immutable snapshot binding; rerun the fix-pattern workflow to regenerate it", + actions..., ) } report.Persisted = true diff --git a/internal/app/mcp_fix_patterns_test.go b/internal/app/mcp_fix_patterns_test.go index 784a606..78e852c 100644 --- a/internal/app/mcp_fix_patterns_test.go +++ b/internal/app/mcp_fix_patterns_test.go @@ -2,6 +2,7 @@ package app import ( "context" + "encoding/json" "errors" "strings" "testing" @@ -184,7 +185,16 @@ func TestGetFixPatternReportRejectsLegacyUnboundArtifact(t *testing.T) { t.Parallel() ctx := context.Background() svc := newSearchTestService(t) - job, err := svc.corpus.CreateJob(ctx, "mine_repository_fix_patterns", `{}`) + request := mcpcontract.MineRepositoryFixPatternsInput{ + Repository: mcpcontract.RepositoryRef{Owner: "acme", Repo: "rocket"}, + TimeWindow: mcpcontract.FixPatternTimeWindow{UpdatedAfter: "2026-07-01T00:00:00Z"}, + SymptomTaxonomy: []mcpcontract.FixPatternSymptom{{Name: "drift", Terms: []string{"drift"}}}, + } + requestJSON, err := json.Marshal(request) + if err != nil { + t.Fatal(err) + } + job, err := svc.corpus.CreateJob(ctx, "mine_repository_fix_patterns", string(requestJSON)) if err != nil { t.Fatal(err) } @@ -199,6 +209,9 @@ func TestGetFixPatternReportRejectsLegacyUnboundArtifact(t *testing.T) { if !errors.As(err, &toolErr) || toolErr.Code != "legacy_artifact" { t.Fatalf("legacy report error = %v", err) } + if toolErr.Recovery == nil || len(toolErr.Recovery.Then) != 1 || toolErr.Recovery.Then[0].MineFixPatterns == nil || toolErr.Recovery.Then[0].MineFixPatterns.Repository != request.Repository { + t.Fatalf("legacy report recovery = %+v", toolErr.Recovery) + } } func TestNormalizeFixPatternInputRejectsInvalidWindow(t *testing.T) { diff --git a/internal/app/mcp_github_acquisition.go b/internal/app/mcp_github_acquisition.go index defe3a3..21532c6 100644 --- a/internal/app/mcp_github_acquisition.go +++ b/internal/app/mcp_github_acquisition.go @@ -41,7 +41,7 @@ func (r *MCPReader) SearchGitHubThreads(ctx context.Context, in mcpcontract.Sear return mcpcontract.SearchGitHubThreadsOutput{}, errors.New("configured GitHub reader does not support thread search") } result, err := searcher.SearchThreads(ctx, github.ThreadSearchOptions{ - Owner: in.Owner, Repo: in.Repo, Query: in.Query, Kind: github.ThreadKind(in.Kind), State: in.State, + Owner: in.Repository.Owner, Repo: in.Repository.Repo, Query: in.Query, Kind: github.ThreadKind(in.Kind), State: in.State, Sort: in.Sort, Order: in.Order, PageOptions: github.PageOptions{Page: in.Page, PerPage: in.Limit}, }) if err != nil { @@ -51,7 +51,7 @@ func (r *MCPReader) SearchGitHubThreads(ctx context.Context, in mcpcontract.Sear } func validateGitHubThreadSearchInput(in *mcpcontract.SearchGitHubThreadsInput) error { - if err := (domain.RepoRef{Owner: in.Owner, Repo: in.Repo}).Validate(); err != nil { + if err := (domain.RepoRef{Owner: in.Repository.Owner, Repo: in.Repository.Repo}).Validate(); err != nil { return err } in.Query = strings.TrimSpace(in.Query) @@ -98,7 +98,7 @@ func (r *MCPReader) persistGitHubThreadSearch(ctx context.Context, in mcpcontrac } now := r.now().UTC() out := mcpcontract.SearchGitHubThreadsOutput{ - Status: "complete", Repository: mcpcontract.RepositoryRef{Owner: in.Owner, Repo: in.Repo}, Query: in.Query, + Status: "complete", Repository: in.Repository, Query: in.Query, ProviderQuery: result.Query, Kind: in.Kind, State: in.State, Sort: in.Sort, Order: in.Order, Page: in.Page, Limit: in.Limit, Total: result.Total, Incomplete: result.Incomplete, Rate: githubRateOutput(result.Rate), Coverage: "repository_thread_coverage_incomplete", ObservedAt: formatTime(now), @@ -140,25 +140,25 @@ func (r *MCPReader) persistGitHubThreadSearch(ctx context.Context, in mcpcontrac } artifact.RecoveryPlans = append([]mcpcontract.RecoveryPlan(nil), out.RecoveryPlans...) - repo, err := ensureSearchRepository(ctx, c, in.Owner, in.Repo) + repo, err := ensureSearchRepository(ctx, c, in.Repository.Owner, in.Repository.Repo) if err != nil { return mcpcontract.SearchGitHubThreadsOutput{}, err } for index, issue := range result.Items { if issue.RepositoryOwner == "" { - issue.RepositoryOwner = in.Owner + issue.RepositoryOwner = in.Repository.Owner } if issue.RepositoryName == "" { - issue.RepositoryName = in.Repo + issue.RepositoryName = in.Repository.Repo } item := mcpcontract.BatchItem[mcpcontract.ThreadOutput]{Key: threadSearchItemKey(issue, index), Status: "complete"} - if !strings.EqualFold(issue.RepositoryOwner, in.Owner) || !strings.EqualFold(issue.RepositoryName, in.Repo) { + if !strings.EqualFold(issue.RepositoryOwner, in.Repository.Owner) || !strings.EqualFold(issue.RepositoryName, in.Repository.Repo) { item.Status = "failed" item.Reason = "repository_scope_mismatch" - item.Message = fmt.Sprintf("provider returned %s/%s for requested %s/%s", issue.RepositoryOwner, issue.RepositoryName, in.Owner, in.Repo) + item.Message = fmt.Sprintf("provider returned %s/%s for requested %s/%s", issue.RepositoryOwner, issue.RepositoryName, in.Repository.Owner, in.Repository.Repo) out.Status = "partial" out.Items[index] = item - artifact.Items[index] = githubThreadSearchArtifactItem(issue, index, in.Owner, in.Repo) + artifact.Items[index] = githubThreadSearchArtifactItem(issue, index, in.Repository.Owner, in.Repository.Repo) continue } thread, payload, payloadErr := threadFromIssue(issue) @@ -178,12 +178,12 @@ func (r *MCPReader) persistGitHubThreadSearch(ctx context.Context, in mcpcontrac out.Status = "partial" } out.Items[index] = item - artifact.Items[index] = githubThreadSearchArtifactItem(issue, index, in.Owner, in.Repo) + artifact.Items[index] = githubThreadSearchArtifactItem(issue, index, in.Repository.Owner, in.Repository.Repo) } snapshot, err := c.MaterializeReadSnapshot(ctx, corpus.SnapshotMaterialization{ Kind: githubThreadSearchArtifactKind, - Scope: map[string]any{"repository": in.Owner + "/" + in.Repo, "query": in.Query, "page": in.Page}, + Scope: map[string]any{"repository": in.Repository.Owner + "/" + in.Repository.Repo, "query": in.Query, "page": in.Page}, SourceManifest: map[string]any{"provider_query": result.Query, "item_ids": artifactItemIDs(artifact.Items)}, DerivedVersions: map[string]string{"github_thread_search": "v1"}, Completeness: artifact.Completeness, @@ -240,7 +240,7 @@ func (r *MCPReader) ReadSourceFiles(ctx context.Context, in mcpcontract.ReadSour for i, file := range in.Files { requests[i] = github.SourceFileRequest{Path: file.Path, StartLine: file.StartLine, EndLine: file.EndLine} } - result, err := fileReader.ReadSourceFiles(ctx, in.Owner, in.Repo, in.Ref, requests, github.SourceFileReadOptions{PerFileBytes: in.PerFileBytes, TotalBytes: in.TotalBytes}) + result, err := fileReader.ReadSourceFiles(ctx, in.Repository.Owner, in.Repository.Repo, in.Ref, requests, github.SourceFileReadOptions{PerFileBytes: in.PerFileBytes, TotalBytes: in.TotalBytes}) if err != nil { return mcpcontract.ReadSourceFilesOutput{}, err } @@ -248,10 +248,11 @@ func (r *MCPReader) ReadSourceFiles(ctx context.Context, in mcpcontract.ReadSour } func validateReadSourceFilesInput(in *mcpcontract.ReadSourceFilesInput) error { - if err := (domain.RepoRef{Owner: in.Owner, Repo: in.Repo}).Validate(); err != nil { + if err := (domain.RepoRef{Owner: in.Repository.Owner, Repo: in.Repository.Repo}).Validate(); err != nil { return err } - if strings.TrimSpace(in.Ref) == "" { + in.Ref = strings.TrimSpace(in.Ref) + if in.Ref == "" { return errors.New("ref is required") } if len(in.Files) < 1 || len(in.Files) > maxSourceFileRequests { @@ -294,7 +295,7 @@ func (r *MCPReader) persistSourceBundle(ctx context.Context, in mcpcontract.Read } now := r.now().UTC() out := mcpcontract.ReadSourceFilesOutput{ - Status: "complete", Repository: mcpcontract.RepositoryRef{Owner: in.Owner, Repo: in.Repo}, + Status: "complete", Repository: in.Repository, RequestedRef: result.Resolution.RequestedRef, ResolvedRef: result.Resolution.ResolvedRef, CommitSHA: result.Resolution.CommitSHA, PerFileBytes: in.PerFileBytes, TotalByteLimit: in.TotalBytes, TotalBytes: result.TotalBytes, Items: make([]mcpcontract.SourceFileBatchItem, len(result.Items)), ObservedAt: formatTime(now), Rate: githubRateOutput(result.Rate), @@ -338,7 +339,7 @@ func (r *MCPReader) persistSourceBundle(ctx context.Context, in mcpcontract.Read artifact.Completeness.ContentsBounded = true snapshot, err := c.MaterializeReadSnapshot(ctx, corpus.SnapshotMaterialization{ Kind: sourceBundleArtifactKind, - Scope: map[string]any{"repository": in.Owner + "/" + in.Repo, "requested_ref": in.Ref, "paths": sourceBundlePaths(in.Files)}, + Scope: map[string]any{"repository": in.Repository.Owner + "/" + in.Repository.Repo, "requested_ref": in.Ref, "paths": sourceBundlePaths(in.Files)}, SourceManifest: map[string]any{"commit_sha": result.Resolution.CommitSHA, "item_statuses": sourceBundleStatuses(result.Items)}, DerivedVersions: map[string]string{"source_bundle": "v1"}, Completeness: artifact.Completeness, Provenance: artifact.Provenance, Payload: artifact, diff --git a/internal/app/mcp_github_acquisition_test.go b/internal/app/mcp_github_acquisition_test.go index 5978d05..0f2b4b5 100644 --- a/internal/app/mcp_github_acquisition_test.go +++ b/internal/app/mcp_github_acquisition_test.go @@ -67,7 +67,7 @@ func TestMCPReaderSearchGitHubThreadsPersistsArtifactWithoutFullCoverage(t *test now := time.Date(2026, 8, 1, 1, 2, 3, 0, time.UTC) svc.SetClock(func() time.Time { return now }) reader := &MCPReader{svc} - out, err := reader.SearchGitHubThreads(context.Background(), mcpcontract.SearchGitHubThreadsInput{Owner: "acme", Repo: "rocket", Query: "persist", Kind: "issue", Limit: 2}) + out, err := reader.SearchGitHubThreads(context.Background(), mcpcontract.SearchGitHubThreadsInput{Repository: mcpcontract.RepositoryRef{Owner: "acme", Repo: "rocket"}, Query: "persist", Kind: "issue", Limit: 2}) if err != nil { t.Fatal(err) } @@ -138,7 +138,7 @@ func TestMCPReaderReadSourceFilesStoresCommitAndBlobProvenanceAndReadsLocally(t defer func() { _ = svc.Close() }() reader := &MCPReader{svc} out, err := reader.ReadSourceFiles(context.Background(), mcpcontract.ReadSourceFilesInput{ - Owner: "acme", Repo: "rocket", Ref: "main", Files: []mcpcontract.SourceFileRequest{{Path: "README.md", StartLine: 2, EndLine: 2}, {Path: "missing.md"}}, + Repository: mcpcontract.RepositoryRef{Owner: "acme", Repo: "rocket"}, Ref: "main", Files: []mcpcontract.SourceFileRequest{{Path: "README.md", StartLine: 2, EndLine: 2}, {Path: "missing.md"}}, PerFileBytes: 100, TotalBytes: 100, }) if err != nil { @@ -164,6 +164,18 @@ func TestMCPReaderReadSourceFilesStoresCommitAndBlobProvenanceAndReadsLocally(t } } +func TestValidateReadSourceFilesInputTrimsRef(t *testing.T) { + in := mcpcontract.ReadSourceFilesInput{ + Repository: mcpcontract.RepositoryRef{Owner: "acme", Repo: "rocket"}, Ref: " main ", Files: []mcpcontract.SourceFileRequest{{Path: "README.md"}}, + } + if err := validateReadSourceFilesInput(&in); err != nil { + t.Fatal(err) + } + if in.Ref != "main" { + t.Fatalf("ref = %q, want canonical main", in.Ref) + } +} + func TestMCPReaderSearchCodeBatchUsesOneOfflineRevisionAndPreservesQueryOrder(t *testing.T) { ctx := context.Background() svc := newSearchTestService(t) diff --git a/internal/app/mcp_job_artifacts.go b/internal/app/mcp_job_artifacts.go index 2eb0cac..8504fcf 100644 --- a/internal/app/mcp_job_artifacts.go +++ b/internal/app/mcp_job_artifacts.go @@ -11,6 +11,8 @@ import ( "github.com/morluto/gitcontribute/internal/mcpcontract" ) +const maxJobArtifactItems = 100 + func jobArtifactsAndFollowUp(job *contracts.JobResult, total int) ([]mcpcontract.JobArtifactReference, *mcpcontract.JobFollowUp) { switch job.Kind { case "mine_repository_fix_patterns": @@ -122,24 +124,29 @@ func decodeSyncBatchResult(job *contracts.JobResult, total int) (syncBatchResult return result, count } -func syncBatchReferences(result syncBatchResult, includeThreads bool) ([]string, []mcpcontract.ThreadRef, []mcpcontract.JobArtifactFailure) { - references := make([]string, 0, min(len(result.Items), 100)) - threadRefs := make([]mcpcontract.ThreadRef, 0, min(len(result.Items), 100)) - failures := make([]mcpcontract.JobArtifactFailure, 0, min(len(result.Items), 100)) +func syncBatchReferences(result syncBatchResult, includeThreads bool) ([]string, []mcpcontract.ThreadRef, []mcpcontract.JobArtifactFailure, bool, bool) { + references := make([]string, 0, min(len(result.Items), maxJobArtifactItems)) + threadRefs := make([]mcpcontract.ThreadRef, 0, min(len(result.Items), maxJobArtifactItems)) + failures := make([]mcpcontract.JobArtifactFailure, 0, min(len(result.Items), maxJobArtifactItems)) + referencesTruncated := false + failuresTruncated := false for _, item := range result.Items { partialThreadBatch := includeThreads && item.Status == "partial" if item.Status != "complete" && !partialThreadBatch { - if len(failures) < 100 { + if len(failures) < maxJobArtifactItems { failures = append(failures, mcpcontract.JobArtifactFailure{ Reference: item.Key, Status: mcpcontract.BatchItemStatus(item.Status), Reason: item.Reason, Message: item.Message, RetryAfterMS: mcpcontract.NonNegativeInt(item.RetryAfter), }) + } else { + failuresTruncated = true } continue } if includeThreads && len(item.Threads) > 0 { for _, ref := range item.Threads { - if len(threadRefs) >= 100 { + if len(threadRefs) >= maxJobArtifactItems { + referencesTruncated = true break } threadRefs = append(threadRefs, ref) @@ -147,34 +154,38 @@ func syncBatchReferences(result syncBatchResult, includeThreads bool) ([]string, } continue } - if item.Key != "" && len(references) < 100 { - references = append(references, item.Key) + if item.Key != "" { + if len(references) < maxJobArtifactItems { + references = append(references, item.Key) + } else { + referencesTruncated = true + } } } - return references, threadRefs, failures + return references, threadRefs, failures, referencesTruncated, failuresTruncated } func repositoryBatchJobArtifact(job *contracts.JobResult, total int) ([]mcpcontract.JobArtifactReference, *mcpcontract.JobFollowUp) { result, count := decodeSyncBatchResult(job, total) - references, _, failures := syncBatchReferences(result, false) + references, _, failures, referencesTruncated, failuresTruncated := syncBatchReferences(result, false) value := mcpcontract.NonNegativeInt(count) - follow := &mcpcontract.JobFollowUp{ - Action: mcpcontract.FollowUpAction{Type: "get_repositories", GetRepositories: &mcpcontract.GetRepositoriesInput{}}, - Reason: "Read synchronized repository facts and coverage from the offline corpus.", - } var request mcpcontract.SyncRepositoryContextInput - if json.Unmarshal([]byte(job.Request), &request) == nil { - follow.Action.GetRepositories.Repositories = append([]mcpcontract.RepositoryRef(nil), request.Repositories...) + var follow *mcpcontract.JobFollowUp + if json.Unmarshal([]byte(job.Request), &request) == nil && len(request.Repositories) > 0 { + follow = &mcpcontract.JobFollowUp{ + Action: mcpcontract.FollowUpAction{Type: "get_repositories", GetRepositories: &mcpcontract.GetRepositoriesInput{Repositories: append([]mcpcontract.RepositoryRef(nil), request.Repositories...)}}, + Reason: "Read synchronized repository facts and coverage from the offline corpus.", + } } return []mcpcontract.JobArtifactReference{{ Kind: "repository_batch", Count: &value, References: references, - ReferencesTruncated: len(result.Items) > len(references), Failures: failures, + ReferencesTruncated: referencesTruncated, Failures: failures, FailuresTruncated: failuresTruncated, }}, follow } func threadBatchJobArtifact(job *contracts.JobResult, total int) ([]mcpcontract.JobArtifactReference, *mcpcontract.JobFollowUp) { result, count := decodeSyncBatchResult(job, total) - references, threadRefs, failures := syncBatchReferences(result, true) + references, threadRefs, failures, referencesTruncated, failuresTruncated := syncBatchReferences(result, true) value := mcpcontract.NonNegativeInt(count) var follow *mcpcontract.JobFollowUp if len(threadRefs) > 0 { @@ -185,13 +196,15 @@ func threadBatchJobArtifact(job *contracts.JobResult, total int) ([]mcpcontract. } return []mcpcontract.JobArtifactReference{{ Kind: "thread_batch", Count: &value, References: references, - ReferencesTruncated: len(result.Items) > len(references), Failures: failures, + ReferencesTruncated: referencesTruncated, Failures: failures, FailuresTruncated: failuresTruncated, }}, follow } func threadFacetJobArtifact(job *contracts.JobResult) ([]mcpcontract.JobArtifactReference, *mcpcontract.JobFollowUp) { var request mcpcontract.HydrateThreadsInput - _ = json.Unmarshal([]byte(job.Request), &request) + if json.Unmarshal([]byte(job.Request), &request) != nil || len(request.Threads) == 0 || len(request.Facets) == 0 { + return nil, nil + } return facetBatchArtifact(append([]mcpcontract.ThreadRef(nil), request.Threads...), request.Facets) } @@ -216,12 +229,18 @@ func portfolioJobArtifact(job *contracts.JobResult) ([]mcpcontract.JobArtifactRe return nil, nil } value := mcpcontract.NonNegativeInt(result.Refreshed) - failures := make([]mcpcontract.JobArtifactFailure, len(result.Failures)) - for i, failure := range result.Failures { - failures[i] = mcpcontract.JobArtifactFailure{ + references, referencesTruncated := boundedArtifactReferences(result.PullRequests) + failures := make([]mcpcontract.JobArtifactFailure, 0, min(len(result.Failures), maxJobArtifactItems)) + failuresTruncated := false + for _, failure := range result.Failures { + if len(failures) >= maxJobArtifactItems { + failuresTruncated = true + continue + } + failures = append(failures, mcpcontract.JobArtifactFailure{ Reference: failure.Reference, Status: mcpcontract.BatchItemStatus(failure.Status), Reason: failure.Reason, Message: failure.Message, RetryAfterMS: mcpcontract.NonNegativeInt(failure.RetryAfterMS), - } + }) } var request mcpcontract.SyncPortfolioInput _ = json.Unmarshal([]byte(job.Request), &request) @@ -236,11 +255,20 @@ func portfolioJobArtifact(job *contracts.JobResult) ([]mcpcontract.JobArtifactRe next.Selection = "authored" } if next.Selection == "" { - next.PullRequests = portfolioResultRefs(result.PullRequests) + next.PullRequests = portfolioResultRefs(references) if len(next.PullRequests) > 0 { next.Selection = "explicit" } } + if next.Selection == "explicit" { + // Stored pre-contract jobs can carry a wider request than the current + // action schema accepts. Recover only the exact, bounded result set + // that this terminal artifact can honestly identify. + next.PullRequests = portfolioResultRefs(references) + if len(next.PullRequests) == 0 { + next.Selection = "" + } + } if next.Selection != "" && next.Selection == "authored" { next.DiscoveryMaxRequests = min(1000, max(next.DiscoveryMaxRequests*2, max(next.DiscoveryMaxRequests+1, 2))) next.Limit = min(100, max(next.Limit*2, max(next.Limit+1, 20))) @@ -249,12 +277,15 @@ func portfolioJobArtifact(job *contracts.JobResult) ([]mcpcontract.JobArtifactRe recovery = recoveryPlan("portfolio_discovery_incomplete", "Portfolio discovery was incomplete or bounded. Repeat synchronization with the returned larger discovery bound, then reread the portfolio.", mcpcontract.RecoveryAction(next)) } } - follow := &mcpcontract.JobFollowUp{ - Action: mcpcontract.FollowUpAction{Type: "list_pull_request_portfolio", ListPortfolio: portfolioReadFollowUpArguments(request, result.Login, result.PullRequests)}, - Reason: "Read these refreshed pull requests from the offline portfolio.", + var follow *mcpcontract.JobFollowUp + if arguments := portfolioReadFollowUpArguments(request, result.Login, references); arguments != nil { + follow = &mcpcontract.JobFollowUp{ + Action: mcpcontract.FollowUpAction{Type: "list_pull_request_portfolio", ListPortfolio: arguments}, + Reason: "Read these refreshed pull requests from the offline portfolio.", + } } return []mcpcontract.JobArtifactReference{{ - Kind: "pull_request_batch", Count: &value, References: append([]string(nil), result.PullRequests...), Failures: failures, + Kind: "pull_request_batch", Count: &value, References: references, ReferencesTruncated: referencesTruncated, Failures: failures, FailuresTruncated: failuresTruncated, Status: result.Status, DiscoveryStatus: result.DiscoveryStatus, SearchIncomplete: result.SearchIncomplete, RequestCapped: result.RequestCapped, Recovery: recovery, }}, follow } @@ -290,12 +321,18 @@ func pullRequestWorkflowJobArtifact(job *contracts.JobResult) ([]mcpcontract.Job kind, reason, resourceKind = "ci_failure_report", "Read the persisted CI reports and bounded job logs through their resource links.", "ci-failure-report" } artifact := mcpcontract.JobArtifactReference{Kind: kind} + completed := 0 if len(result.Items) == 0 { var request struct { PullRequests []mcpcontract.ThreadRef `json:"pull_requests"` } if json.Unmarshal([]byte(job.Request), &request) == nil { for _, ref := range request.PullRequests { + completed++ + if len(artifact.References) >= maxJobArtifactItems { + artifact.ReferencesTruncated = true + continue + } artifact.References = append(artifact.References, fmt.Sprintf( "gitcontribute://%s/%s/%s/%d", resourceKind, ref.Owner, ref.Repo, ref.Number, )) @@ -304,7 +341,16 @@ func pullRequestWorkflowJobArtifact(job *contracts.JobResult) ([]mcpcontract.Job } for _, item := range result.Items { if item.Status == "complete" { - artifact.References = append(artifact.References, item.ResourceURI) + completed++ + if len(artifact.References) < maxJobArtifactItems { + artifact.References = append(artifact.References, item.ResourceURI) + } else { + artifact.ReferencesTruncated = true + } + continue + } + if len(artifact.Failures) >= maxJobArtifactItems { + artifact.FailuresTruncated = true continue } artifact.Failures = append(artifact.Failures, mcpcontract.JobArtifactFailure{ @@ -312,7 +358,7 @@ func pullRequestWorkflowJobArtifact(job *contracts.JobResult) ([]mcpcontract.Job RetryAfterMS: mcpcontract.NonNegativeInt(item.RetryAfterMS), }) } - count := mcpcontract.NonNegativeInt(len(artifact.References)) + count := mcpcontract.NonNegativeInt(completed) artifact.Count = &count var follow *mcpcontract.JobFollowUp if len(artifact.References) > 0 { @@ -321,6 +367,13 @@ func pullRequestWorkflowJobArtifact(job *contracts.JobResult) ([]mcpcontract.Job return []mcpcontract.JobArtifactReference{artifact}, follow } +func boundedArtifactReferences(values []string) ([]string, bool) { + if len(values) <= maxJobArtifactItems { + return append([]string(nil), values...), false + } + return append([]string(nil), values[:maxJobArtifactItems]...), true +} + func pullRequestFeedbackIndexJobArtifact(job *contracts.JobResult) ([]mcpcontract.JobArtifactReference, *mcpcontract.JobFollowUp) { var result pullRequestFeedbackIndexResult if json.Unmarshal([]byte(job.Result), &result) != nil { @@ -330,16 +383,28 @@ func pullRequestFeedbackIndexJobArtifact(job *contracts.JobResult) ([]mcpcontrac if json.Unmarshal([]byte(job.Request), &request) != nil { return nil, nil } - refs := make([]string, 0, len(result.Items)) - failures := make([]mcpcontract.JobArtifactFailure, 0, len(result.Items)) + refs := make([]string, 0, min(len(result.Items), maxJobArtifactItems)) + failures := make([]mcpcontract.JobArtifactFailure, 0, min(len(result.Items), maxJobArtifactItems)) + completed := 0 + referencesTruncated := false + failuresTruncated := false for _, item := range result.Items { if item.Status == "complete" { - refs = append(refs, item.Key) + completed++ + if len(refs) < maxJobArtifactItems { + refs = append(refs, item.Key) + } else { + referencesTruncated = true + } continue } - failures = append(failures, mcpcontract.JobArtifactFailure{Reference: item.Key, Status: item.Status, Reason: item.Code, Message: item.Message, RetryAfterMS: mcpcontract.NonNegativeInt(item.RetryAfterMS)}) + if len(failures) < maxJobArtifactItems { + failures = append(failures, mcpcontract.JobArtifactFailure{Reference: item.Key, Status: item.Status, Reason: item.Code, Message: item.Message, RetryAfterMS: mcpcontract.NonNegativeInt(item.RetryAfterMS)}) + } else { + failuresTruncated = true + } } - artifact := mcpcontract.JobArtifactReference{Kind: "pull_request_feedback_index", Count: ptrNonNegative(len(refs)), References: refs, ReferencesTruncated: len(result.Items) > len(refs), Failures: failures, Status: result.Status, DiscoveryStatus: result.DiscoveryStatus, Recovery: result.Recovery} + artifact := mcpcontract.JobArtifactReference{Kind: "pull_request_feedback_index", Count: ptrNonNegative(completed), References: refs, ReferencesTruncated: referencesTruncated, Failures: failures, FailuresTruncated: failuresTruncated, Status: result.Status, DiscoveryStatus: result.DiscoveryStatus, Recovery: result.Recovery} follow := &mcpcontract.JobFollowUp{Action: mcpcontract.FollowUpAction{Type: "search_pull_request_feedback", SearchFeedback: &mcpcontract.SearchPullRequestFeedbackInput{Repository: request.Repository}}, Reason: "Search the indexed pull-request feedback through the offline corpus."} return []mcpcontract.JobArtifactReference{artifact}, follow } @@ -373,15 +438,20 @@ func indexRepositoriesJobArtifact(job *contracts.JobResult) ([]mcpcontract.JobAr return nil, nil } artifacts := make([]mcpcontract.JobArtifactReference, 0, len(result.Items)) - completedRefs := make([]string, 0, min(len(result.Items), 100)) - failures := make([]mcpcontract.JobArtifactFailure, 0, min(len(result.Items), 100)) + completedRefs := make([]string, 0, min(len(result.Items), maxJobArtifactItems)) + failures := make([]mcpcontract.JobArtifactFailure, 0, min(len(result.Items), maxJobArtifactItems)) + completed := 0 + referencesTruncated := false + failuresTruncated := false for _, item := range result.Items { if item.Status != "complete" { - if len(failures) < 100 { + if len(failures) < maxJobArtifactItems { failures = append(failures, mcpcontract.JobArtifactFailure{ Reference: item.Key, Status: mcpcontract.BatchItemStatus(item.Status), Reason: item.Reason, Message: item.Message, RetryAfterMS: mcpcontract.NonNegativeInt(item.RetryAfterMS), }) + } else { + failuresTruncated = true } continue } @@ -395,6 +465,7 @@ func indexRepositoriesJobArtifact(job *contracts.JobResult) ([]mcpcontract.JobAr if item.ArtifactDigest == "" { continue } + completed++ artifact := mcpcontract.CodeIndexArtifact{Kind: "code_index", ID: "code-index:" + item.ArtifactDigest, Repository: mcpcontract.RepositoryRef{Owner: owner, Repo: repo}, CommitSHA: item.CommitSHA, SnapshotToken: item.SnapshotToken, @@ -404,15 +475,17 @@ func indexRepositoriesJobArtifact(job *contracts.JobResult) ([]mcpcontract.JobAr artifact.SnapshotToken = result.SnapshotToken } artifacts = append(artifacts, mcpcontract.JobArtifactReference{Kind: artifact.Kind, ID: artifact.ID, URI: artifact.ResourceURI, CodeIndex: &artifact}) - if len(completedRefs) < 100 { + if len(completedRefs) < maxJobArtifactItems { completedRefs = append(completedRefs, item.Key) + } else { + referencesTruncated = true } } if len(failures) > 0 { - count := mcpcontract.NonNegativeInt(len(completedRefs)) + count := mcpcontract.NonNegativeInt(completed) artifacts = append(artifacts, mcpcontract.JobArtifactReference{ Kind: "repository_batch", Count: &count, References: completedRefs, - ReferencesTruncated: len(result.Items) > len(completedRefs), Failures: failures, + ReferencesTruncated: referencesTruncated, Failures: failures, FailuresTruncated: failuresTruncated, }) } return artifacts, firstCodeIndexFollowUp(artifacts) diff --git a/internal/app/mcp_jobs.go b/internal/app/mcp_jobs.go index a8f9e92..e9b5614 100644 --- a/internal/app/mcp_jobs.go +++ b/internal/app/mcp_jobs.go @@ -191,20 +191,34 @@ func jobResultStatus(job *contracts.JobResult) string { } func portfolioReadFollowUpArguments(request mcpcontract.SyncPortfolioInput, login string, references []string) *mcpcontract.ListPullRequestPortfolioInput { + if request.Selection == "" { + // Legacy portfolio jobs predate the required selection discriminator. + // An observed login proves authored discovery; otherwise preserve the + // exact result references instead of widening the offline reread. + if login != "" { + request.Selection = "authored" + } else if refs := portfolioResultRefs(references); len(refs) > 0 { + return &mcpcontract.ListPullRequestPortfolioInput{PullRequests: refs, View: "compact"} + } else { + return nil + } + } if request.Selection == "explicit" { - return &mcpcontract.ListPullRequestPortfolioInput{PullRequests: portfolioResultRefs(references), View: "compact"} + refs := portfolioResultRefs(references) + if len(refs) == 0 { + return nil + } + return &mcpcontract.ListPullRequestPortfolioInput{PullRequests: refs, View: "compact"} } limit := request.Limit state := request.State if request.Selection == "authored" { if login != "" { - return &mcpcontract.ListPullRequestPortfolioInput{Authors: []string{login}, State: state, Limit: limit, View: "compact"} + return &mcpcontract.ListPullRequestPortfolioInput{Repository: request.Repository, Authors: []string{login}, State: state, Limit: limit, View: "compact"} } + return nil } - if limit == 0 { - limit = 20 - } - return &mcpcontract.ListPullRequestPortfolioInput{State: state, Limit: limit, View: "compact"} + return nil } func facetBatchArtifact(refs []mcpcontract.ThreadRef, facetNames []string) ([]mcpcontract.JobArtifactReference, *mcpcontract.JobFollowUp) { diff --git a/internal/app/mcp_jobs_test.go b/internal/app/mcp_jobs_test.go index 5c0b858..a582002 100644 --- a/internal/app/mcp_jobs_test.go +++ b/internal/app/mcp_jobs_test.go @@ -2,6 +2,7 @@ package app import ( "encoding/json" + "fmt" "strings" "testing" @@ -9,6 +10,167 @@ import ( "github.com/morluto/gitcontribute/internal/mcpcontract" ) +func TestFeedbackIndexArtifactBoundsReferencesAndKeepsCount(t *testing.T) { + t.Parallel() + items := make([]pullRequestFeedbackIndexItem, 0, maxJobArtifactItems+1) + for number := 1; number <= maxJobArtifactItems+1; number++ { + items = append(items, pullRequestFeedbackIndexItem{Key: fmt.Sprintf("acme/rocket/pull_request#%d", number), Status: "complete"}) + } + result, err := json.Marshal(pullRequestFeedbackIndexResult{Status: "complete", DiscoveryStatus: "complete", Items: items}) + if err != nil { + t.Fatal(err) + } + request, err := json.Marshal(mcpcontract.IndexPullRequestFeedbackInput{Repository: mcpcontract.RepositoryRef{Owner: "acme", Repo: "rocket"}}) + if err != nil { + t.Fatal(err) + } + artifacts, _ := pullRequestFeedbackIndexJobArtifact(&contracts.JobResult{Kind: jobKindIndexPullRequestFeedback, Request: string(request), Result: string(result)}) + if len(artifacts) != 1 { + t.Fatalf("artifacts = %+v", artifacts) + } + artifact := artifacts[0] + if artifact.Count == nil || int(*artifact.Count) != maxJobArtifactItems+1 || len(artifact.References) != maxJobArtifactItems || !artifact.ReferencesTruncated { + t.Fatalf("bounded feedback-index artifact = %+v", artifact) + } +} + +func TestFeedbackIndexArtifactSignalsBoundedFailures(t *testing.T) { + t.Parallel() + items := make([]pullRequestFeedbackIndexItem, 0, maxJobArtifactItems+1) + for number := 1; number <= maxJobArtifactItems+1; number++ { + items = append(items, pullRequestFeedbackIndexItem{Key: fmt.Sprintf("acme/rocket/pull_request#%d", number), Status: "failed", Code: "transient", Message: "retry later"}) + } + result, err := json.Marshal(pullRequestFeedbackIndexResult{Status: "partial", DiscoveryStatus: "partial", Items: items}) + if err != nil { + t.Fatal(err) + } + request, err := json.Marshal(mcpcontract.IndexPullRequestFeedbackInput{Repository: mcpcontract.RepositoryRef{Owner: "acme", Repo: "rocket"}}) + if err != nil { + t.Fatal(err) + } + artifacts, _ := pullRequestFeedbackIndexJobArtifact(&contracts.JobResult{Kind: jobKindIndexPullRequestFeedback, Request: string(request), Result: string(result)}) + if len(artifacts) != 1 || len(artifacts[0].Failures) != maxJobArtifactItems || !artifacts[0].FailuresTruncated { + t.Fatalf("bounded feedback-index failures = %+v", artifacts) + } +} + +func TestRepositoryBatchArtifactDoesNotCallFailuresReferenceTruncation(t *testing.T) { + t.Parallel() + job := &contracts.JobResult{Kind: "sync_repository_context", Result: `{"items":[{"key":"acme/rocket","status":"complete"},{"key":"acme/missing","status":"failed","reason":"not_found"}]}`} + artifacts, _ := repositoryBatchJobArtifact(job, 2) + if len(artifacts) != 1 || artifacts[0].ReferencesTruncated || len(artifacts[0].References) != 1 || len(artifacts[0].Failures) != 1 { + t.Fatalf("repository batch artifact = %+v", artifacts) + } +} + +func TestRepositoryBatchArtifactSignalsBoundedFailures(t *testing.T) { + t.Parallel() + items := make([]syncBatchItem, maxJobArtifactItems+1) + for i := range items { + items[i] = syncBatchItem{Key: fmt.Sprintf("acme/repo-%d", i), Status: "failed", Reason: "transient"} + } + result, err := json.Marshal(syncBatchResult{Items: items}) + if err != nil { + t.Fatal(err) + } + artifacts, _ := repositoryBatchJobArtifact(&contracts.JobResult{Kind: "sync_repository_context", Result: string(result)}, len(items)) + if len(artifacts) != 1 || len(artifacts[0].Failures) != maxJobArtifactItems || !artifacts[0].FailuresTruncated { + t.Fatalf("bounded repository failures = %+v", artifacts) + } +} + +func TestWorkflowArtifactBoundsPersistedTerminalLists(t *testing.T) { + t.Parallel() + items := make([]pullRequestWorkflowItem, 0, 2*maxJobArtifactItems+2) + for i := 0; i <= maxJobArtifactItems; i++ { + items = append(items, pullRequestWorkflowItem{Key: fmt.Sprintf("acme/rocket/pull_request#%d", i+1), Status: "complete", ResourceURI: fmt.Sprintf("gitcontribute://pull-request-feedback/acme/rocket/%d", i+1)}) + } + for i := 0; i <= maxJobArtifactItems; i++ { + items = append(items, pullRequestWorkflowItem{Key: fmt.Sprintf("acme/failed/pull_request#%d", i+1), Status: "failed", Code: "transient"}) + } + result, err := json.Marshal(pullRequestWorkflowResult{BatchStatus: "partial", Items: items}) + if err != nil { + t.Fatal(err) + } + artifacts, _ := pullRequestWorkflowJobArtifact(&contracts.JobResult{Kind: "sync_pull_request_feedback", Result: string(result)}) + if len(artifacts) != 1 { + t.Fatalf("artifacts = %+v", artifacts) + } + artifact := artifacts[0] + if artifact.Count == nil || int(*artifact.Count) != maxJobArtifactItems+1 || len(artifact.References) != maxJobArtifactItems || !artifact.ReferencesTruncated || len(artifact.Failures) != maxJobArtifactItems || !artifact.FailuresTruncated { + t.Fatalf("bounded workflow artifact = %+v", artifact) + } +} + +func TestPortfolioArtifactBoundsPersistedTerminalLists(t *testing.T) { + t.Parallel() + refs := make([]string, maxJobArtifactItems+1) + failures := make([]pullRequestStatusFailure, maxJobArtifactItems+1) + for i := range refs { + refs[i] = fmt.Sprintf("acme/rocket/pull_request#%d", i+1) + failures[i] = pullRequestStatusFailure{Reference: refs[i], Status: "failed", Reason: "transient"} + } + result, err := json.Marshal(syncPortfolioResult{Status: "partial", PullRequests: refs, Failures: failures}) + if err != nil { + t.Fatal(err) + } + artifacts, _ := portfolioJobArtifact(&contracts.JobResult{Kind: jobKindSyncPullRequestPortfolio, Request: `{"selection":"explicit"}`, Result: string(result)}) + if len(artifacts) != 1 { + t.Fatalf("artifacts = %+v", artifacts) + } + artifact := artifacts[0] + if len(artifact.References) != maxJobArtifactItems || !artifact.ReferencesTruncated || len(artifact.Failures) != maxJobArtifactItems || !artifact.FailuresTruncated { + t.Fatalf("bounded portfolio artifact = %+v", artifact) + } + if artifact.Recovery == nil || len(artifact.Recovery.Then) != 1 || artifact.Recovery.Then[0].SyncPortfolio == nil || len(artifact.Recovery.Then[0].SyncPortfolio.PullRequests) != maxJobArtifactItems { + t.Fatalf("portfolio recovery exceeds bounded artifact scope: %+v", artifact.Recovery) + } +} + +func TestPortfolioArtifactOmitsExplicitRecoveryWithoutUsableReferences(t *testing.T) { + t.Parallel() + result, err := json.Marshal(syncPortfolioResult{ + Status: "partial", + PullRequests: []string{"malformed"}, + }) + if err != nil { + t.Fatal(err) + } + artifacts, _ := portfolioJobArtifact(&contracts.JobResult{ + Kind: jobKindSyncPullRequestPortfolio, + Request: `{"selection":"explicit","pull_requests":[{"owner":"acme","repo":"rocket","number":7}]}`, + Result: string(result), + }) + if len(artifacts) != 1 { + t.Fatalf("artifacts = %+v", artifacts) + } + if artifacts[0].Recovery != nil { + t.Fatalf("recovery without usable references = %+v", artifacts[0].Recovery) + } +} + +func TestCodeIndexBatchArtifactDoesNotCallFailuresReferenceTruncation(t *testing.T) { + t.Parallel() + result, err := json.Marshal(indexJobResult{Items: []indexJobItem{ + {Key: "acme/rocket", Status: "complete", CommitSHA: "abc123", ArtifactDigest: "artifact"}, + {Key: "acme/missing", Status: "failed", Reason: "not_found"}, + }}) + if err != nil { + t.Fatal(err) + } + artifacts, _ := indexRepositoriesJobArtifact(&contracts.JobResult{Kind: "index_repositories", Result: string(result)}) + for _, artifact := range artifacts { + if artifact.Kind != "repository_batch" { + continue + } + if artifact.ReferencesTruncated || len(artifact.References) != 1 || len(artifact.Failures) != 1 { + t.Fatalf("code-index batch artifact = %+v", artifact) + } + return + } + t.Fatalf("missing code-index batch artifact: %+v", artifacts) +} + func TestJobExecutionSeparatesRunningStateFromTerminalOutcome(t *testing.T) { t.Parallel() tests := []struct { @@ -124,14 +286,14 @@ func TestPortfolioFollowUpUsesPortfolioReadArguments(t *testing.T) { t.Parallel() job := &contracts.JobResult{ Kind: jobKindSyncPullRequestPortfolio, Status: "succeeded", - Request: `{"selection":"authored","state":"closed","limit":10}`, + Request: `{"selection":"authored","repository":{"owner":"acme","repo":"rocket"},"state":"closed","limit":10}`, Result: `{"status":"complete","login":"alice","pull_requests":["acme/rocket/pull_request#7"],"refreshed":1}`, } _, follow := jobArtifactsAndFollowUp(job, 1) if follow == nil || follow.Action.Type != "list_pull_request_portfolio" || follow.Action.ListPortfolio == nil { t.Fatalf("portfolio handoff = %+v", follow) } - if len(follow.Action.ListPortfolio.Authors) != 1 || follow.Action.ListPortfolio.Authors[0] != "alice" || follow.Action.ListPortfolio.State != "closed" || follow.Action.ListPortfolio.Limit != 10 || follow.Action.ListPortfolio.View != "compact" { + if follow.Action.ListPortfolio.Repository == nil || follow.Action.ListPortfolio.Repository.Owner != "acme" || follow.Action.ListPortfolio.Repository.Repo != "rocket" || len(follow.Action.ListPortfolio.Authors) != 1 || follow.Action.ListPortfolio.Authors[0] != "alice" || follow.Action.ListPortfolio.State != "closed" || follow.Action.ListPortfolio.Limit != 10 || follow.Action.ListPortfolio.View != "compact" { t.Fatalf("portfolio follow-up arguments = %+v", follow.Action) } } @@ -153,6 +315,81 @@ func TestExplicitPortfolioFollowUpPreservesExactReferences(t *testing.T) { } } +func TestLegacyAuthoredPortfolioFollowUpUsesObservedLogin(t *testing.T) { + t.Parallel() + job := &contracts.JobResult{ + Kind: jobKindSyncPullRequestPortfolio, Status: "succeeded", + Request: `{}`, + Result: `{"status":"complete","login":"alice","pull_requests":["acme/rocket/pull_request#7"],"refreshed":1,"discovery_status":"complete"}`, + } + _, follow := jobArtifactsAndFollowUp(job, 1) + if follow == nil || follow.Action.ListPortfolio == nil || len(follow.Action.ListPortfolio.Authors) != 1 || follow.Action.ListPortfolio.Authors[0] != "alice" { + t.Fatalf("legacy authored portfolio handoff = %+v", follow) + } +} + +func TestLegacyExplicitPortfolioFollowUpPreservesResultReferences(t *testing.T) { + t.Parallel() + job := &contracts.JobResult{ + Kind: jobKindSyncPullRequestPortfolio, Status: "succeeded", + Request: `{}`, + Result: `{"status":"complete","pull_requests":["acme/rocket/pull_request#7"],"refreshed":1,"discovery_status":"complete"}`, + } + _, follow := jobArtifactsAndFollowUp(job, 1) + if follow == nil || follow.Action.ListPortfolio == nil || len(follow.Action.ListPortfolio.PullRequests) != 1 { + t.Fatalf("legacy explicit portfolio handoff = %+v", follow) + } +} + +func TestPortfolioFollowUpOmitsUnprovenScope(t *testing.T) { + t.Parallel() + for _, job := range []*contracts.JobResult{ + { + Kind: jobKindSyncPullRequestPortfolio, Status: "succeeded", + Request: `{"selection":"authored","state":"closed","limit":10}`, + Result: `{"status":"complete","pull_requests":["acme/rocket/pull_request#7"],"refreshed":1}`, + }, + { + Kind: jobKindSyncPullRequestPortfolio, Status: "succeeded", + Request: `{"selection":"explicit","pull_requests":[{"owner":"acme","repo":"rocket","kind":"pull_request","number":7}]}`, + Result: `{"status":"complete","pull_requests":["not-a-pull-request"],"refreshed":1}`, + }, + { + Kind: jobKindSyncPullRequestPortfolio, Status: "succeeded", + Request: `{}`, + Result: `{"status":"complete","pull_requests":["not-a-pull-request"],"refreshed":1}`, + }, + } { + _, follow := jobArtifactsAndFollowUp(job, 1) + if follow != nil { + t.Fatalf("unproven portfolio scope yielded follow-up = %+v", follow) + } + } +} + +func TestJobArtifactsOmitFollowUpWhenRequestCannotProveScope(t *testing.T) { + t.Parallel() + for _, job := range []*contracts.JobResult{ + { + Kind: "sync_repository_context", + Status: "succeeded", + Request: `not-json`, + Result: `{"items":[{"key":"acme/rocket","status":"complete"}]}`, + }, + { + Kind: jobKindSyncThreadFacets, + Status: "succeeded", + Request: `not-json`, + Result: `{"status":"complete"}`, + }, + } { + _, follow := jobArtifactsAndFollowUp(job, 1) + if follow != nil { + t.Fatalf("unproven job scope yielded follow-up = %+v", follow) + } + } +} + func TestIndexJobPreservesFailuresAndBindsCompletedArtifactsToSnapshot(t *testing.T) { t.Parallel() job := &contracts.JobResult{ diff --git a/internal/app/mcp_local_repository_search.go b/internal/app/mcp_local_repository_search.go index ab98d7e..a2b500b 100644 --- a/internal/app/mcp_local_repository_search.go +++ b/internal/app/mcp_local_repository_search.go @@ -11,6 +11,7 @@ import ( // SearchRepositories performs a local-only repository search. func (r *MCPReader) SearchRepositories(ctx context.Context, in mcpcontract.SearchRepositoriesInput) (mcpcontract.SearchRepositoriesOutput, error) { + in.Query = strings.TrimSpace(in.Query) repoRef := domain.RepoRef{Owner: in.Owner, Repo: in.Repo} repoFilter := "" if in.Owner != "" || in.Repo != "" { diff --git a/internal/app/mcp_portfolio_reads.go b/internal/app/mcp_portfolio_reads.go new file mode 100644 index 0000000..b91f440 --- /dev/null +++ b/internal/app/mcp_portfolio_reads.go @@ -0,0 +1,150 @@ +package app + +import ( + "context" + "errors" + "strings" + + "github.com/morluto/gitcontribute/internal/corpus" + "github.com/morluto/gitcontribute/internal/domain" + "github.com/morluto/gitcontribute/internal/mcpcontract" +) + +// ListPullRequestPortfolio performs an offline projection over stored authored +// PRs and status facets; unsupported health facets remain explicitly unknown. +func (r *MCPReader) ListPullRequestPortfolio(ctx context.Context, in mcpcontract.ListPullRequestPortfolioInput) (mcpcontract.ListPullRequestPortfolioOutput, error) { + if len(in.Authors) > 1 { + return mcpcontract.ListPullRequestPortfolioOutput{}, errors.New("authors must contain at most one item") + } + if len(in.PullRequests) > 0 { + if len(in.PullRequests) > 100 { + return mcpcontract.ListPullRequestPortfolioOutput{}, errors.New("pull_requests must contain at most 100 items") + } + if in.Repository != nil || len(in.Authors) > 0 || in.State != "" || in.Limit != 0 { + return mcpcontract.ListPullRequestPortfolioOutput{}, errors.New("pull_requests cannot be combined with repository, authors, state, or limit") + } + in.PullRequests = canonicalPullRequestRefs(in.PullRequests) + if err := rejectDuplicateThreadRefs(in.PullRequests); err != nil { + return mcpcontract.ListPullRequestPortfolioOutput{}, err + } + if err := validatePullRequestRefs(in.PullRequests, "pull_requests"); err != nil { + return mcpcontract.ListPullRequestPortfolioOutput{}, err + } + } + if in.Repository != nil { + in.Repository.Owner = strings.TrimSpace(in.Repository.Owner) + in.Repository.Repo = strings.TrimSpace(in.Repository.Repo) + if err := (domain.RepoRef{Owner: in.Repository.Owner, Repo: in.Repository.Repo}).Validate(); err != nil { + return mcpcontract.ListPullRequestPortfolioOutput{}, err + } + } + if in.State == "" { + in.State = "open" + } + if in.State != "open" && in.State != "closed" && in.State != "all" { + return mcpcontract.ListPullRequestPortfolioOutput{}, errors.New("state must be open, closed, or all") + } + if in.View == "" { + in.View = "compact" + } + if in.View != "compact" && in.View != "full" { + return mcpcontract.ListPullRequestPortfolioOutput{}, errors.New("view must be compact or full") + } + if in.Limit == 0 && len(in.PullRequests) == 0 { + in.Limit = 20 + } + if len(in.PullRequests) == 0 && (in.Limit < 1 || in.Limit > 100) { + return mcpcontract.ListPullRequestPortfolioOutput{}, errors.New("limit must be between 1 and 100") + } + c, err := r.openReadOnlyCorpus(ctx) + if err != nil { + return mcpcontract.ListPullRequestPortfolioOutput{}, err + } + revision, err := beginCorpusRead(ctx, c, in.SnapshotToken) + if err != nil { + return mcpcontract.ListPullRequestPortfolioOutput{}, err + } + page, unavailable, err := portfolioPage(ctx, c, in) + if err != nil { + return mcpcontract.ListPullRequestPortfolioOutput{}, err + } + format := portfolioResponseFormat(map[string]string{"compact": "concise", "full": "detailed"}[in.View]) + readSet, err := loadPortfolioReadSet(ctx, c, page.PullRequests, format) + if err != nil { + return mcpcontract.ListPullRequestPortfolioOutput{}, err + } + out := mcpcontract.ListPullRequestPortfolioOutput{Status: "complete", View: in.View, RuleVersion: "portfolio.v2", GeneratedAt: formatTime(r.now()), PullRequests: make([]mcpcontract.PullRequestPortfolioItem, 0, len(page.PullRequests)), Total: page.Total, Truncated: page.Truncated, UnavailablePullRequests: unavailable, SnapshotToken: snapshotIdentity(in.SnapshotToken, revision)} + if len(unavailable) > 0 { + out.Status = "partial" + out.Recovery = recoveryPlan("portfolio_items_unavailable", "Some exact pull requests are not present in the local corpus. Refresh those exact pull requests, then reread the portfolio.", mcpcontract.RecoveryAction(mcpcontract.SyncPortfolioInput{Selection: "explicit", PullRequests: append([]mcpcontract.ThreadRef(nil), unavailable...)})) + } + for _, storedPR := range page.PullRequests { + item, err := portfolioItem(storedPR, r.now(), readSet, format) + if err != nil { + return mcpcontract.ListPullRequestPortfolioOutput{}, err + } + if item.StatusCoverage != "complete" { + out.Status = "partial" + item.Recovery = recoveryPlan("portfolio_facet_incomplete", "Refresh the incomplete pull-request health facets, then reread this exact portfolio item.", syncPullRequestCalls([]mcpcontract.ThreadRef{{Owner: item.Owner, Repo: item.Repo, Kind: "pull_request", Number: item.Number}})...) + } + out.PullRequests = append(out.PullRequests, item) + } + if err := finishCorpusRead(ctx, c, revision); err != nil { + return mcpcontract.ListPullRequestPortfolioOutput{}, err + } + if out.Truncated { + out.Status = "partial" + nextLimit := min(100, max(in.Limit*2, in.Limit+1)) + out.Recovery = recoveryPlan("portfolio_truncated", "The portfolio page is bounded. Read the next larger page before treating the returned set as exhaustive.", mcpcontract.RecoveryAction(mcpcontract.ListPullRequestPortfolioInput{Repository: in.Repository, Authors: append([]string(nil), in.Authors...), State: in.State, Limit: nextLimit, View: in.View, SnapshotToken: in.SnapshotToken})) + } + return out, nil +} + +func portfolioPage(ctx context.Context, c *corpus.Corpus, in mcpcontract.ListPullRequestPortfolioInput) (corpus.PortfolioPage, []mcpcontract.ThreadRef, error) { + if len(in.PullRequests) == 0 { + author := "" + if len(in.Authors) > 0 { + author = strings.TrimSpace(in.Authors[0]) + } + var repository *corpus.RepositoryKey + if in.Repository != nil { + repository = &corpus.RepositoryKey{Owner: in.Repository.Owner, Name: in.Repository.Repo} + } + page, err := c.ListPullRequestPortfolioPage(ctx, author, in.State, repository, in.Limit) + return page, nil, err + } + repositoryKeys := make([]corpus.RepositoryKey, 0, len(in.PullRequests)) + for _, ref := range in.PullRequests { + repositoryKeys = append(repositoryKeys, corpus.RepositoryKey{Owner: ref.Owner, Name: ref.Repo}) + } + repositories, err := c.GetRepositoriesBatch(ctx, repositoryKeys) + if err != nil { + return corpus.PortfolioPage{}, nil, err + } + threadKeys := make([]corpus.ThreadKey, 0, len(in.PullRequests)) + for _, ref := range in.PullRequests { + if repository := repositories[corpus.RepositoryKey{Owner: ref.Owner, Name: ref.Repo}]; repository != nil { + threadKeys = append(threadKeys, corpus.ThreadKey{RepositoryID: repository.ID, Kind: corpus.ThreadKindPullRequest, Number: ref.Number}) + } + } + threads, err := c.GetThreadsBatch(ctx, threadKeys) + if err != nil { + return corpus.PortfolioPage{}, nil, err + } + page := corpus.PortfolioPage{PullRequests: make([]corpus.PortfolioPullRequest, 0, len(in.PullRequests)), Total: len(in.PullRequests)} + unavailable := make([]mcpcontract.ThreadRef, 0) + for _, ref := range in.PullRequests { + repository := repositories[corpus.RepositoryKey{Owner: ref.Owner, Name: ref.Repo}] + if repository == nil { + unavailable = append(unavailable, ref) + continue + } + thread := threads[corpus.ThreadKey{RepositoryID: repository.ID, Kind: corpus.ThreadKindPullRequest, Number: ref.Number}] + if thread == nil { + unavailable = append(unavailable, ref) + continue + } + page.PullRequests = append(page.PullRequests, corpus.PortfolioPullRequest{Owner: repository.Owner, Repo: repository.Name, Thread: *thread}) + } + return page, unavailable, nil +} diff --git a/internal/app/mcp_portfolio_refs.go b/internal/app/mcp_portfolio_refs.go new file mode 100644 index 0000000..9b702df --- /dev/null +++ b/internal/app/mcp_portfolio_refs.go @@ -0,0 +1,19 @@ +package app + +import ( + "github.com/morluto/gitcontribute/internal/corpus" + "github.com/morluto/gitcontribute/internal/mcpcontract" +) + +// canonicalPullRequestRefs makes the optional kind explicit before callers +// compare references. A blank kind means pull_request for portfolio operations, +// so it must not create a second identity for the same pull request. +func canonicalPullRequestRefs(inputs []mcpcontract.ThreadRef) []mcpcontract.ThreadRef { + refs := append([]mcpcontract.ThreadRef(nil), inputs...) + for i := range refs { + if refs[i].Kind == "" { + refs[i].Kind = corpus.ThreadKindPullRequest + } + } + return refs +} diff --git a/internal/app/mcp_portfolio_sync.go b/internal/app/mcp_portfolio_sync.go index eab9364..21d9bfa 100644 --- a/internal/app/mcp_portfolio_sync.go +++ b/internal/app/mcp_portfolio_sync.go @@ -4,131 +4,158 @@ import ( "context" "errors" "fmt" + "strings" "time" + "github.com/morluto/gitcontribute/internal/domain" "github.com/morluto/gitcontribute/internal/mcpcontract" ) // SyncPortfolio submits one bounded job that discovers pull requests authored // by the active credential and refreshes health for the resulting stored set. func (r *MCPReader) SyncPortfolio(ctx context.Context, in mcpcontract.SyncPortfolioInput) (mcpcontract.JobReference, error) { + in, err := normalizeSyncPortfolioInput(in) + if err != nil { + return mcpcontract.JobReference{}, err + } + id, err := r.submitJob(ctx, jobKindSyncPullRequestPortfolio, in, func(ctx context.Context, report func(string, string) error) (any, error) { + return r.runPortfolioSync(ctx, in, report) + }) + if err != nil { + return mcpcontract.JobReference{}, err + } + return queuedJobReference(id, jobKindSyncPullRequestPortfolio, "portfolio synchronization job started"), nil +} + +func normalizeSyncPortfolioInput(in mcpcontract.SyncPortfolioInput) (mcpcontract.SyncPortfolioInput, error) { if in.Selection == "" { - return mcpcontract.JobReference{}, errors.New("selection is required: choose authored or explicit") + return mcpcontract.SyncPortfolioInput{}, errors.New("selection is required: choose authored or explicit") } if in.Selection != "authored" && in.Selection != "explicit" { - return mcpcontract.JobReference{}, errors.New("selection must be authored or explicit") + return mcpcontract.SyncPortfolioInput{}, errors.New("selection must be authored or explicit") } if in.Selection == "explicit" { - if len(in.PullRequests) < 1 || len(in.PullRequests) > 100 { - return mcpcontract.JobReference{}, errors.New("pull_requests must contain 1 to 100 items in explicit mode") - } - if err := rejectDuplicateThreadRefs(in.PullRequests); err != nil { - return mcpcontract.JobReference{}, err - } - if err := validatePullRequestRefs(in.PullRequests, "pull_requests"); err != nil { - return mcpcontract.JobReference{}, err - } - if in.State != "" || in.UpdatedAfter != "" || in.Limit != 0 || in.DiscoveryMaxRequests != 0 { - return mcpcontract.JobReference{}, errors.New("state, updated_after, limit, and discovery_max_requests are only valid in authored mode") + return normalizeExplicitPortfolioInput(in) + } + if len(in.PullRequests) > 0 { + return mcpcontract.SyncPortfolioInput{}, errors.New("pull_requests is only valid in explicit mode") + } + return normalizeAuthoredPortfolioInput(in) +} + +func normalizeExplicitPortfolioInput(in mcpcontract.SyncPortfolioInput) (mcpcontract.SyncPortfolioInput, error) { + if len(in.PullRequests) < 1 || len(in.PullRequests) > 100 { + return mcpcontract.SyncPortfolioInput{}, errors.New("pull_requests must contain 1 to 100 items in explicit mode") + } + in.PullRequests = canonicalPullRequestRefs(in.PullRequests) + if err := rejectDuplicateThreadRefs(in.PullRequests); err != nil { + return mcpcontract.SyncPortfolioInput{}, err + } + if err := validatePullRequestRefs(in.PullRequests, "pull_requests"); err != nil { + return mcpcontract.SyncPortfolioInput{}, err + } + if in.State != "" || in.UpdatedAfter != "" || in.Limit != 0 || in.DiscoveryMaxRequests != 0 { + return mcpcontract.SyncPortfolioInput{}, errors.New("state, updated_after, limit, and discovery_max_requests are only valid in authored mode") + } + if in.Repository != nil { + return mcpcontract.SyncPortfolioInput{}, errors.New("repository is only valid in authored mode") + } + return normalizePortfolioStatusMaxPages(in) +} + +func normalizeAuthoredPortfolioInput(in mcpcontract.SyncPortfolioInput) (mcpcontract.SyncPortfolioInput, error) { + if in.Repository != nil { + in.Repository.Owner = strings.TrimSpace(in.Repository.Owner) + in.Repository.Repo = strings.TrimSpace(in.Repository.Repo) + if err := (domain.RepoRef{Owner: in.Repository.Owner, Repo: in.Repository.Repo}).Validate(); err != nil { + return mcpcontract.SyncPortfolioInput{}, err } - } else if len(in.PullRequests) > 0 { - return mcpcontract.JobReference{}, errors.New("pull_requests is only valid in explicit mode") } - if in.Selection == "authored" && in.State == "" { + if in.State == "" { in.State = "open" } - if in.Selection == "authored" && in.State != "open" && in.State != "closed" && in.State != "all" { - return mcpcontract.JobReference{}, errors.New("state must be open, closed, or all") + if in.State != "open" && in.State != "closed" && in.State != "all" { + return mcpcontract.SyncPortfolioInput{}, errors.New("state must be open, closed, or all") } - if in.Selection == "authored" && in.UpdatedAfter != "" { + if in.UpdatedAfter != "" { if _, err := time.Parse(time.RFC3339, in.UpdatedAfter); err != nil { - return mcpcontract.JobReference{}, errors.New("updated_after must be RFC 3339") + return mcpcontract.SyncPortfolioInput{}, errors.New("updated_after must be RFC 3339") } } - if in.Selection == "authored" && in.Limit == 0 { + if in.Limit == 0 { in.Limit = 100 } - if in.Selection == "authored" && (in.Limit < 1 || in.Limit > 100) { - return mcpcontract.JobReference{}, errors.New("limit must be between 1 and 100") + if in.Limit < 1 || in.Limit > 100 { + return mcpcontract.SyncPortfolioInput{}, errors.New("limit must be between 1 and 100") } - if in.Selection == "authored" && in.DiscoveryMaxRequests == 0 { + if in.DiscoveryMaxRequests == 0 { in.DiscoveryMaxRequests = defaultSyncBatchMaxRequests } - if in.Selection == "authored" && (in.DiscoveryMaxRequests < 2 || in.DiscoveryMaxRequests > defaultSyncBatchMaxRequests) { - return mcpcontract.JobReference{}, fmt.Errorf("discovery_max_requests must be between 2 and %d", defaultSyncBatchMaxRequests) + if in.DiscoveryMaxRequests < 2 || in.DiscoveryMaxRequests > defaultSyncBatchMaxRequests { + return mcpcontract.SyncPortfolioInput{}, fmt.Errorf("discovery_max_requests must be between 2 and %d", defaultSyncBatchMaxRequests) } + return normalizePortfolioStatusMaxPages(in) +} + +func normalizePortfolioStatusMaxPages(in mcpcontract.SyncPortfolioInput) (mcpcontract.SyncPortfolioInput, error) { if in.StatusMaxPages == 0 { in.StatusMaxPages = 3 } if in.StatusMaxPages < 1 || in.StatusMaxPages > 20 { - return mcpcontract.JobReference{}, errors.New("status_max_pages must be between 1 and 20") + return mcpcontract.SyncPortfolioInput{}, errors.New("status_max_pages must be between 1 and 20") } + return in, nil +} - id, err := r.submitJob(ctx, jobKindSyncPullRequestPortfolio, in, func(ctx context.Context, report func(string, string) error) (any, error) { - if in.Selection == "explicit" { - references := make([]string, len(in.PullRequests)) - for i, ref := range in.PullRequests { - if ref.Kind == "" { - ref.Kind = "pull_request" - } - references[i] = threadRefKey(ref) - } - refreshed := 0 - failures := make([]pullRequestStatusFailure, 0) - status := "complete" - for start := 0; start < len(in.PullRequests); start += 50 { - end := min(start+50, len(in.PullRequests)) - batch, err := r.syncPullRequestStatusBatch(ctx, pullRequestStatusBatchInput{PullRequests: in.PullRequests[start:end], MaxPages: in.StatusMaxPages}, report) - if err != nil { - return nil, err - } - refreshed += batch.Completed - failures = append(failures, batch.Failures...) - if batch.Status != "complete" { - status = "partial" - } - } - return syncPortfolioResult{Status: status, Discovered: len(in.PullRequests), Refreshed: refreshed, PullRequests: references, Failures: failures, DiscoveryStatus: "complete"}, nil - } - discovery, err := r.syncAuthoredPullRequests(ctx, authoredPullRequestSyncOptions{ - State: in.State, UpdatedAfter: in.UpdatedAfter, Limit: in.Limit, MaxRequests: in.DiscoveryMaxRequests, - }, report) +func (r *MCPReader) runPortfolioSync(ctx context.Context, in mcpcontract.SyncPortfolioInput, report func(string, string) error) (syncPortfolioResult, error) { + if in.Selection == "explicit" { + return r.syncExplicitPortfolio(ctx, in, report) + } + return r.syncAuthoredPortfolio(ctx, in, report) +} + +func (r *MCPReader) syncExplicitPortfolio(ctx context.Context, in mcpcontract.SyncPortfolioInput, report func(string, string) error) (syncPortfolioResult, error) { + refreshed, failures, status, err := r.syncPortfolioStatusBatches(ctx, in.PullRequests, in.StatusMaxPages, report) + if err != nil { + return syncPortfolioResult{}, err + } + return syncPortfolioResult{Status: status, Discovered: len(in.PullRequests), Refreshed: refreshed, PullRequests: threadRefKeys(in.PullRequests), Failures: failures, DiscoveryStatus: "complete"}, nil +} + +func (r *MCPReader) syncAuthoredPortfolio(ctx context.Context, in mcpcontract.SyncPortfolioInput, report func(string, string) error) (syncPortfolioResult, error) { + discovery, err := r.syncAuthoredPullRequests(ctx, authoredPullRequestSyncOptions{ + Repository: in.Repository, State: in.State, UpdatedAfter: in.UpdatedAfter, Limit: in.Limit, MaxRequests: in.DiscoveryMaxRequests, + }, report) + if err != nil { + return syncPortfolioResult{}, err + } + refreshed, failures, status, err := r.syncPortfolioStatusBatches(ctx, discovery.PullRequestTargets, in.StatusMaxPages, report) + if err != nil { + return syncPortfolioResult{}, err + } + if discovery.Status != "complete" || discovery.SearchIncomplete || discovery.RequestCapped { + status = "partial" + } + return syncPortfolioResult{Status: status, Login: discovery.Login, Discovered: discovery.PullRequests, Refreshed: refreshed, PullRequests: append([]string(nil), discovery.PullRequestRefs...), Failures: failures, DiscoveryStatus: discovery.Status, SearchIncomplete: discovery.SearchIncomplete, RequestCapped: discovery.RequestCapped}, nil +} + +func (r *MCPReader) syncPortfolioStatusBatches(ctx context.Context, refs []mcpcontract.ThreadRef, maxPages int, report func(string, string) error) (int, []pullRequestStatusFailure, string, error) { + refreshed := 0 + failures := make([]pullRequestStatusFailure, 0) + status := "complete" + for start := 0; start < len(refs); start += 50 { + end := min(start+50, len(refs)) + batch, err := r.syncPullRequestStatusBatch(ctx, pullRequestStatusBatchInput{PullRequests: refs[start:end], MaxPages: maxPages}, report) if err != nil { - return nil, err + return 0, nil, "", err } - refs := append([]mcpcontract.ThreadRef(nil), discovery.PullRequestTargets...) - references := append([]string(nil), discovery.PullRequestRefs...) - status := "complete" - if discovery.Status != "complete" || discovery.SearchIncomplete || discovery.RequestCapped { + refreshed += batch.Completed + failures = append(failures, batch.Failures...) + if batch.Status != "complete" { status = "partial" } - refreshed := 0 - failures := make([]pullRequestStatusFailure, 0) - for start := 0; start < len(refs); start += 50 { - end := min(start+50, len(refs)) - batch, err := r.syncPullRequestStatusBatch(ctx, pullRequestStatusBatchInput{ - PullRequests: refs[start:end], MaxPages: in.StatusMaxPages, - }, report) - if err != nil { - return nil, err - } - refreshed += batch.Completed - failures = append(failures, batch.Failures...) - if batch.Status != "complete" { - status = "partial" - } - } - return syncPortfolioResult{ - Status: status, Login: discovery.Login, Discovered: discovery.PullRequests, Refreshed: refreshed, - PullRequests: references, Failures: failures, - DiscoveryStatus: discovery.Status, SearchIncomplete: discovery.SearchIncomplete, - RequestCapped: discovery.RequestCapped, - }, nil - }) - if err != nil { - return mcpcontract.JobReference{}, err } - return queuedJobReference(id, jobKindSyncPullRequestPortfolio, "portfolio synchronization job started"), nil + return refreshed, failures, status, nil } type syncPortfolioResult struct { diff --git a/internal/app/mcp_portfolio_test.go b/internal/app/mcp_portfolio_test.go index da0ac1f..36745e2 100644 --- a/internal/app/mcp_portfolio_test.go +++ b/internal/app/mcp_portfolio_test.go @@ -174,3 +174,56 @@ func TestPullRequestPortfolioExactSelectionDoesNotSubstituteNewerPullRequests(t t.Fatalf("exact portfolio = %+v", out) } } + +func TestPullRequestPortfolioRepositoryScopePreservesTotalAndTruncationRecovery(t *testing.T) { + t.Parallel() + ctx := context.Background() + svc := newSearchTestService(t) + now := time.Unix(1000, 0).UTC() + selected, err := svc.corpus.UpsertRepository(ctx, corpus.Repository{Owner: "acme", Name: "rocket"}, `{}`) + if err != nil { + t.Fatal(err) + } + other, err := svc.corpus.UpsertRepository(ctx, corpus.Repository{Owner: "acme", Name: "other"}, `{}`) + if err != nil { + t.Fatal(err) + } + for _, thread := range []corpus.Thread{ + {RepositoryID: selected.ID, Kind: corpus.ThreadKindPullRequest, Number: 1, State: "open", Author: "alice", Title: "older selected", SourceUpdatedAt: now}, + {RepositoryID: selected.ID, Kind: corpus.ThreadKindPullRequest, Number: 2, State: "open", Author: "alice", Title: "newer selected", SourceUpdatedAt: now.Add(time.Second)}, + {RepositoryID: other.ID, Kind: corpus.ThreadKindPullRequest, Number: 3, State: "open", Author: "alice", Title: "newest other repository", SourceUpdatedAt: now.Add(2 * time.Second)}, + } { + if _, err := svc.corpus.UpsertThread(ctx, thread, `{}`); err != nil { + t.Fatal(err) + } + } + scope := &mcpcontract.RepositoryRef{Owner: "acme", Repo: "rocket"} + out, err := (&MCPReader{svc}).ListPullRequestPortfolio(ctx, mcpcontract.ListPullRequestPortfolioInput{Repository: scope, Authors: []string{"alice"}, State: "open", Limit: 1}) + if err != nil { + t.Fatal(err) + } + if out.Total != 2 || !out.Truncated || len(out.PullRequests) != 1 || out.PullRequests[0].Repo != "rocket" || out.PullRequests[0].Number != 2 { + t.Fatalf("scoped portfolio = %+v", out) + } + if out.Recovery == nil || len(out.Recovery.Then) != 1 || out.Recovery.Then[0].ListPortfolio == nil || out.Recovery.Then[0].ListPortfolio.Repository == nil || *out.Recovery.Then[0].ListPortfolio.Repository != *scope { + t.Fatalf("scoped truncation recovery = %+v", out.Recovery) + } + if _, err := (&MCPReader{svc}).ListPullRequestPortfolio(ctx, mcpcontract.ListPullRequestPortfolioInput{Repository: scope, PullRequests: []mcpcontract.ThreadRef{{Owner: "acme", Repo: "rocket", Kind: "pull_request", Number: 1}}}); err == nil { + t.Fatal("explicit portfolio selection accepted repository scope") + } +} + +func TestPullRequestPortfolioRejectsDuplicateDefaultKindReferences(t *testing.T) { + t.Parallel() + ctx := context.Background() + svc := newSearchTestService(t) + _, err := (&MCPReader{svc}).ListPullRequestPortfolio(ctx, mcpcontract.ListPullRequestPortfolioInput{ + PullRequests: []mcpcontract.ThreadRef{ + {Owner: "acme", Repo: "rocket", Number: 7}, + {Owner: "acme", Repo: "rocket", Kind: "pull_request", Number: 7}, + }, + }) + if err == nil { + t.Fatal("expected duplicate pull-request references to be rejected") + } +} diff --git a/internal/app/mcp_pr_workflows.go b/internal/app/mcp_pr_workflows.go index 1fb2575..a262cab 100644 --- a/internal/app/mcp_pr_workflows.go +++ b/internal/app/mcp_pr_workflows.go @@ -18,6 +18,7 @@ const ( facetPRFeedbackInlineComments = "pr_feedback_inline_comments" facetPRFeedbackReviewThreads = "pr_feedback_review_threads" facetPRCIReport = "pr_ci_report" + maxFeedbackItemsPerChannel = 1000 ) var ( @@ -64,7 +65,7 @@ func (r *MCPReader) SyncPullRequestFeedback(ctx context.Context, in mcpcontract. if in.MaxItemsPerChannel == 0 { in.MaxItemsPerChannel = 300 } - if in.MaxItemsPerChannel < 1 || in.MaxItemsPerChannel > 1000 { + if in.MaxItemsPerChannel < 1 || in.MaxItemsPerChannel > maxFeedbackItemsPerChannel { return mcpcontract.JobReference{}, errors.New("max_items_per_channel must be between 1 and 1000") } if in.MaxRequests == 0 { @@ -154,7 +155,7 @@ func (r *MCPReader) syncPullRequestFeedback(ctx context.Context, in mcpcontract. item.Status = "retryable" item.Code = "feedback_coverage_incomplete" item.Message = "one or more feedback channels reached max_items_per_channel" - item.Recovery = recoveryPlan("facet_incomplete", item.Message, mcpcontract.RecoveryAction(mcpcontract.SyncPullRequestFeedbackInput{PullRequests: []mcpcontract.ThreadRef{ref}, Channels: append([]string(nil), in.Channels...), ThreadState: in.ThreadState, MaxItemsPerChannel: in.MaxItemsPerChannel * 2, MaxRequests: in.MaxRequests})) + item.Recovery = feedbackCoverageRecovery(ref, in, item.Message) item.HeadSHA = snapshot.HeadSHA out.BatchStatus = "partial" } else { @@ -180,6 +181,17 @@ func (r *MCPReader) syncPullRequestFeedback(ctx context.Context, in mcpcontract. return out, nil } +func feedbackCoverageRecovery(ref mcpcontract.ThreadRef, in mcpcontract.SyncPullRequestFeedbackInput, message string) *mcpcontract.RecoveryPlan { + if in.MaxItemsPerChannel >= maxFeedbackItemsPerChannel { + return nil + } + next := min(maxFeedbackItemsPerChannel, max(in.MaxItemsPerChannel*2, in.MaxItemsPerChannel+1)) + return recoveryPlan("facet_incomplete", message, mcpcontract.RecoveryAction(mcpcontract.SyncPullRequestFeedbackInput{ + PullRequests: []mcpcontract.ThreadRef{ref}, Channels: append([]string(nil), in.Channels...), ThreadState: in.ThreadState, + MaxItemsPerChannel: next, MaxRequests: in.MaxRequests, + })) +} + // persistPullRequestIdentity stores only the repository and exact PR identity // needed by the feedback facets. It deliberately does not refresh repository // metadata or unrelated thread headers. diff --git a/internal/app/mcp_pr_workflows_test.go b/internal/app/mcp_pr_workflows_test.go index 2f6c1e9..494cbd9 100644 --- a/internal/app/mcp_pr_workflows_test.go +++ b/internal/app/mcp_pr_workflows_test.go @@ -234,6 +234,9 @@ func TestBoundedWorkflowSnapshotsReturnRetryablePartialItems(t *testing.T) { if feedback.BatchStatus != "partial" || feedback.Items[0].Status != "retryable" || feedback.Items[0].ResourceURI != "" { t.Fatalf("feedback result = %+v", feedback) } + if feedback.Items[0].Recovery == nil || len(feedback.Items[0].Recovery.Then) != 1 || feedback.Items[0].Recovery.Then[0].SyncFeedback == nil || feedback.Items[0].Recovery.Then[0].SyncFeedback.MaxItemsPerChannel != 20 { + t.Fatalf("feedback recovery = %+v", feedback.Items[0].Recovery) + } ci, err := reader.syncCIFailures(ctx, mcpcontract.SyncCIFailuresInput{ PullRequests: []mcpcontract.ThreadRef{ref}, MaxRunsPerPR: 1, MaxJobsPerRun: 1, @@ -268,6 +271,18 @@ func TestBoundedWorkflowSnapshotsReturnRetryablePartialItems(t *testing.T) { } } +func TestFeedbackCoverageRecoveryRespectsAdvertisedItemLimit(t *testing.T) { + t.Parallel() + ref := mcpcontract.ThreadRef{Owner: "acme", Repo: "rocket", Kind: "pull_request", Number: 7} + if plan := feedbackCoverageRecovery(ref, mcpcontract.SyncPullRequestFeedbackInput{MaxItemsPerChannel: maxFeedbackItemsPerChannel}, "incomplete"); plan != nil { + t.Fatalf("hard-limit recovery = %+v, want nil", plan) + } + plan := feedbackCoverageRecovery(ref, mcpcontract.SyncPullRequestFeedbackInput{MaxItemsPerChannel: maxFeedbackItemsPerChannel - 1}, "incomplete") + if plan == nil || len(plan.Then) != 1 || plan.Then[0].SyncFeedback == nil || plan.Then[0].SyncFeedback.MaxItemsPerChannel != maxFeedbackItemsPerChannel { + t.Fatalf("capped recovery = %+v", plan) + } +} + type boundedWorkflowReader struct { panicRadarReader feedback github.PullRequestFeedback @@ -353,3 +368,29 @@ func TestFeedbackSearchRecoveryRefreshesAllThreadState(t *testing.T) { t.Fatalf("feedback recovery plan = %+v", plan) } } + +func TestFeedbackSearchRecoveryBoundsMergeStateHydration(t *testing.T) { + unknown := make([]int, 0, 50) + for number := 1; number <= 50; number++ { + unknown = append(unknown, number) + } + items := make([]corpus.PullRequestFeedbackProjection, 0, 100) + for number := 51; number <= 150; number++ { + items = append(items, corpus.PullRequestFeedbackProjection{PullRequestNumber: number}) + } + plan := feedbackSearchRecovery(context.Background(), nil, 0, domain.RepoRef{Owner: "acme", Repo: "rocket"}, mcpcontract.SearchPullRequestFeedbackInput{}, corpus.FeedbackSearchPage{ + Coverage: corpus.FeedbackCoverageSummary{Status: "complete", DiscoveryComplete: true}, + UnknownMergePullRequests: unknown, + Items: items, + }) + if plan == nil || len(plan.Then) != 1 || plan.Then[0].HydrateThreads == nil { + t.Fatalf("merge-state recovery plan = %+v", plan) + } + threads := plan.Then[0].HydrateThreads.Threads + if len(threads) != maxFeedbackMergeStateRecoveryThreads { + t.Fatalf("recovery thread count = %d, want %d", len(threads), maxFeedbackMergeStateRecoveryThreads) + } + if threads[0].Number != 1 || threads[len(threads)-1].Number != 100 { + t.Fatalf("recovery thread bounds = first %d, last %d; want 1 through 100", threads[0].Number, threads[len(threads)-1].Number) + } +} diff --git a/internal/app/mcp_pull_request_feedback_index.go b/internal/app/mcp_pull_request_feedback_index.go index 922bd05..80d9687 100644 --- a/internal/app/mcp_pull_request_feedback_index.go +++ b/internal/app/mcp_pull_request_feedback_index.go @@ -262,7 +262,10 @@ func (r *MCPReader) indexOnePullRequestFeedback(ctx context.Context, feedbackRea } if readErr != nil { if len(snapshot.Coverage) > 0 { - _ = r.persistPullRequestFeedback(ctx, ref, snapshot, coveredFeedbackChannels(in.Channels, snapshot.Coverage)) + if persistErr := r.persistPullRequestFeedback(ctx, ref, snapshot, coveredFeedbackChannels(in.Channels, snapshot.Coverage)); persistErr != nil { + item.Status, item.Code, item.Message = "failed", "feedback_persistence_failed", persistErr.Error() + return item + } } item = pullRequestFeedbackIndexFailure(ref, in, readErr) item.HeadSHA = snapshot.HeadSHA diff --git a/internal/app/mcp_pull_request_feedback_index_test.go b/internal/app/mcp_pull_request_feedback_index_test.go index 2afe47f..48bd2a8 100644 --- a/internal/app/mcp_pull_request_feedback_index_test.go +++ b/internal/app/mcp_pull_request_feedback_index_test.go @@ -2,6 +2,7 @@ package app import ( "context" + "errors" "fmt" "testing" "time" @@ -17,6 +18,18 @@ type feedbackIndexTestReader struct { withThread bool } +type cancelledPartialFeedbackReader struct { + panicRadarReader + cancel context.CancelFunc +} + +func (r *cancelledPartialFeedbackReader) GetPullRequestFeedback(_ context.Context, _, _ string, _ int, _ github.PullRequestFeedbackOptions, _ *github.RequestBudget) (github.PullRequestFeedback, error) { + r.cancel() + return github.PullRequestFeedback{Coverage: map[string]github.FeedbackCoverage{ + "issue_comments": {Complete: true, Fetched: 1, Total: 1}, + }}, errors.New("provider interrupted") +} + func (r *feedbackIndexTestReader) ListPullRequests(_ context.Context, _, _ string, opts github.PullRequestListOptions) (github.ListResult[github.Issue], error) { r.perPage = append(r.perPage, opts.PerPage) return r.pages[opts.Page], nil @@ -141,3 +154,15 @@ func TestPullRequestFeedbackSearchKeepsThreadResourceReadable(t *testing.T) { t.Fatalf("exact feedback resource = %+v", item) } } + +func TestFeedbackIndexReportsPartialSnapshotPersistenceFailure(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + svc := newLocalService(t) + t.Cleanup(func() { _ = svc.Close() }) + reader := &MCPReader{Service: svc} + item := reader.indexOnePullRequestFeedback(ctx, &cancelledPartialFeedbackReader{cancel: cancel}, mcpcontract.ThreadRef{Owner: "acme", Repo: "rocket", Kind: "pull_request", Number: 7}, mcpcontract.IndexPullRequestFeedbackInput{Channels: []string{"issue_comments"}}, github.NewRequestBudget(10)) + if item.Code != "feedback_persistence_failed" { + t.Fatalf("partial snapshot persistence failure was hidden: %+v", item) + } +} diff --git a/internal/app/mcp_pull_request_feedback_search.go b/internal/app/mcp_pull_request_feedback_search.go index b30230a..06b6845 100644 --- a/internal/app/mcp_pull_request_feedback_search.go +++ b/internal/app/mcp_pull_request_feedback_search.go @@ -14,6 +14,8 @@ import ( "github.com/morluto/gitcontribute/internal/mcpcontract" ) +const maxFeedbackMergeStateRecoveryThreads = 100 + // SearchPullRequestFeedback is an offline read over the repository feedback // projection. Coverage state is returned independently from match count. func (r *MCPReader) SearchPullRequestFeedback(ctx context.Context, in mcpcontract.SearchPullRequestFeedbackInput) (mcpcontract.SearchPullRequestFeedbackOutput, error) { @@ -182,7 +184,11 @@ func feedbackSearchRecovery(ctx context.Context, c *corpus.Corpus, repositoryID unknown = append(unknown, mcpcontract.ThreadRef{Owner: ref.Owner, Repo: ref.Repo, Kind: "pull_request", Number: item.PullRequestNumber}) } if len(unknown) > 0 { - return recoveryPlan("merge_state_unknown", "Some matching pull requests have no observed merge state; refresh the exact PR-details facet before filtering on merge state.", mcpcontract.RecoveryAction(mcpcontract.HydrateThreadsInput{Threads: uniqueThreadRefs(unknown), Facets: []string{facets.PRDetails}, MaxPages: 1})) + threads := uniqueThreadRefs(unknown) + if len(threads) > maxFeedbackMergeStateRecoveryThreads { + threads = threads[:maxFeedbackMergeStateRecoveryThreads] + } + return recoveryPlan("merge_state_unknown", "Some matching pull requests have no observed merge state; refresh the exact PR-details facet before filtering on merge state.", mcpcontract.RecoveryAction(mcpcontract.HydrateThreadsInput{Threads: threads, Facets: []string{facets.PRDetails}, MaxPages: 1})) } return recoveryPlan("feedback_coverage_partial", "Feedback coverage is partial; continue indexing or retry the returned exact synchronization before treating missing feedback as absence.", mcpcontract.RecoveryAction(mcpcontract.IndexPullRequestFeedbackInput{Repository: mcpcontract.RepositoryRef{Owner: ref.Owner, Repo: ref.Repo}})) } diff --git a/internal/app/mcp_scalable_operations.go b/internal/app/mcp_scalable_operations.go index eac8da8..ac75287 100644 --- a/internal/app/mcp_scalable_operations.go +++ b/internal/app/mcp_scalable_operations.go @@ -636,13 +636,7 @@ func syncRepositoryContextItem( if err != nil { return corpus.Repository{}, err } - defer func() { - if resultErr != nil { - cleanupCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 5*time.Second) - defer cancel() - _ = c.FailRun(cleanupCtx, run.ID, resultErr.Error()) - } - }() + defer failRunOnError(ctx, c, run.ID, &resultErr) repo, _, err := syncRepositoryHeader(ctx, c, reader, ref, run.ID, budget) if err != nil { return corpus.Repository{}, err diff --git a/internal/app/mcp_scalable_reads.go b/internal/app/mcp_scalable_reads.go index a6f4660..94c275d 100644 --- a/internal/app/mcp_scalable_reads.go +++ b/internal/app/mcp_scalable_reads.go @@ -251,133 +251,6 @@ func (r *MCPReader) GetJobs(ctx context.Context, in mcpcontract.GetJobsInput) (m return out, nil } -// ListPullRequestPortfolio performs an offline projection over stored authored -// PRs and status facets; unsupported health facets remain explicitly unknown. -func (r *MCPReader) ListPullRequestPortfolio(ctx context.Context, in mcpcontract.ListPullRequestPortfolioInput) (mcpcontract.ListPullRequestPortfolioOutput, error) { - if len(in.Authors) > 1 { - return mcpcontract.ListPullRequestPortfolioOutput{}, errors.New("authors must contain at most one item") - } - if len(in.PullRequests) > 0 { - if len(in.PullRequests) > 100 { - return mcpcontract.ListPullRequestPortfolioOutput{}, errors.New("pull_requests must contain at most 100 items") - } - if len(in.Authors) > 0 || in.State != "" || in.Limit != 0 { - return mcpcontract.ListPullRequestPortfolioOutput{}, errors.New("pull_requests cannot be combined with authors, state, or limit") - } - if err := rejectDuplicateThreadRefs(in.PullRequests); err != nil { - return mcpcontract.ListPullRequestPortfolioOutput{}, err - } - if err := validatePullRequestRefs(in.PullRequests, "pull_requests"); err != nil { - return mcpcontract.ListPullRequestPortfolioOutput{}, err - } - } - if in.State == "" { - in.State = "open" - } - if in.State != "open" && in.State != "closed" && in.State != "all" { - return mcpcontract.ListPullRequestPortfolioOutput{}, errors.New("state must be open, closed, or all") - } - if in.View == "" { - in.View = "compact" - } - if in.View != "compact" && in.View != "full" { - return mcpcontract.ListPullRequestPortfolioOutput{}, errors.New("view must be compact or full") - } - if in.Limit == 0 && len(in.PullRequests) == 0 { - in.Limit = 20 - } - if len(in.PullRequests) == 0 && (in.Limit < 1 || in.Limit > 100) { - return mcpcontract.ListPullRequestPortfolioOutput{}, errors.New("limit must be between 1 and 100") - } - c, err := r.openReadOnlyCorpus(ctx) - if err != nil { - return mcpcontract.ListPullRequestPortfolioOutput{}, err - } - revision, err := beginCorpusRead(ctx, c, in.SnapshotToken) - if err != nil { - return mcpcontract.ListPullRequestPortfolioOutput{}, err - } - page, unavailable, err := portfolioPage(ctx, c, in) - if err != nil { - return mcpcontract.ListPullRequestPortfolioOutput{}, err - } - format := portfolioResponseFormat(map[string]string{"compact": "concise", "full": "detailed"}[in.View]) - readSet, err := loadPortfolioReadSet(ctx, c, page.PullRequests, format) - if err != nil { - return mcpcontract.ListPullRequestPortfolioOutput{}, err - } - out := mcpcontract.ListPullRequestPortfolioOutput{Status: "complete", View: in.View, RuleVersion: "portfolio.v2", GeneratedAt: formatTime(r.now()), PullRequests: make([]mcpcontract.PullRequestPortfolioItem, 0, len(page.PullRequests)), Total: page.Total, Truncated: page.Truncated, UnavailablePullRequests: unavailable, SnapshotToken: snapshotIdentity(in.SnapshotToken, revision)} - if len(unavailable) > 0 { - out.Status = "partial" - out.Recovery = recoveryPlan("portfolio_items_unavailable", "Some exact pull requests are not present in the local corpus. Refresh those exact pull requests, then reread the portfolio.", mcpcontract.RecoveryAction(mcpcontract.SyncPortfolioInput{Selection: "explicit", PullRequests: append([]mcpcontract.ThreadRef(nil), unavailable...)})) - } - for _, storedPR := range page.PullRequests { - item, err := portfolioItem(storedPR, r.now(), readSet, format) - if err != nil { - return mcpcontract.ListPullRequestPortfolioOutput{}, err - } - if item.StatusCoverage != "complete" { - out.Status = "partial" - item.Recovery = recoveryPlan("portfolio_facet_incomplete", "Refresh the incomplete pull-request health facets, then reread this exact portfolio item.", syncPullRequestCalls([]mcpcontract.ThreadRef{{Owner: item.Owner, Repo: item.Repo, Kind: "pull_request", Number: item.Number}})...) - } - out.PullRequests = append(out.PullRequests, item) - } - if err := finishCorpusRead(ctx, c, revision); err != nil { - return mcpcontract.ListPullRequestPortfolioOutput{}, err - } - if out.Truncated { - out.Status = "partial" - nextLimit := min(100, max(in.Limit*2, in.Limit+1)) - out.Recovery = recoveryPlan("portfolio_truncated", "The portfolio page is bounded. Read the next larger page before treating the returned set as exhaustive.", mcpcontract.RecoveryAction(mcpcontract.ListPullRequestPortfolioInput{Authors: append([]string(nil), in.Authors...), State: in.State, Limit: nextLimit, View: in.View, SnapshotToken: in.SnapshotToken})) - } - return out, nil -} - -func portfolioPage(ctx context.Context, c *corpus.Corpus, in mcpcontract.ListPullRequestPortfolioInput) (corpus.PortfolioPage, []mcpcontract.ThreadRef, error) { - if len(in.PullRequests) == 0 { - author := "" - if len(in.Authors) > 0 { - author = strings.TrimSpace(in.Authors[0]) - } - page, err := c.ListPullRequestPortfolioPage(ctx, author, in.State, in.Limit) - return page, nil, err - } - repositoryKeys := make([]corpus.RepositoryKey, 0, len(in.PullRequests)) - for _, ref := range in.PullRequests { - repositoryKeys = append(repositoryKeys, corpus.RepositoryKey{Owner: ref.Owner, Name: ref.Repo}) - } - repositories, err := c.GetRepositoriesBatch(ctx, repositoryKeys) - if err != nil { - return corpus.PortfolioPage{}, nil, err - } - threadKeys := make([]corpus.ThreadKey, 0, len(in.PullRequests)) - for _, ref := range in.PullRequests { - if repository := repositories[corpus.RepositoryKey{Owner: ref.Owner, Name: ref.Repo}]; repository != nil { - threadKeys = append(threadKeys, corpus.ThreadKey{RepositoryID: repository.ID, Kind: corpus.ThreadKindPullRequest, Number: ref.Number}) - } - } - threads, err := c.GetThreadsBatch(ctx, threadKeys) - if err != nil { - return corpus.PortfolioPage{}, nil, err - } - page := corpus.PortfolioPage{PullRequests: make([]corpus.PortfolioPullRequest, 0, len(in.PullRequests)), Total: len(in.PullRequests)} - unavailable := make([]mcpcontract.ThreadRef, 0) - for _, ref := range in.PullRequests { - repository := repositories[corpus.RepositoryKey{Owner: ref.Owner, Name: ref.Repo}] - if repository == nil { - unavailable = append(unavailable, ref) - continue - } - thread := threads[corpus.ThreadKey{RepositoryID: repository.ID, Kind: corpus.ThreadKindPullRequest, Number: ref.Number}] - if thread == nil { - unavailable = append(unavailable, ref) - continue - } - page.PullRequests = append(page.PullRequests, corpus.PortfolioPullRequest{Owner: repository.Owner, Repo: repository.Name, Thread: *thread}) - } - return page, unavailable, nil -} - type portfolioReadSet struct { coverage map[corpus.ThreadFacetKey]*corpus.Coverage observations map[corpus.ThreadFacetKey]corpus.FacetObservationBatch @@ -424,18 +297,35 @@ func loadPortfolioReadSet(ctx context.Context, c *corpus.Corpus, pullRequests [] return portfolioReadSet{coverage: coverage, observations: observations}, nil } -// The projection deliberately keeps coverage, observation decoding, and the -// portfolio.v2 classification together so unknown facets cannot become facts. -// -//nolint:gocognit,cyclop func portfolioItem(stored corpus.PortfolioPullRequest, now time.Time, readSet portfolioReadSet, format portfolioResponseFormat) (mcpcontract.PullRequestPortfolioItem, error) { t := stored.Thread out := mcpcontract.PullRequestPortfolioItem{Ref: fmt.Sprintf("%s/%s#%d", stored.Owner, stored.Repo, t.Number), Owner: stored.Owner, Repo: stored.Repo, Number: t.Number, Title: t.Title, State: t.State, Author: t.Author, Draft: t.Draft, SourceUpdatedAt: formatTime(t.SourceUpdatedAt), StatusCoverage: "missing"} + coverage := portfolioCoverage(&out, t.ID, readSet.coverage, format) + details, err := applyPortfolioDetails(&out, t.ID, coverage[FacetPRDetails], readSet.observations, format) + if err != nil { + return out, fmt.Errorf("decode pull-request details for %s: %w", out.Ref, err) + } + if err := applyPortfolioReviews(&out, t.ID, coverage[FacetPRReviews], readSet.observations); err != nil { + return out, fmt.Errorf("decode pull-request reviews for %s: %w", out.Ref, err) + } + mergeabilityKnown, err := applyPortfolioHealth(&out, t.ID, coverage, readSet.observations) + if err != nil { + return out, err + } + if err := applyPortfolioSupplementalDetails(&out, t.ID, coverage, readSet.observations, format); err != nil { + return out, err + } + addPortfolioCoverageReasons(&out, coverage, mergeabilityKnown) + setPortfolioAttention(&out, t, details, coverage, mergeabilityKnown, now) + return out, nil +} + +func portfolioCoverage(out *mcpcontract.PullRequestPortfolioItem, threadID int64, all map[corpus.ThreadFacetKey]*corpus.Coverage, format portfolioResponseFormat) map[string]*corpus.Coverage { facets := portfolioFacets() coverage := make(map[string]*corpus.Coverage, len(facets)) complete, observed := true, 0 for _, facet := range facets { - cov := readSet.coverage[corpus.ThreadFacetKey{ThreadID: t.ID, Facet: facet}] + cov := all[corpus.ThreadFacetKey{ThreadID: threadID, Facet: facet}] coverage[facet] = cov status := "missing" if cov != nil { @@ -448,11 +338,11 @@ func portfolioItem(stored corpus.PortfolioPullRequest, now time.Time, readSet po if cov == nil || !cov.Complete { complete = false } - entry := mcpcontract.FacetCoverageOutput{Facet: facet, Status: status} - if cov != nil { - entry.Complete, entry.UpdatedAt = cov.Complete, formatTime(cov.UpdatedAt) - } if format.includesDetails() { + entry := mcpcontract.FacetCoverageOutput{Facet: facet, Status: status} + if cov != nil { + entry.Complete, entry.UpdatedAt = cov.Complete, formatTime(cov.UpdatedAt) + } out.Facets = append(out.Facets, entry) } } @@ -462,54 +352,66 @@ func portfolioItem(stored corpus.PortfolioPullRequest, now time.Time, readSet po if complete { out.StatusCoverage = "complete" } - detailCoverage, reviewCoverage := coverage[FacetPRDetails], coverage[FacetPRReviews] + return coverage +} + +func applyPortfolioDetails(out *mcpcontract.PullRequestPortfolioItem, threadID int64, coverage *corpus.Coverage, observations map[corpus.ThreadFacetKey]corpus.FacetObservationBatch, format portfolioResponseFormat) (github.PullRequestDetails, error) { var details github.PullRequestDetails - if detailCoverage != nil && detailCoverage.Complete { - observedAt, err := decodeLatestFacet(readSet.observations, t.ID, FacetPRDetails, &details) - if err != nil { - return out, fmt.Errorf("decode pull-request details for %s: %w", out.Ref, err) - } - out.Mergeable = details.Mergeable - if format.includesDetails() { - out.HeadRef, out.HeadSHA, out.BaseRef, out.BaseSHA = details.HeadRef, details.HeadSHA, details.BaseRef, details.BaseSHA - } - out.StatusObservedAt = observedAt + if coverage == nil || !coverage.Complete { + return details, nil } - if reviewCoverage != nil && reviewCoverage.Complete { - reviewObservations := readSet.observations[corpus.ThreadFacetKey{ThreadID: t.ID, Facet: FacetPRReviews}].Observations - latest := make(map[string]github.Review) - for _, observation := range reviewObservations { - var reviews []github.Review - if err := json.Unmarshal([]byte(observation.Payload), &reviews); err != nil { - return out, fmt.Errorf("decode pull-request reviews for %s: %w", out.Ref, err) - } - for _, review := range reviews { - previous, ok := latest[strings.ToLower(review.Author)] - if !ok || review.SubmittedAt.After(previous.SubmittedAt) { - latest[strings.ToLower(review.Author)] = review - } - } - } - changes, approved := false, false - for _, review := range latest { - switch strings.ToUpper(review.State) { - case "CHANGES_REQUESTED": - changes = true - case "APPROVED": - approved = true + observedAt, err := decodeLatestFacet(observations, threadID, FacetPRDetails, &details) + if err != nil { + return details, err + } + out.Mergeable, out.StatusObservedAt = details.Mergeable, observedAt + if format.includesDetails() { + out.HeadRef, out.HeadSHA, out.BaseRef, out.BaseSHA = details.HeadRef, details.HeadSHA, details.BaseRef, details.BaseSHA + } + return details, nil +} + +func applyPortfolioReviews(out *mcpcontract.PullRequestPortfolioItem, threadID int64, coverage *corpus.Coverage, observations map[corpus.ThreadFacetKey]corpus.FacetObservationBatch) error { + if coverage == nil || !coverage.Complete { + return nil + } + latest := make(map[string]github.Review) + for _, observation := range observations[corpus.ThreadFacetKey{ThreadID: threadID, Facet: FacetPRReviews}].Observations { + var reviews []github.Review + if err := json.Unmarshal([]byte(observation.Payload), &reviews); err != nil { + return err + } + for _, review := range reviews { + key := strings.ToLower(review.Author) + previous, ok := latest[key] + if !ok || review.SubmittedAt.After(previous.SubmittedAt) { + latest[key] = review } } - if changes { - out.ReviewDecision = "changes_requested" - } else if approved { - out.ReviewDecision = "approved" + } + changes, approved := false, false + for _, review := range latest { + switch strings.ToUpper(review.State) { + case "CHANGES_REQUESTED": + changes = true + case "APPROVED": + approved = true } } + if changes { + out.ReviewDecision = "changes_requested" + } else if approved { + out.ReviewDecision = "approved" + } + return nil +} + +func applyPortfolioHealth(out *mcpcontract.PullRequestPortfolioItem, threadID int64, coverage map[string]*corpus.Coverage, observations map[corpus.ThreadFacetKey]corpus.FacetObservationBatch) (bool, error) { mergeabilityKnown := false if cov := coverage[FacetPRMergeState]; cov != nil && cov.Complete { var value github.PullRequestMergeState - if _, err := decodeLatestFacet(readSet.observations, t.ID, FacetPRMergeState, &value); err != nil { - return out, err + if _, err := decodeLatestFacet(observations, threadID, FacetPRMergeState, &value); err != nil { + return false, err } out.MergeStateStatus = strings.ToLower(value.MergeStateStatus) if value.MergeableKnown { @@ -520,16 +422,15 @@ func portfolioItem(stored corpus.PortfolioPullRequest, now time.Time, readSet po } if cov := coverage[FacetPRChecks]; cov != nil && cov.Complete { var checks []github.PullRequestCheck - if _, err := decodeLatestFacet(readSet.observations, t.ID, FacetPRChecks, &checks); err != nil { - return out, err + if _, err := decodeLatestFacet(observations, threadID, FacetPRChecks, &checks); err != nil { + return false, err } - out.ChecksTotal = len(checks) - out.ChecksStatus = classifyChecks(checks) + out.ChecksTotal, out.ChecksStatus = len(checks), classifyChecks(checks) } if cov := coverage[FacetPRReviewThreads]; cov != nil && cov.Complete { var threads []github.PullRequestReviewThread - if _, err := decodeLatestFacet(readSet.observations, t.ID, FacetPRReviewThreads, &threads); err != nil { - return out, err + if _, err := decodeLatestFacet(observations, threadID, FacetPRReviewThreads, &threads); err != nil { + return false, err } unresolved := 0 for _, thread := range threads { @@ -541,31 +442,42 @@ func portfolioItem(stored corpus.PortfolioPullRequest, now time.Time, readSet po } if cov := coverage[FacetPRMergeQueue]; cov != nil && cov.Complete { var queue *github.PullRequestMergeQueueEntry - if _, err := decodeLatestFacet(readSet.observations, t.ID, FacetPRMergeQueue, &queue); err != nil { - return out, err + if _, err := decodeLatestFacet(observations, threadID, FacetPRMergeQueue, &queue); err != nil { + return false, err } if queue != nil { out.MergeQueueState, out.MergeQueuePosition = strings.ToLower(queue.State), queue.Position } } - if cov := coverage[FacetPRClosingIssues]; format.includesDetails() && cov != nil && cov.Complete { + return mergeabilityKnown, nil +} + +func applyPortfolioSupplementalDetails(out *mcpcontract.PullRequestPortfolioItem, threadID int64, coverage map[string]*corpus.Coverage, observations map[corpus.ThreadFacetKey]corpus.FacetObservationBatch, format portfolioResponseFormat) error { + if !format.includesDetails() { + return nil + } + if cov := coverage[FacetPRClosingIssues]; cov != nil && cov.Complete { var issues []github.PullRequestClosingIssue - if _, err := decodeLatestFacet(readSet.observations, t.ID, FacetPRClosingIssues, &issues); err != nil { - return out, err + if _, err := decodeLatestFacet(observations, threadID, FacetPRClosingIssues, &issues); err != nil { + return err } for _, issue := range issues { out.ClosingIssues = append(out.ClosingIssues, fmt.Sprintf("%s#%d", issue.RepositoryFullName, issue.Number)) } } - if cov := coverage[FacetPRFiles]; format.includesDetails() && cov != nil && cov.Complete { + if cov := coverage[FacetPRFiles]; cov != nil && cov.Complete { var files []github.PullRequestFile - if _, err := decodeLatestFacet(readSet.observations, t.ID, FacetPRFiles, &files); err != nil { - return out, err + if _, err := decodeLatestFacet(observations, threadID, FacetPRFiles, &files); err != nil { + return err } for _, file := range files { out.ChangedFiles = append(out.ChangedFiles, file.Path) } } + return nil +} + +func addPortfolioCoverageReasons(out *mcpcontract.PullRequestPortfolioItem, coverage map[string]*corpus.Coverage, mergeabilityKnown bool) { for _, facet := range []string{FacetPRChecks, FacetPRReviewThreads, FacetPRMergeState, FacetPRMergeQueue} { if coverage[facet] == nil || !coverage[facet].Complete { out.Reasons = append(out.Reasons, facet+" coverage is incomplete") @@ -574,15 +486,19 @@ func portfolioItem(stored corpus.PortfolioPullRequest, now time.Time, readSet po if coverage[FacetPRMergeState] != nil && coverage[FacetPRMergeState].Complete && !mergeabilityKnown { out.Reasons = append(out.Reasons, "GitHub mergeability is still computing") } - healthComplete := coverage[FacetPRChecks] != nil && coverage[FacetPRChecks].Complete && coverage[FacetPRReviewThreads] != nil && coverage[FacetPRReviewThreads].Complete && coverage[FacetPRMergeState] != nil && coverage[FacetPRMergeState].Complete && mergeabilityKnown && coverage[FacetPRMergeQueue] != nil && coverage[FacetPRMergeQueue].Complete +} + +func setPortfolioAttention(out *mcpcontract.PullRequestPortfolioItem, thread corpus.Thread, details github.PullRequestDetails, coverage map[string]*corpus.Coverage, mergeabilityKnown bool, now time.Time) { + detailCoverage := coverage[FacetPRDetails] + healthComplete := completePortfolioHealthCoverage(coverage, mergeabilityKnown) switch { - case t.Merged: + case thread.Merged: out.Attention = "merged" out.Reasons = append([]string{"pull request is merged"}, out.Reasons...) - case t.State == "closed" && t.MergedKnown: + case thread.State == "closed" && thread.MergedKnown: out.Attention = "closed_unmerged" out.Reasons = append([]string{"pull request is closed and GitHub reports it was not merged"}, out.Reasons...) - case t.State == "closed": + case thread.State == "closed": out.Attention = "unknown" out.Reasons = append([]string{"pull request is closed but merge state has not been observed"}, out.Reasons...) case detailCoverage == nil: @@ -612,7 +528,7 @@ func portfolioItem(stored corpus.PortfolioPullRequest, now time.Time, readSet po case !healthComplete: out.Attention = "unknown" out.Reasons = append([]string{"required pull-request health coverage is incomplete"}, out.Reasons...) - case now.Sub(t.SourceUpdatedAt) > 14*24*time.Hour: + case now.Sub(thread.SourceUpdatedAt) > 14*24*time.Hour: out.Attention = "stale" out.Reasons = append([]string{"pull request has not been updated for more than 14 days"}, out.Reasons...) case out.ReviewDecision == "approved": @@ -622,7 +538,15 @@ func portfolioItem(stored corpus.PortfolioPullRequest, now time.Time, readSet po out.Attention = "awaiting_review" out.Reasons = append([]string{"no approval or change request is stored"}, out.Reasons...) } - return out, nil +} + +func completePortfolioHealthCoverage(coverage map[string]*corpus.Coverage, mergeabilityKnown bool) bool { + for _, facet := range []string{FacetPRChecks, FacetPRReviewThreads, FacetPRMergeState, FacetPRMergeQueue} { + if coverage[facet] == nil || !coverage[facet].Complete { + return false + } + } + return mergeabilityKnown } func portfolioFacets() []string { diff --git a/internal/app/mcp_scalable_test.go b/internal/app/mcp_scalable_test.go index 8345e24..19df0e6 100644 --- a/internal/app/mcp_scalable_test.go +++ b/internal/app/mcp_scalable_test.go @@ -715,6 +715,21 @@ func TestPullRequestWorkflowsRejectMalformedReferencesBeforeSubmission(t *testin } } +func TestSyncPortfolioRejectsDuplicateDefaultKindReferences(t *testing.T) { + t.Parallel() + reader := &MCPReader{newSearchTestService(t)} + _, err := reader.SyncPortfolio(context.Background(), mcpcontract.SyncPortfolioInput{ + Selection: "explicit", + PullRequests: []mcpcontract.ThreadRef{ + {Owner: "acme", Repo: "rocket", Number: 7}, + {Owner: "acme", Repo: "rocket", Kind: "pull_request", Number: 7}, + }, + }) + if err == nil { + t.Fatal("expected duplicate pull-request references to be rejected") + } +} + func TestScalableRuntimeRejectsPageBoundsBeforeSubmittingJob(t *testing.T) { t.Parallel() reader := &MCPReader{newSearchTestService(t)} diff --git a/internal/app/mcp_snapshot_token_test.go b/internal/app/mcp_snapshot_token_test.go index 4694e8a..fcef6c0 100644 --- a/internal/app/mcp_snapshot_token_test.go +++ b/internal/app/mcp_snapshot_token_test.go @@ -51,3 +51,10 @@ func TestSnapshotTokenReadFailsClosedAfterCorpusMutation(t *testing.T) { t.Fatalf("ephemeral snapshot reuse error = %v", err) } } + +func TestManifestSnapshotRecoveryDropsExpiredToken(t *testing.T) { + action := manifestSnapshotRecovery(mcpcontract.ExportManifestInput{OpportunityID: "opp-1", WorkspaceID: "ws-1", SnapshotToken: "snapshot-stale"}) + if action.Type != "export_manifest" || action.ExportManifest == nil || action.ExportManifest.SnapshotToken != "" || action.ExportManifest.OpportunityID != "opp-1" || action.ExportManifest.WorkspaceID != "ws-1" { + t.Fatalf("manifest stale-snapshot recovery = %+v", action) + } +} diff --git a/internal/app/mcp_test.go b/internal/app/mcp_test.go index 4417f99..7c83484 100644 --- a/internal/app/mcp_test.go +++ b/internal/app/mcp_test.go @@ -131,6 +131,9 @@ func TestMCPReaderRepositorySearchDoesNotFallBackFromMissingExactRepository(t *t if blank.Total != 1 || len(blank.Matches) != 1 { t.Fatalf("blank exact repository search = %+v", blank) } + if blank.Query != "" { + t.Fatalf("blank query was not normalized: %q", blank.Query) + } } func TestMCPReaderRepositorySearchPreservesIncompleteNestedProjection(t *testing.T) { diff --git a/internal/app/mcp_thread_search.go b/internal/app/mcp_thread_search.go index abefb06..b7de7e4 100644 --- a/internal/app/mcp_thread_search.go +++ b/internal/app/mcp_thread_search.go @@ -12,7 +12,7 @@ import ( // Search performs a local-only corpus search through the MCP interface. func (r *MCPReader) Search(ctx context.Context, in mcpcontract.SearchInput) (mcpcontract.SearchOutput, error) { - if in.Query == "" { + if strings.TrimSpace(in.Query) == "" { return mcpcontract.SearchOutput{}, errors.New("query is required") } if in.Limit == 0 { diff --git a/internal/app/mcp_v1.go b/internal/app/mcp_v1.go index 320b241..804ea9c 100644 --- a/internal/app/mcp_v1.go +++ b/internal/app/mcp_v1.go @@ -61,6 +61,7 @@ func (r *MCPReader) ThreadByNumber(ctx context.Context, in mcpcontract.ThreadByN // ExplainMatch explains why a search result matched. func (r *MCPReader) ExplainMatch(ctx context.Context, in mcpcontract.ExplainMatchInput) (mcpcontract.ExplainMatchOutput, error) { + in.Query = strings.TrimSpace(in.Query) ref := domain.RepoRef{Owner: in.Owner, Repo: in.Repo} if err := ref.Validate(); err != nil { return mcpcontract.ExplainMatchOutput{}, err @@ -639,6 +640,7 @@ func (r *MCPReader) ExportManifest(ctx context.Context, in mcpcontract.ExportMan return mcpcontract.ManifestOutput{}, mcpcontract.Unavailable( "snapshot_expired", fmt.Sprintf("snapshot watermark is no longer current; expected %d, current %d", stale.Expected, stale.Current), + manifestSnapshotRecovery(in), ) } return mcpcontract.ManifestOutput{}, err @@ -646,6 +648,11 @@ func (r *MCPReader) ExportManifest(ctx context.Context, in mcpcontract.ExportMan return manifestStatementToMCP(statement, snapshotIdentity(in.SnapshotToken, revision)), nil } +func manifestSnapshotRecovery(in mcpcontract.ExportManifestInput) mcpcontract.ToolCall { + in.SnapshotToken = "" + return mcpcontract.RecoveryAction(in) +} + func manifestStatementToMCP(statement *manifest.Statement, snapshotToken string) mcpcontract.ManifestOutput { return mcpcontract.ManifestOutput{ ManifestID: statement.Predicate.ManifestID, ContentSHA256: statement.Predicate.ContentSHA256, diff --git a/internal/app/radar_related_work.go b/internal/app/radar_related_work.go index 684e2aa..962f111 100644 --- a/internal/app/radar_related_work.go +++ b/internal/app/radar_related_work.go @@ -292,14 +292,26 @@ func radarTimelineReference(event github.IssueTimelineEvent, defaultRepo domain. func resolveRadarRelatedWork(ctx context.Context, c *corpus.Corpus, raw []rawRadarRelatedWork) ([]radar.RelatedWork, error) { values := make([]radar.RelatedWork, 0, len(raw)) + resolvedByReference := make(map[string]radar.RelatedWork, len(raw)) for _, item := range raw { if err := ctx.Err(); err != nil { return nil, err } - resolved, err := resolveRadarReference(ctx, c, item.reference, item.direction, item.evidence) - if err != nil { - return nil, err + key := radarReferenceKey(item.reference) + resolved, ok := resolvedByReference[key] + if !ok { + var err error + resolved, err = resolveRadarReference(ctx, c, item.reference, item.direction, item.evidence) + if err != nil { + return nil, err + } + resolvedByReference[key] = resolved } + // The resolved thread fields are shared, but every raw observation keeps + // its own relationship semantics and source-bound evidence. + resolved.Relation = item.reference.Relation + resolved.Direction = item.direction + resolved.Evidence = []radar.RelatedWorkEvidence{item.evidence} values = append(values, resolved) } return values, nil diff --git a/internal/app/radar_test.go b/internal/app/radar_test.go index 8656314..fedb495 100644 --- a/internal/app/radar_test.go +++ b/internal/app/radar_test.go @@ -372,6 +372,51 @@ func TestContributionRadarUnifiesCommentDependenciesAndTimelineCrossReferences(t } } +func TestContributionRadarPreservesRepeatedReferenceEvidence(t *testing.T) { + t.Parallel() + fixture := newRadarTestFixture(t) + if _, err := fixture.svc.corpus.UpsertThread(fixture.ctx, corpus.Thread{ + RepositoryID: fixture.repoID, Kind: corpus.ThreadKindPullRequest, Number: 10, State: "open", + Title: "Related PR", SourceUpdatedAt: fixture.now.Add(-10 * time.Minute), + }, `{}`); err != nil { + t.Fatal(err) + } + comments, err := json.Marshal([]github.IssueComment{{ + ID: 20, Body: "This depends on https://github.com/owner/repo/pull/10.", + CreatedAt: fixture.now.Add(-20 * time.Minute), UpdatedAt: fixture.now.Add(-19 * time.Minute), + HTMLURL: "https://github.com/owner/repo/issues/2#issuecomment-20", + }}) + if err != nil { + t.Fatal(err) + } + if err := fixture.svc.corpus.ApplyFacetObservationSet(fixture.ctx, fixture.repoID, &fixture.issue2ID, FacetIssueComments, fixture.now.Add(-19*time.Minute), []corpus.FacetObservationInput{{ + SourceUpdatedAt: fixture.now.Add(-19 * time.Minute), Payload: string(comments), + }}, true, 0); err != nil { + t.Fatal(err) + } + timeline, err := json.Marshal([]github.IssueTimelineEvent{{ + ID: 21, Event: "cross-referenced", SourceOwner: "owner", SourceRepository: "repo", + SourceNumber: 10, SourceIsPullRequest: true, CreatedAt: fixture.now.Add(-18 * time.Minute), + }}) + if err != nil { + t.Fatal(err) + } + if err := fixture.svc.corpus.ApplyFacetObservationSet(fixture.ctx, fixture.repoID, &fixture.issue2ID, FacetIssueTimeline, fixture.now.Add(-18*time.Minute), []corpus.FacetObservationInput{{ + SourceUpdatedAt: fixture.now.Add(-18 * time.Minute), Payload: string(timeline), + }}, true, 0); err != nil { + t.Fatal(err) + } + + report, err := fixture.svc.ContributionRadar(fixture.ctx, contracts.RadarOptions{Repo: contracts.RepoRef{Owner: "owner", Repo: "repo"}}) + if err != nil { + t.Fatal(err) + } + work := radarRelatedWork(radarCandidate(report, 2), "pull_request:owner/repo#10") + if work == nil || work.Relation != relatedwork.RelationDependsOn || !radarRelatedEvidence(*work, "issue_comment") || !radarRelatedEvidence(*work, "issue_timeline") { + t.Fatalf("related work = %+v", work) + } +} + func TestNormalizeRadarRelatedWorkReportsEvidenceTruncation(t *testing.T) { t.Parallel() values := make([]radar.RelatedWork, 0, maxRadarEvidencePerRelation+1) diff --git a/internal/app/run_lifecycle.go b/internal/app/run_lifecycle.go new file mode 100644 index 0000000..c15172e --- /dev/null +++ b/internal/app/run_lifecycle.go @@ -0,0 +1,26 @@ +package app + +import ( + "context" + "errors" + "fmt" + "time" + + "github.com/morluto/gitcontribute/internal/corpus" +) + +const runCleanupTimeout = 5 * time.Second + +// failRunOnError records an operation failure after its caller's context may +// have been cancelled. If persistence also fails, retain both errors so callers +// never mistake an unrecorded failed run for a cleanly finalized one. +func failRunOnError(ctx context.Context, c *corpus.Corpus, runID int64, resultErr *error) { + if *resultErr == nil { + return + } + cleanupCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), runCleanupTimeout) + defer cancel() + if err := c.FailRun(cleanupCtx, runID, (*resultErr).Error()); err != nil { + *resultErr = errors.Join(*resultErr, fmt.Errorf("record failed run %d: %w", runID, err)) + } +} diff --git a/internal/app/run_lifecycle_test.go b/internal/app/run_lifecycle_test.go new file mode 100644 index 0000000..30e2700 --- /dev/null +++ b/internal/app/run_lifecycle_test.go @@ -0,0 +1,27 @@ +package app + +import ( + "context" + "errors" + "strings" + "testing" +) + +func TestFailRunOnErrorJoinsFinalizationFailure(t *testing.T) { + t.Parallel() + svc := newJobTestService(t) + if err := svc.corpus.Close(); err != nil { + t.Fatalf("close corpus: %v", err) + } + + original := errors.New("acquisition failed") + resultErr := original + failRunOnError(context.Background(), svc.corpus, 42, &resultErr) + + if !errors.Is(resultErr, original) { + t.Fatalf("original operation error was lost: %v", resultErr) + } + if !strings.Contains(resultErr.Error(), "record failed run 42") { + t.Fatalf("finalization failure was not reported: %v", resultErr) + } +} diff --git a/internal/app/search_test.go b/internal/app/search_test.go index aef43e4..97ca7e5 100644 --- a/internal/app/search_test.go +++ b/internal/app/search_test.go @@ -219,6 +219,15 @@ func TestMCPSearchDefaultsCompactAndOffersFullView(t *testing.T) { } } +func TestMCPSearchRejectsWhitespaceOnlyQuery(t *testing.T) { + t.Parallel() + + _, err := (&MCPReader{Service: newSearchTestService(t)}).Search(context.Background(), mcpcontract.SearchInput{Query: " \t\n "}) + if err == nil || err.Error() != "query is required" { + t.Fatalf("whitespace query error = %v", err) + } +} + func TestSearchRejectsMalformedCursor(t *testing.T) { t.Parallel() ctx := context.Background() diff --git a/internal/app/surfaces_extra.go b/internal/app/surfaces_extra.go index c2247d0..30bd73b 100644 --- a/internal/app/surfaces_extra.go +++ b/internal/app/surfaces_extra.go @@ -33,13 +33,7 @@ func (s *Service) RepositoryContextSync(ctx context.Context, repo contracts.Repo if err != nil { return nil, err } - defer func() { - if resultErr != nil { - cleanupCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 5*time.Second) - defer cancel() - _ = c.FailRun(cleanupCtx, run.ID, resultErr.Error()) - } - }() + defer failRunOnError(ctx, c, run.ID, &resultErr) budget := newSyncRequestBudget(plan.RequestBudget) if _, _, err := syncRepositoryHeader(ctx, c, reader, ref, run.ID, budget); err != nil { return nil, err diff --git a/internal/app/sync_budget_test.go b/internal/app/sync_budget_test.go index bb43d22..3215f94 100644 --- a/internal/app/sync_budget_test.go +++ b/internal/app/sync_budget_test.go @@ -17,6 +17,7 @@ type authoredHeaderReader struct { prDetailRequests int now time.Time searchResult github.AuthoredPullRequestSearchResult + searchOptions github.AuthoredPullRequestSearchOptions } func (r *authoredHeaderReader) GetRepository(context.Context, string, string) (github.Repository, github.RateInfo, error) { @@ -61,7 +62,8 @@ func (*authoredHeaderReader) GetAuthenticatedIdentity(context.Context) (github.I return github.Identity{Login: "contributor", ID: 1}, github.RateInfo{}, nil } -func (r *authoredHeaderReader) SearchAuthoredPullRequests(context.Context, github.AuthoredPullRequestSearchOptions) (github.AuthoredPullRequestSearchResult, error) { +func (r *authoredHeaderReader) SearchAuthoredPullRequests(_ context.Context, options github.AuthoredPullRequestSearchOptions) (github.AuthoredPullRequestSearchResult, error) { + r.searchOptions = options if r.searchResult.Items != nil { return r.searchResult, nil } @@ -71,6 +73,34 @@ func (r *authoredHeaderReader) SearchAuthoredPullRequests(context.Context, githu }}, nil } +func TestAuthoredPullRequestSyncScopesDiscoveryBeforeLimit(t *testing.T) { + t.Parallel() + ctx := context.Background() + paths := config.NewPaths(&config.Env{Home: t.TempDir()}) + svc, err := New(paths, "test", nil) + if err != nil { + t.Fatal(err) + } + defer func() { _ = svc.Close() }() + if _, err := svc.Init(ctx); err != nil { + t.Fatal(err) + } + now := time.Date(2026, time.August, 8, 0, 0, 0, 0, time.UTC) + reader := &authoredHeaderReader{now: now, searchResult: github.AuthoredPullRequestSearchResult{Items: []github.Issue{ + {RepositoryOwner: "acme", RepositoryName: "other", Kind: github.ThreadKindPullRequest, Number: 9, State: "open", Title: "newer unrelated", UpdatedAt: now.Add(time.Second)}, + {RepositoryOwner: "acme", RepositoryName: "rocket", Kind: github.ThreadKindPullRequest, Number: 7, State: "open", Title: "selected", UpdatedAt: now}, + }}} + svc.SetGitHubReader(reader) + scope := &mcpcontract.RepositoryRef{Owner: "acme", Repo: "rocket"} + out, err := svc.syncAuthoredPullRequests(ctx, authoredPullRequestSyncOptions{Repository: scope, State: "open", Limit: 1, MaxRequests: 20}, func(string, string) error { return nil }) + if err != nil { + t.Fatal(err) + } + if reader.searchOptions.RepositoryOwner != "acme" || reader.searchOptions.RepositoryName != "rocket" || len(out.PullRequestTargets) != 1 || out.PullRequestTargets[0].Repo != "rocket" || out.PullRequestTargets[0].Number != 7 { + t.Fatalf("scoped discovery options=%+v result=%+v", reader.searchOptions, out) + } +} + func TestAuthoredPullRequestSyncReportsItemLimitTruncation(t *testing.T) { t.Parallel() ctx := context.Background() diff --git a/internal/app/sync_thread_operations.go b/internal/app/sync_thread_operations.go index 7a9ab2b..821002d 100644 --- a/internal/app/sync_thread_operations.go +++ b/internal/app/sync_thread_operations.go @@ -47,13 +47,7 @@ func (s *Service) syncProvidedThreadHeaders(ctx context.Context, repo contracts. if err != nil { return nil, err } - defer func() { - if resultErr != nil { - cleanupCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 5*time.Second) - defer cancel() - _ = c.FailRun(cleanupCtx, run.ID, resultErr.Error()) - } - }() + defer failRunOnError(ctx, c, run.ID, &resultErr) writer := &syncThreadWriter{ ctx: ctx, corpus: c, owner: ref.Owner, repo: ref.Repo, repositoryID: stored.ID, kind: "pull_request", sourceUpdatedAt: sourceUpdatedAt, } @@ -105,13 +99,7 @@ func (s *Service) syncThreadHeaders(ctx context.Context, repo contracts.RepoRef, if err != nil { return nil, err } - defer func() { - if resultErr != nil { - cleanupCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 5*time.Second) - defer cancel() - _ = c.FailRun(cleanupCtx, run.ID, resultErr.Error()) - } - }() + defer failRunOnError(ctx, c, run.ID, &resultErr) budget := newSyncRequestBudget(syncOpts.MaxRequests) selection, err := syncThreadHeaderSelection(ctx, c, reader, ref, repoProjection.ID, repoProjection.SourceUpdatedAt, syncOpts, nil, budget) if err != nil { diff --git a/internal/app/upgrade.go b/internal/app/upgrade.go index 33f3b4a..6f2eea8 100644 --- a/internal/app/upgrade.go +++ b/internal/app/upgrade.go @@ -442,6 +442,8 @@ func readPackageVersion(root string) string { return normalizeVersion(pkg.Version) } +const maxUpgradePackageBytes = 1 << 20 + func readUpgradeFile(path string) (_ []byte, err error) { root, err := os.OpenRoot(filepath.Dir(path)) if err != nil { @@ -453,7 +455,14 @@ func readUpgradeFile(path string) (_ []byte, err error) { return nil, err } defer func() { err = errors.Join(err, file.Close()) }() - return io.ReadAll(file) + data, err := io.ReadAll(io.LimitReader(file, maxUpgradePackageBytes+1)) + if err != nil { + return nil, err + } + if len(data) > maxUpgradePackageBytes { + return nil, fmt.Errorf("upgrade metadata exceeds %d bytes", maxUpgradePackageBytes) + } + return data, nil } func (s *Service) privateRuntimeStage(current, latest string) contracts.UpgradeStage { diff --git a/internal/app/upgrade_file_test.go b/internal/app/upgrade_file_test.go new file mode 100644 index 0000000..4f15129 --- /dev/null +++ b/internal/app/upgrade_file_test.go @@ -0,0 +1,19 @@ +package app + +import ( + "bytes" + "os" + "path/filepath" + "strings" + "testing" +) + +func TestReadUpgradeFileBoundsPackageMetadata(t *testing.T) { + path := filepath.Join(t.TempDir(), "package.json") + if err := os.WriteFile(path, bytes.Repeat([]byte("x"), maxUpgradePackageBytes+1), 0o600); err != nil { + t.Fatal(err) + } + if _, err := readUpgradeFile(path); err == nil || !strings.Contains(err.Error(), "upgrade metadata exceeds") { + t.Fatalf("oversized package metadata error = %v", err) + } +} diff --git a/internal/app/workspace.go b/internal/app/workspace.go index f78bd28..7551c9b 100644 --- a/internal/app/workspace.go +++ b/internal/app/workspace.go @@ -87,7 +87,7 @@ func (s *Service) workspaceReader() (*workspace.Manager, error) { } // CreateWorkspace creates a managed worktree for an investigation. -func (s *Service) CreateWorkspace(ctx context.Context, investigationID string, opts contracts.WorkspaceCreateOptions) (*contracts.WorkspaceResult, error) { +func (s *Service) CreateWorkspace(ctx context.Context, investigationID string, opts contracts.WorkspaceCreateOptions) (result *contracts.WorkspaceResult, returnErr error) { invSvc, err := s.writeInvestigationSvc(ctx) if err != nil { return nil, err @@ -141,7 +141,9 @@ func (s *Service) CreateWorkspace(ctx context.Context, investigationID string, o } cleanup, cancel := context.WithTimeout(context.WithoutCancel(ctx), 10*time.Second) defer cancel() - _ = mgr.Remove(cleanup, ws.Path, true) + if err := mgr.Remove(cleanup, ws.Path, true); err != nil { + returnErr = errors.Join(returnErr, fmt.Errorf("cleanup unpersisted workspace: %w", err)) + } }() ws.InvestigationID = inv.ID diff --git a/internal/cli/archive_cli.go b/internal/cli/archive_cli.go index 92e72ef..4b525ec 100644 --- a/internal/cli/archive_cli.go +++ b/internal/cli/archive_cli.go @@ -70,7 +70,9 @@ func (c *CLI) runArchive(ctx context.Context, command string, cmd *archiveCmd) e if err != nil { return NewCLIError(ExitUsage, err) } - _, _ = fmt.Fprintf(c.stderr, "hydrating %s#%d...\n", repo, number) + if err := c.writeProgressf("hydrating %s#%d...\n", repo, number); err != nil { + return err + } result, err := service.Hydrate(ctx, repo, number, contracts.HydrateOptions{ Facets: splitCSV(cmd.Hydrate.With), MaxPages: cmd.Hydrate.MaxPages, }) diff --git a/internal/cli/cli.go b/internal/cli/cli.go index 6537a4d..50fe8cc 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -66,6 +66,13 @@ func (c *CLI) SetInput(input io.Reader) { // tests and alternate accessible frontends. func (c *CLI) SetSetupPrompter(prompter SetupPrompter) { c.setupPrompter = prompter } +func (c *CLI) writeProgressf(format string, args ...any) error { + if _, err := fmt.Fprintf(c.stderr, format, args...); err != nil { + return c.mapError(fmt.Errorf("write progress: %w", err)) + } + return nil +} + type rootCmd struct { Setup setupCmd `cmd:"" help:"Set up GitContribute for MCP, CLI, or both"` Remove removeCmd `cmd:"" help:"Remove GitContribute coding-agent integrations"` @@ -543,8 +550,7 @@ func (c *CLI) runRemoveCommand(ctx context.Context, cmd *removeCmd) error { return NewCLIError(ExitUsage, err) } if !ok { - _, _ = fmt.Fprintln(c.stderr, "Removal cancelled; no changes were made.") - return nil + return c.writeProgressf("Removal cancelled; no changes were made.\n") } } return c.executeSetup(ctx, contracts.SetupOptions{Remove: true, Clients: clients, AllClients: all, DryRun: cmd.DryRun}, cmd.JSON) @@ -595,7 +601,9 @@ func (c *CLI) promptClients(action string, allowNone bool) ([]string, error) { } func (c *CLI) confirmSetup(prompt string) (bool, error) { - _, _ = fmt.Fprintf(c.stderr, "%s? [Y/n]: ", prompt) + if _, err := fmt.Fprintf(c.stderr, "%s? [Y/n]: ", prompt); err != nil { + return false, fmt.Errorf("write confirmation prompt: %w", err) + } line, err := c.promptLine() if err != nil { return false, err @@ -624,134 +632,6 @@ func (c *CLI) promptLine() (string, error) { } } -func (c *CLI) discoveryService() (contracts.DiscoveryService, error) { - service, ok := c.svc.(contracts.DiscoveryService) - if !ok { - return nil, NewCLIError(ExitNotWired, ErrNotWired) - } - return service, nil -} - -func (c *CLI) tailService() (contracts.TailService, error) { - service, ok := c.svc.(contracts.TailService) - if !ok { - return nil, NewCLIError(ExitNotWired, ErrNotWired) - } - return service, nil -} - -func (c *CLI) controlService() (contracts.ControlService, error) { - service, ok := c.svc.(contracts.ControlService) - if !ok { - return nil, NewCLIError(ExitNotWired, ErrNotWired) - } - return service, nil -} - -func (c *CLI) workflowService() (contracts.WorkflowService, error) { - service, ok := c.svc.(contracts.WorkflowService) - if !ok { - return nil, NewCLIError(ExitNotWired, ErrNotWired) - } - return service, nil -} - -func (c *CLI) dossierService() (contracts.DossierService, error) { - service, ok := c.svc.(contracts.DossierService) - if !ok { - return nil, NewCLIError(ExitNotWired, ErrNotWired) - } - return service, nil -} - -func (c *CLI) investigationService() (contracts.InvestigationService, error) { - service, ok := c.svc.(contracts.InvestigationService) - if !ok { - return nil, NewCLIError(ExitNotWired, ErrNotWired) - } - return service, nil -} - -func (c *CLI) validationService() (contracts.ValidationService, error) { - service, ok := c.svc.(contracts.ValidationService) - if !ok { - return nil, NewCLIError(ExitNotWired, ErrNotWired) - } - return service, nil -} - -func (c *CLI) contributionService() (contracts.ContributionService, error) { - service, ok := c.svc.(contracts.ContributionService) - if !ok { - return nil, NewCLIError(ExitNotWired, ErrNotWired) - } - return service, nil -} - -func (c *CLI) clusteringService() (contracts.ClusteringService, error) { - service, ok := c.svc.(contracts.ClusteringService) - if !ok { - return nil, NewCLIError(ExitNotWired, ErrNotWired) - } - return service, nil -} - -func (c *CLI) lensService() (contracts.LensService, error) { - service, ok := c.svc.(contracts.LensService) - if !ok { - return nil, NewCLIError(ExitNotWired, ErrNotWired) - } - return service, nil -} - -func (c *CLI) collectionService() (contracts.CollectionService, error) { - service, ok := c.svc.(contracts.CollectionService) - if !ok { - return nil, NewCLIError(ExitNotWired, ErrNotWired) - } - return service, nil -} - -func (c *CLI) archiveService() (contracts.ArchiveService, error) { - service, ok := c.svc.(contracts.ArchiveService) - if !ok { - return nil, NewCLIError(ExitNotWired, ErrNotWired) - } - return service, nil -} - -func (c *CLI) localQueryService() (contracts.LocalQueryService, error) { - service, ok := c.svc.(contracts.LocalQueryService) - if !ok { - return nil, NewCLIError(ExitNotWired, ErrNotWired) - } - return service, nil -} - -func (c *CLI) archiveThreadService() (contracts.ArchiveThreadService, error) { - service, ok := c.svc.(contracts.ArchiveThreadService) - if !ok { - return nil, NewCLIError(ExitNotWired, ErrNotWired) - } - return service, nil -} - -func (c *CLI) acquisitionService() (contracts.AcquisitionService, error) { - service, ok := c.svc.(contracts.AcquisitionService) - if !ok { - return nil, NewCLIError(ExitNotWired, ErrNotWired) - } - return service, nil -} - -func (c *CLI) exportService() (contracts.ExportService, error) { - service, ok := c.svc.(contracts.ExportService) - if !ok { - return nil, NewCLIError(ExitNotWired, ErrNotWired) - } - return service, nil -} - func (c *CLI) runSource(ctx context.Context, command string, cmd *sourceCmd) error { service, err := c.discoveryService() if err != nil { @@ -904,7 +784,9 @@ func (c *CLI) runCrawl(ctx context.Context, cmd *crawlCmd) error { if err != nil { return err } - _, _ = fmt.Fprintf(c.stderr, "crawling %s...\n", cmd.Name) + if err := c.writeProgressf("crawling %s...\n", cmd.Name); err != nil { + return err + } result, err := service.Crawl(ctx, cmd.Name, contracts.CrawlOptions{Since: cmd.Since, Budget: cmd.Budget}) if err != nil { return c.mapError(err) @@ -920,7 +802,9 @@ func (c *CLI) runTail(ctx context.Context, cmd *tailCmd) error { if err != nil { return err } - _, _ = fmt.Fprintf(c.stderr, "tailing source %s every %s...\n", cmd.Name, cmd.Interval) + if err := c.writeProgressf("tailing source %s every %s...\n", cmd.Name, cmd.Interval); err != nil { + return err + } result, err := service.TailSource(ctx, cmd.Name, contracts.TailOptions{ Since: cmd.Since, Budget: cmd.Budget, Interval: cmd.Interval, Once: cmd.Once, }) @@ -944,7 +828,9 @@ func (c *CLI) runInvestigation(ctx context.Context, command string, cmd *investi if err != nil { return err } - _, _ = fmt.Fprintf(c.stderr, "starting investigation for %s...\n", repo) + if err := c.writeProgressf("starting investigation for %s...\n", repo); err != nil { + return err + } result, err := service.StartInvestigation(ctx, repo, cmd.Start.Commit, cmd.Start.Lens) if err != nil { return c.mapError(err) @@ -974,7 +860,9 @@ func (c *CLI) runHypothesis(ctx context.Context, command string, cmd *hypothesis } switch command { case "hypothesis add": - _, _ = fmt.Fprintf(c.stderr, "recording hypothesis for investigation %s...\n", cmd.Add.InvestigationID) + if err := c.writeProgressf("recording hypothesis for investigation %s...\n", cmd.Add.InvestigationID); err != nil { + return err + } result, err := service.AddHypothesis(ctx, cmd.Add.InvestigationID, cmd.Add.Title, cmd.Add.Description, cmd.Add.Category) if err != nil { return c.mapError(err) @@ -1023,7 +911,9 @@ func (c *CLI) runOpportunity(ctx context.Context, command string, cmd *opportuni } switch command { case "opportunity promote": - _, _ = fmt.Fprintf(c.stderr, "promoting hypothesis %s to opportunity...\n", cmd.Promote.HypothesisID) + if err := c.writeProgressf("promoting hypothesis %s to opportunity...\n", cmd.Promote.HypothesisID); err != nil { + return err + } result, err := service.PromoteOpportunity(ctx, cmd.Promote.HypothesisID, cmd.Promote.Problem, cmd.Promote.Scope, cmd.Promote.Impact, cmd.Promote.Effort, cmd.Promote.Confidence) if err != nil { return c.mapError(err) @@ -1105,7 +995,9 @@ func (c *CLI) runIndex(ctx context.Context, cmd *indexCmd) error { if err != nil { return err } - _, _ = fmt.Fprintf(c.stderr, "indexing %s from %s...\n", repo, cmd.Path) + if err := c.writeProgressf("indexing %s from %s...\n", repo, cmd.Path); err != nil { + return err + } result, err := c.svc.Index(ctx, repo, cmd.Path) if err != nil { return c.mapError(err) @@ -1122,7 +1014,9 @@ func (c *CLI) runAcquire(ctx context.Context, cmd *acquireCmd) error { if err != nil { return err } - _, _ = fmt.Fprintf(c.stderr, "acquiring and indexing %s...\n", repo) + if err := c.writeProgressf("acquiring and indexing %s...\n", repo); err != nil { + return err + } result, err := service.Acquire(ctx, repo, cmd.Remote) if err != nil { return c.mapError(err) @@ -1131,7 +1025,9 @@ func (c *CLI) runAcquire(ctx context.Context, cmd *acquireCmd) error { } func (c *CLI) runInit(ctx context.Context, cmd *initCmd) error { - _, _ = fmt.Fprintln(c.stderr, "initializing...") + if err := c.writeProgressf("initializing...\n"); err != nil { + return err + } res, err := c.svc.Init(ctx) if err != nil { return c.mapError(err) @@ -1380,8 +1276,7 @@ func (c *CLI) runExport(ctx context.Context, command string, cmd *exportCmd) err if err := os.WriteFile(output, []byte(result.Content), 0600); err != nil { return c.mapError(fmt.Errorf("write export: %w", err)) } - _, _ = fmt.Fprintf(c.stderr, "wrote %s %s export to %s\n", result.Kind, result.Format, output) - return nil + return c.writeProgressf("wrote %s %s export to %s\n", result.Kind, result.Format, output) } _, err = io.WriteString(c.stdout, result.Content) if err == nil && !strings.HasSuffix(result.Content, "\n") { @@ -1450,7 +1345,9 @@ func (c *CLI) runLens(ctx context.Context, command string, cmd *lensCmd) error { if strings.TrimSpace(cmd.Add.Name) == "" { return NewCLIError(ExitUsage, errors.New("lens name is required")) } - _, _ = fmt.Fprintf(c.stderr, "saving lens %s...\n", cmd.Add.Name) + if err := c.writeProgressf("saving lens %s...\n", cmd.Add.Name); err != nil { + return err + } res, err := service.AddLens(ctx, cmd.Add.Name, def) if err != nil { return c.mapError(err) @@ -1503,7 +1400,9 @@ func (c *CLI) runCollection(ctx context.Context, command string, cmd *collection } switch command { case "collection create": - _, _ = fmt.Fprintf(c.stderr, "creating collection %s...\n", cmd.Create.Name) + if err := c.writeProgressf("creating collection %s...\n", cmd.Create.Name); err != nil { + return err + } res, err := service.CreateCollection(ctx, cmd.Create.Name) if err != nil { return c.mapError(err) @@ -1521,7 +1420,9 @@ func (c *CLI) runCollection(ctx context.Context, command string, cmd *collection } members[i] = member } - _, _ = fmt.Fprintf(c.stderr, "adding %d member(s) to collection %s...\n", len(members), cmd.Add.Name) + if err := c.writeProgressf("adding %d member(s) to collection %s...\n", len(members), cmd.Add.Name); err != nil { + return err + } res, err := service.AddCollectionMembers(ctx, cmd.Add.Name, members) if err != nil { return c.mapError(err) diff --git a/internal/cli/cli_parallel_test.go b/internal/cli/cli_parallel_test.go index 88861e1..518954c 100644 --- a/internal/cli/cli_parallel_test.go +++ b/internal/cli/cli_parallel_test.go @@ -2,6 +2,8 @@ package cli_test import ( "context" + "errors" + "io" "os" "path/filepath" "strings" @@ -13,6 +15,10 @@ import ( "github.com/morluto/gitcontribute/internal/health" ) +type failingProgressWriter struct{ err error } + +func (w failingProgressWriter) Write([]byte) (int, error) { return 0, w.err } + func TestIndex(t *testing.T) { t.Parallel() svc := &fakeService{indexResult: &contracts.IndexResult{Repo: contracts.RepoRef{Owner: "o", Repo: "r"}, Commit: "abc", Files: 2}} @@ -191,6 +197,21 @@ func TestCrawlDispatchesBoundedOptions(t *testing.T) { } } +func TestCrawlDoesNotDispatchWhenProgressWriteFails(t *testing.T) { + t.Parallel() + svc := &fakeService{} + want := errors.New("broken stderr") + c := cli.New(svc, nil, io.Discard, failingProgressWriter{err: want}) + err := c.Run(context.Background(), []string{"crawl", "active-go"}) + requireCLIError(t, err, cli.ExitGeneral) + if !errors.Is(err, want) { + t.Fatalf("crawl error = %v, want %v", err, want) + } + if svc.crawlCalled { + t.Fatal("crawl dispatched after its progress write failed") + } +} + func TestCrawlRejectsInvalidBudgetBeforeDispatch(t *testing.T) { t.Parallel() svc := &fakeService{} diff --git a/internal/cli/draft_export_test.go b/internal/cli/draft_export_test.go index 9978759..85c03c7 100644 --- a/internal/cli/draft_export_test.go +++ b/internal/cli/draft_export_test.go @@ -4,6 +4,7 @@ import ( "encoding/json" "os" "path/filepath" + "runtime" "testing" "github.com/morluto/gitcontribute/internal/contracts" @@ -18,6 +19,13 @@ func TestExportDraftPreservesExactBytes(t *testing.T) { if err := exportDraft(dir, draft, false); err != nil { t.Fatal(err) } + info, err := os.Stat(dir) + if err != nil { + t.Fatal(err) + } + if runtime.GOOS != "windows" && info.Mode().Perm()&0o077 != 0 { + t.Errorf("draft directory permissions = %04o, want no group or world access", info.Mode().Perm()) + } for name, want := range map[string]string{"title.txt": draft.Title, "body.md": draft.Body} { got, err := os.ReadFile(filepath.Join(dir, name)) if err != nil { diff --git a/internal/cli/output.go b/internal/cli/output.go index 31eea10..c407ca7 100644 --- a/internal/cli/output.go +++ b/internal/cli/output.go @@ -277,6 +277,9 @@ func controlStatusHuman(r *contracts.ControlStatusResult) string { for _, rate := range r.RateLimits { fmt.Fprintf(&b, "\nGitHub rate limit %s: %d/%d remaining (observed %s)", rate.Resource, rate.Remaining, rate.Limit, rate.ObservedAt) + if rate.Stale { + fmt.Fprint(&b, ", stale") + } if rate.ResetAt != "" { fmt.Fprintf(&b, ", resets %s", rate.ResetAt) } diff --git a/internal/cli/output_internal_test.go b/internal/cli/output_internal_test.go new file mode 100644 index 0000000..fcf8b80 --- /dev/null +++ b/internal/cli/output_internal_test.go @@ -0,0 +1,42 @@ +package cli + +import ( + "errors" + "io" + "strings" + "testing" + + "github.com/morluto/gitcontribute/internal/contracts" +) + +type failingWriter struct{ err error } + +func (w failingWriter) Write([]byte) (int, error) { return 0, w.err } + +func TestConfirmSetupReturnsPromptWriteErrorWithoutReadingInput(t *testing.T) { + want := errors.New("broken stderr") + c := &CLI{ + stdin: failingInput{}, + stderr: failingWriter{err: want}, + } + confirmed, err := c.confirmSetup("Apply changes") + if confirmed || !errors.Is(err, want) { + t.Fatalf("confirm setup = (%t, %v), want false and %v", confirmed, err, want) + } +} + +type failingInput struct{} + +func (failingInput) Read([]byte) (int, error) { return 0, io.ErrUnexpectedEOF } + +func TestControlStatusHumanMarksExpiredRateLimitObservationStale(t *testing.T) { + output := controlStatusHuman(&contracts.ControlStatusResult{ + RateLimits: []contracts.RateLimitState{{ + Resource: "core", Limit: 5000, Remaining: 4999, + ObservedAt: "2026-08-08T23:00:00Z", ResetAt: "2026-08-08T23:59:00Z", Stale: true, + }}, + }) + if !strings.Contains(output, "observed 2026-08-08T23:00:00Z), stale, resets 2026-08-08T23:59:00Z") { + t.Fatalf("stale rate-limit observation was not identified in human output: %q", output) + } +} diff --git a/internal/cli/prepare.go b/internal/cli/prepare.go index 703bd37..80fd0cf 100644 --- a/internal/cli/prepare.go +++ b/internal/cli/prepare.go @@ -55,7 +55,9 @@ func (c *CLI) runPrepare(ctx context.Context, command string, cmd *prepareCmd) e } switch command { case "prepare issue": - _, _ = fmt.Fprintf(c.stderr, "preparing issue draft for opportunity %s...\n", cmd.Issue.OpportunityID) + if err := c.writeProgressf("preparing issue draft for opportunity %s...\n", cmd.Issue.OpportunityID); err != nil { + return err + } result, err := service.PrepareIssue(ctx, cmd.Issue.OpportunityID, contracts.PrepareIssueOptions{ Guidance: cmd.Issue.Guidance, Success: cmd.Issue.Success, ManifestID: cmd.Issue.ManifestID, }) @@ -67,7 +69,9 @@ func (c *CLI) runPrepare(ctx context.Context, command string, cmd *prepareCmd) e } return c.render(cmd.Issue.JSON, result) case "prepare pr": - _, _ = fmt.Fprintf(c.stderr, "preparing pull request draft for opportunity %s...\n", cmd.PR.OpportunityID) + if err := c.writeProgressf("preparing pull request draft for opportunity %s...\n", cmd.PR.OpportunityID); err != nil { + return err + } result, err := service.PreparePullRequest(ctx, cmd.PR.OpportunityID, contracts.PreparePROptions{ WorkspaceID: cmd.PR.WorkspaceID, Approach: cmd.PR.Approach, @@ -120,7 +124,7 @@ func exportDraft(dir string, draft *contracts.DraftResult, allowWarnings bool) e } } } - if err := os.MkdirAll(dir, 0o755); err != nil { + if err := os.MkdirAll(dir, 0o700); err != nil { return fmt.Errorf("create draft export directory: %w", err) } metadata, err := json.MarshalIndent(draft, "", " ") diff --git a/internal/cli/services.go b/internal/cli/services.go new file mode 100644 index 0000000..1605ca7 --- /dev/null +++ b/internal/cli/services.go @@ -0,0 +1,61 @@ +package cli + +import "github.com/morluto/gitcontribute/internal/contracts" + +func serviceOrNotWired[T any](service any) (T, error) { + typed, ok := service.(T) + if !ok { + var zero T + return zero, NewCLIError(ExitNotWired, ErrNotWired) + } + return typed, nil +} + +func (c *CLI) discoveryService() (contracts.DiscoveryService, error) { + return serviceOrNotWired[contracts.DiscoveryService](c.svc) +} +func (c *CLI) tailService() (contracts.TailService, error) { + return serviceOrNotWired[contracts.TailService](c.svc) +} +func (c *CLI) controlService() (contracts.ControlService, error) { + return serviceOrNotWired[contracts.ControlService](c.svc) +} +func (c *CLI) workflowService() (contracts.WorkflowService, error) { + return serviceOrNotWired[contracts.WorkflowService](c.svc) +} +func (c *CLI) dossierService() (contracts.DossierService, error) { + return serviceOrNotWired[contracts.DossierService](c.svc) +} +func (c *CLI) investigationService() (contracts.InvestigationService, error) { + return serviceOrNotWired[contracts.InvestigationService](c.svc) +} +func (c *CLI) validationService() (contracts.ValidationService, error) { + return serviceOrNotWired[contracts.ValidationService](c.svc) +} +func (c *CLI) contributionService() (contracts.ContributionService, error) { + return serviceOrNotWired[contracts.ContributionService](c.svc) +} +func (c *CLI) clusteringService() (contracts.ClusteringService, error) { + return serviceOrNotWired[contracts.ClusteringService](c.svc) +} +func (c *CLI) lensService() (contracts.LensService, error) { + return serviceOrNotWired[contracts.LensService](c.svc) +} +func (c *CLI) collectionService() (contracts.CollectionService, error) { + return serviceOrNotWired[contracts.CollectionService](c.svc) +} +func (c *CLI) archiveService() (contracts.ArchiveService, error) { + return serviceOrNotWired[contracts.ArchiveService](c.svc) +} +func (c *CLI) localQueryService() (contracts.LocalQueryService, error) { + return serviceOrNotWired[contracts.LocalQueryService](c.svc) +} +func (c *CLI) archiveThreadService() (contracts.ArchiveThreadService, error) { + return serviceOrNotWired[contracts.ArchiveThreadService](c.svc) +} +func (c *CLI) acquisitionService() (contracts.AcquisitionService, error) { + return serviceOrNotWired[contracts.AcquisitionService](c.svc) +} +func (c *CLI) exportService() (contracts.ExportService, error) { + return serviceOrNotWired[contracts.ExportService](c.svc) +} diff --git a/internal/cli/tracking.go b/internal/cli/tracking.go index f344247..5f1f7e8 100644 --- a/internal/cli/tracking.go +++ b/internal/cli/tracking.go @@ -112,7 +112,9 @@ func (c *CLI) runTriage(ctx context.Context, command string, cmd *triageCmd) err } switch command { case "triage record": - _, _ = fmt.Fprintf(c.stderr, "recording triage outcome for %s...\n", cmd.Record.Target) + if err := c.writeProgressf("recording triage outcome for %s...\n", cmd.Record.Target); err != nil { + return err + } result, err := service.RecordTriageEvent(ctx, contracts.RecordTriageEventOptions{ Target: cmd.Record.Target, Outcome: cmd.Record.Outcome, @@ -150,7 +152,9 @@ func (c *CLI) runContribution(ctx context.Context, command string, cmd *contribu } switch command { case "contribution record": - _, _ = fmt.Fprintf(c.stderr, "recording contribution for opportunity %s...\n", cmd.Record.OpportunityID) + if err := c.writeProgressf("recording contribution for opportunity %s...\n", cmd.Record.OpportunityID); err != nil { + return err + } result, err := service.RecordContribution(ctx, contracts.RecordContributionOptions{ OpportunityID: cmd.Record.OpportunityID, Kind: cmd.Record.Kind, @@ -183,7 +187,9 @@ func (c *CLI) runContribution(ctx context.Context, command string, cmd *contribu } return c.render(cmd.Show.JSON, result) case "contribution outcome": - _, _ = fmt.Fprintf(c.stderr, "recording outcome %s for contribution %s...\n", cmd.Outcome.Outcome, cmd.Outcome.ContributionID) + if err := c.writeProgressf("recording outcome %s for contribution %s...\n", cmd.Outcome.Outcome, cmd.Outcome.ContributionID); err != nil { + return err + } result, err := service.RecordContributionOutcome(ctx, contracts.RecordContributionOutcomeOptions{ ContributionID: cmd.Outcome.ContributionID, Outcome: cmd.Outcome.Outcome, @@ -223,7 +229,9 @@ func (c *CLI) runTrackingExport(ctx context.Context, cmd *trackingExportCmd, ser if cmd.Limit <= 0 || cmd.Limit > 100000 { return NewCLIError(ExitUsage, errors.New("limit must be between 1 and 100000")) } - _, _ = fmt.Fprintln(c.stderr, "exporting local tracking metadata...") + if err := c.writeProgressf("exporting local tracking metadata...\n"); err != nil { + return err + } result, err := service.ExportLocalMetadata(ctx, contracts.MetadataExportOptions{Limit: cmd.Limit}) if err != nil { return c.mapError(err) @@ -245,7 +253,9 @@ func (c *CLI) runTrackingExport(ctx context.Context, cmd *trackingExportCmd, ser return c.mapError(err) } if len(result.Data) == 0 || result.Data[len(result.Data)-1] != '\n' { - _, _ = fmt.Fprintln(c.stdout) + if _, err := fmt.Fprintln(c.stdout); err != nil { + return c.mapError(err) + } } return nil } @@ -255,7 +265,9 @@ func (c *CLI) runTrackingImport(ctx context.Context, cmd *trackingImportCmd, ser if err != nil { return NewCLIError(ExitUsage, err) } - _, _ = fmt.Fprintln(c.stderr, "importing local tracking metadata...") + if err := c.writeProgressf("importing local tracking metadata...\n"); err != nil { + return err + } result, err := service.ImportLocalMetadata(ctx, contracts.MetadataImportOptions{Data: data}) if err != nil { return c.mapError(err) diff --git a/internal/cli/upgrade_cli.go b/internal/cli/upgrade_cli.go index fdca831..e69c19f 100644 --- a/internal/cli/upgrade_cli.go +++ b/internal/cli/upgrade_cli.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "strings" "github.com/morluto/gitcontribute/internal/contracts" ) @@ -22,8 +23,7 @@ func (c *CLI) runUpgrade(ctx context.Context, cmd *upgradeCmd) error { return NewCLIError(ExitUsage, err) } if !confirmed { - _, _ = fmt.Fprintln(c.stderr, "Upgrade cancelled.") - return nil + return c.writeProgressf("Upgrade cancelled.\n") } cmd.Yes = true } @@ -34,27 +34,26 @@ func (c *CLI) runUpgrade(ctx context.Context, cmd *upgradeCmd) error { if cmd.JSON { return writeJSON(c.stdout, report) } - _, err = fmt.Fprintf(c.stdout, "Upgrade [%s]: %s", report.Context, report.Status) + var output strings.Builder + fmt.Fprintf(&output, "Upgrade [%s]: %s", report.Context, report.Status) if report.Latest != "" { - _, err = fmt.Fprintf(c.stdout, " (current %s, latest %s)", report.Current, report.Latest) + fmt.Fprintf(&output, " (current %s, latest %s)", report.Current, report.Latest) } if report.Command != "" { - _, err = fmt.Fprintf(c.stdout, "\n%s", report.Command) + fmt.Fprintf(&output, "\n%s", report.Command) } for _, stage := range report.Stages { - _, err = fmt.Fprintf(c.stdout, "\n- %s: %s", stage.Name, stage.Status) + fmt.Fprintf(&output, "\n- %s: %s", stage.Name, stage.Status) if stage.Message != "" { - _, err = fmt.Fprintf(c.stdout, " — %s", stage.Message) + fmt.Fprintf(&output, " — %s", stage.Message) } } if report.Action != "" { - _, err = fmt.Fprintf(c.stdout, "\nNext: %s", report.Action) + fmt.Fprintf(&output, "\nNext: %s", report.Action) } if report.Rollback != "" { - _, err = fmt.Fprintf(c.stdout, "\nRollback: %s", report.Rollback) - } - if err == nil { - _, err = fmt.Fprintln(c.stdout) + fmt.Fprintf(&output, "\nRollback: %s", report.Rollback) } + _, err = fmt.Fprintln(c.stdout, output.String()) return err } diff --git a/internal/cli/upgrade_cli_test.go b/internal/cli/upgrade_cli_test.go index 3d5bc09..3a22a4f 100644 --- a/internal/cli/upgrade_cli_test.go +++ b/internal/cli/upgrade_cli_test.go @@ -3,6 +3,7 @@ package cli_test import ( "bytes" "context" + "errors" "os" "strings" "testing" @@ -13,16 +14,34 @@ import ( type fakeUpgradeService struct { *fakeService - calls int - opts contracts.UpgradeOptions + calls int + opts contracts.UpgradeOptions + report *contracts.UpgradeReport } func (s *fakeUpgradeService) Upgrade(_ context.Context, opts contracts.UpgradeOptions) (*contracts.UpgradeReport, error) { s.calls++ s.opts = opts + if s.report != nil { + return s.report, nil + } return &contracts.UpgradeReport{}, nil } +type failFirstWriter struct { + bytes.Buffer + err error + writes int +} + +func (w *failFirstWriter) Write(data []byte) (int, error) { + w.writes++ + if w.writes == 1 { + return 0, w.err + } + return w.Buffer.Write(data) +} + func TestUpgradeDoesNotPromptWhenStandardOutputIsRedirected(t *testing.T) { redirected, err := os.CreateTemp(t.TempDir(), "upgrade-stdout") if err != nil { @@ -59,3 +78,20 @@ func TestUpgradeConsentDescribesCheckAndEligibleManagedUpdate(t *testing.T) { t.Fatalf("consent prompt = %q", output) } } + +func TestUpgradeDoesNotLoseAnEarlierOutputFailure(t *testing.T) { + want := errors.New("broken stdout") + service := &fakeUpgradeService{fakeService: &fakeService{}, report: &contracts.UpgradeReport{ + Context: "managed", Status: "updated", Latest: "1.2.3", Current: "1.2.2", Command: "npm install", + }} + stdout := &failFirstWriter{err: want} + var stderr bytes.Buffer + c := cli.New(service, nil, stdout, &stderr) + err := c.Run(context.Background(), []string{"upgrade", "--yes"}) + if !errors.Is(err, want) { + t.Fatalf("upgrade error = %v, want %v", err, want) + } + if stdout.writes != 1 { + t.Fatalf("upgrade wrote %d times after output failure, want 1", stdout.writes) + } +} diff --git a/internal/contracts/application_contracts.go b/internal/contracts/application_contracts.go index 09681d7..70a8bf7 100644 --- a/internal/contracts/application_contracts.go +++ b/internal/contracts/application_contracts.go @@ -119,6 +119,7 @@ type RateLimitState struct { Remaining int `json:"remaining"` Used int `json:"used"` ResetAt string `json:"reset_at,omitempty"` + Stale bool `json:"stale"` StatusCode int `json:"status_code"` ObservedAt string `json:"observed_at"` } diff --git a/internal/corpus/observations_test.go b/internal/corpus/observations_test.go index d24d1e6..146d355 100644 --- a/internal/corpus/observations_test.go +++ b/internal/corpus/observations_test.go @@ -131,7 +131,7 @@ func TestListPullRequestPortfolioFiltersByAuthorAndState(t *testing.T) { } } - got, err := c.ListPullRequestPortfolio(ctx, "ALICE", "OPEN", 10) + got, err := c.ListPullRequestPortfolio(ctx, "ALICE", "OPEN", nil, 10) if err != nil { t.Fatalf("list pull request portfolio: %v", err) } @@ -142,14 +142,14 @@ func TestListPullRequestPortfolioFiltersByAuthorAndState(t *testing.T) { t.Fatalf("portfolio item = %+v, want owner/repo#1", got[0]) } - got, err = c.ListPullRequestPortfolio(ctx, "alice", "all", 10) + got, err = c.ListPullRequestPortfolio(ctx, "alice", "all", nil, 10) if err != nil { t.Fatalf("list pull request portfolio for all states: %v", err) } if len(got) != 2 || got[0].Thread.Number != 2 || got[1].Thread.Number != 1 { t.Fatalf("all-state portfolio = %+v, want #2 then #1", got) } - page, err := c.ListPullRequestPortfolioPage(ctx, "alice", "all", 1) + page, err := c.ListPullRequestPortfolioPage(ctx, "alice", "all", nil, 1) if err != nil { t.Fatal(err) } @@ -193,7 +193,7 @@ func TestListPullRequestPortfolioUsesDeterministicGlobalOrder(t *testing.T) { } } - got, err := c.ListPullRequestPortfolio(ctx, "", "", 100) + got, err := c.ListPullRequestPortfolio(ctx, "", "", nil, 100) if err != nil { t.Fatalf("list pull request portfolio: %v", err) } diff --git a/internal/corpus/portfolio.go b/internal/corpus/portfolio.go index 874b18f..d9f9778 100644 --- a/internal/corpus/portfolio.go +++ b/internal/corpus/portfolio.go @@ -13,8 +13,8 @@ import ( // state "all" is equivalent to no state filter. The read is bounded and // deterministic so callers can build portfolio views without repository-level // N+1 queries. -func (c *Corpus) ListPullRequestPortfolio(ctx context.Context, author, state string, limit int) (_ []PortfolioPullRequest, err error) { - page, err := c.ListPullRequestPortfolioPage(ctx, author, state, limit) +func (c *Corpus) ListPullRequestPortfolio(ctx context.Context, author, state string, repository *RepositoryKey, limit int) (_ []PortfolioPullRequest, err error) { + page, err := c.ListPullRequestPortfolioPage(ctx, author, state, repository, limit) if err != nil { return nil, err } @@ -23,7 +23,7 @@ func (c *Corpus) ListPullRequestPortfolio(ctx context.Context, author, state str // ListPullRequestPortfolioPage returns a bounded portfolio and the exact // matching population so callers never mistake the page size for the total. -func (c *Corpus) ListPullRequestPortfolioPage(ctx context.Context, author, state string, limit int) (_ PortfolioPage, err error) { +func (c *Corpus) ListPullRequestPortfolioPage(ctx context.Context, author, state string, repository *RepositoryKey, limit int) (_ PortfolioPage, err error) { if limit <= 0 { limit = 1000 } @@ -48,16 +48,24 @@ func (c *Corpus) ListPullRequestPortfolioPage(ctx context.Context, author, state query += ` AND lower(t.author) = lower(?)` args = append(args, author) } + if repository != nil { + query += ` AND lower(r.owner) = lower(?) AND lower(r.name) = lower(?)` + args = append(args, repository.Owner, repository.Name) + } if state != "" && !strings.EqualFold(state, "all") { query += ` AND lower(t.state) = lower(?)` args = append(args, state) } - countQuery := `SELECT COUNT(*) FROM threads t WHERE t.kind = ?` + countQuery := `SELECT COUNT(*) FROM threads t JOIN repositories r ON r.id = t.repository_id WHERE t.kind = ?` countArgs := []any{ThreadKindPullRequest} if author != "" { countQuery += ` AND lower(t.author) = lower(?)` countArgs = append(countArgs, author) } + if repository != nil { + countQuery += ` AND lower(r.owner) = lower(?) AND lower(r.name) = lower(?)` + countArgs = append(countArgs, repository.Owner, repository.Name) + } if state != "" && !strings.EqualFold(state, "all") { countQuery += ` AND lower(t.state) = lower(?)` countArgs = append(countArgs, state) diff --git a/internal/github/attempt_timeout_transport.go b/internal/github/attempt_timeout_transport.go new file mode 100644 index 0000000..32bc937 --- /dev/null +++ b/internal/github/attempt_timeout_transport.go @@ -0,0 +1,51 @@ +package github + +import ( + "context" + "io" + "net/http" + "time" +) + +// attemptTimeoutTransport bounds one network attempt without consuming retry +// backoff or rate-limiter wait time from the caller's request budget. +type attemptTimeoutTransport struct { + Base http.RoundTripper + Timeout time.Duration +} + +func (t *attemptTimeoutTransport) base() http.RoundTripper { + if t.Base != nil { + return t.Base + } + return http.DefaultTransport +} + +func (t *attemptTimeoutTransport) RoundTrip(req *http.Request) (*http.Response, error) { + if t.Timeout <= 0 { + return t.base().RoundTrip(req) + } + ctx, cancel := context.WithTimeout(req.Context(), t.Timeout) + resp, err := t.base().RoundTrip(req.Clone(ctx)) + if err != nil { + cancel() + return nil, err + } + if resp.Body == nil { + cancel() + return resp, nil + } + resp.Body = &cancelOnClose{ReadCloser: resp.Body, cancel: cancel} + return resp, nil +} + +type cancelOnClose struct { + io.ReadCloser + cancel context.CancelFunc +} + +func (r *cancelOnClose) Close() error { + err := r.ReadCloser.Close() + r.cancel() + return err +} diff --git a/internal/github/client.go b/internal/github/client.go index 63bb331..fc58b88 100644 --- a/internal/github/client.go +++ b/internal/github/client.go @@ -18,6 +18,7 @@ const ( DefaultUploadURL = "https://uploads.github.com/" DefaultRequestsPerSecond = 10.0 DefaultBurst = 20 + defaultHTTPTimeout = 30 * time.Second ) // Reader is the product-owned read contract for GitHub. @@ -110,6 +111,7 @@ func NewClient(cfg Config) (*Client, error) { if cfg.UploadURL == "" { cfg.UploadURL = DefaultUploadURL } + useDefaultAttemptTimeout := cfg.HTTPClient == nil if cfg.HTTPClient == nil { cfg.HTTPClient = &http.Client{} } @@ -118,6 +120,9 @@ func NewClient(cfg Config) (*Client, error) { if baseTransport == nil { baseTransport = http.DefaultTransport } + if useDefaultAttemptTimeout { + baseTransport = &attemptTimeoutTransport{Base: baseTransport, Timeout: defaultHTTPTimeout} + } limiter := cfg.Limiter if limiter == nil { diff --git a/internal/github/client_config_test.go b/internal/github/client_config_test.go new file mode 100644 index 0000000..b21a78e --- /dev/null +++ b/internal/github/client_config_test.go @@ -0,0 +1,32 @@ +package github + +import ( + "net/http" + "testing" + "time" +) + +func TestNewClientUsesBoundedDefaultAttemptTimeout(t *testing.T) { + client, err := NewClient(Config{}) + if err != nil { + t.Fatalf("new client: %v", err) + } + if client.downloadClient.Timeout != 0 { + t.Fatalf("default client timeout = %s, want no whole-request timeout", client.downloadClient.Timeout) + } + transport, ok := client.downloadClient.Transport.(*attemptTimeoutTransport) + if !ok || transport.Timeout != defaultHTTPTimeout { + t.Fatalf("default download transport = %#v, want %s per attempt", client.downloadClient.Transport, defaultHTTPTimeout) + } +} + +func TestNewClientPreservesExplicitHTTPTimeout(t *testing.T) { + const timeout = 7 * time.Second + client, err := NewClient(Config{HTTPClient: &http.Client{Timeout: timeout}}) + if err != nil { + t.Fatalf("new client: %v", err) + } + if client.downloadClient.Timeout != timeout { + t.Fatalf("explicit HTTP timeout = %s, want %s", client.downloadClient.Timeout, timeout) + } +} diff --git a/internal/github/client_portfolio_test.go b/internal/github/client_portfolio_test.go index 40b84fd..b5f337f 100644 --- a/internal/github/client_portfolio_test.go +++ b/internal/github/client_portfolio_test.go @@ -82,6 +82,16 @@ func TestSearchAuthoredPullRequestsBuildsQueryAndExtractsRepository(t *testing.T wantPage: 1, wantPerPage: 50, }, + { + name: "scopes authored discovery before pagination", + opts: AuthoredPullRequestSearchOptions{ + Login: "morluto", RepositoryOwner: "lab", RepositoryName: "runtime", State: "open", + PageOptions: PageOptions{Page: 1, PerPage: 1}, + }, + wantQuery: "is:pr author:morluto repo:lab/runtime is:open", + wantPage: 1, + wantPerPage: 1, + }, } for _, tt := range tests { diff --git a/internal/github/pull_request_workflows.go b/internal/github/pull_request_workflows.go index f715cec..e7f96d2 100644 --- a/internal/github/pull_request_workflows.go +++ b/internal/github/pull_request_workflows.go @@ -233,7 +233,7 @@ func (c *Client) feedbackInlineComments(ctx context.Context, owner, repo string, return items, FeedbackCoverage{Fetched: len(items), Total: 0, Reason: "item_limit_reached"}, nil } -const pullRequestFeedbackThreadsQuery = `query PullRequestFeedback($owner: String!, $repo: String!, $number: Int!, $first: Int!, $after: String) { +const pullRequestFeedbackThreadsQuery = `query PullRequestFeedback($owner: String!, $repo: String!, $number: Int!, $first: Int!, $after: String, $commentFirst: Int!) { repository(owner: $owner, name: $repo) { pullRequest(number: $number) { headRefOid @@ -242,7 +242,7 @@ const pullRequestFeedbackThreadsQuery = `query PullRequestFeedback($owner: Strin totalCount nodes { id isResolved isOutdated path line startLine resolvedBy { login } - comments(first: 100) { + comments(first: $commentFirst) { totalCount nodes { id databaseId body createdAt updatedAt path line startLine side startSide outdated commit { oid } author { login } replyTo { databaseId } } pageInfo { hasNextPage endCursor } @@ -324,6 +324,7 @@ func (c *Client) feedbackReviewThreads(ctx context.Context, owner, repo string, items := make([]FeedbackThread, 0, opts.MaxItemsPerChannel) cursor := "" total := 0 + comments := 0 var head string var updated time.Time for len(items) < opts.MaxItemsPerChannel { @@ -332,6 +333,7 @@ func (c *Client) feedbackReviewThreads(ctx context.Context, owner, repo string, } body := graphQLRequest{Query: pullRequestFeedbackThreadsQuery, Variables: map[string]any{ "owner": owner, "repo": repo, "number": number, "first": min(100, opts.MaxItemsPerChannel-len(items)), "after": optionalGraphQLCursor(cursor), + "commentFirst": 1, }} req, err := c.gh.NewRequest(ctx, http.MethodPost, "graphql", body) if err != nil { @@ -352,7 +354,9 @@ func (c *Client) feedbackReviewThreads(ctx context.Context, owner, repo string, return nil, "", time.Time{}, FeedbackCoverage{}, &TransientError{Cause: errors.New("pull request changed while feedback was paged")} } total = pr.Threads.TotalCount - for _, node := range pr.Threads.Nodes { + first := len(items) + sourceIndexes := make([]int, 0, len(pr.Threads.Nodes)) + for nodeIndex, node := range pr.Threads.Nodes { if opts.ThreadState == "unresolved" && node.IsResolved { continue } @@ -362,21 +366,35 @@ func (c *Client) feedbackReviewThreads(ctx context.Context, owner, repo string, } thread := FeedbackThread{ID: node.ID, Resolved: node.IsResolved, ResolvedBy: resolvedBy, Outdated: node.IsOutdated, Path: node.Path, Line: node.Line, StartLine: node.StartLine, TotalCount: node.Comments.TotalCount} thread.Comments = appendFeedbackComments(thread.Comments, node.Comments.Nodes) - if node.Comments.PageInfo.HasNextPage { - comments, err := c.pageReviewThreadComments(ctx, node.ID, node.Comments.PageInfo.EndCursor, budget) - if err != nil { - return nil, "", time.Time{}, FeedbackCoverage{}, err - } - thread.Comments = append(thread.Comments, comments...) - } - thread.Truncated = len(thread.Comments) < thread.TotalCount + comments += len(thread.Comments) items = append(items, thread) + sourceIndexes = append(sourceIndexes, nodeIndex) if len(items) == opts.MaxItemsPerChannel { break } } + for index := first; index < len(items); index++ { + thread := &items[index] + if source := pr.Threads.Nodes[sourceIndexes[index-first]]; source.Comments.PageInfo.HasNextPage && thread.TotalCount > len(thread.Comments) && comments < opts.MaxItemsPerChannel { + more, truncated, err := c.pageReviewThreadComments(ctx, thread.ID, source.Comments.PageInfo.EndCursor, opts.MaxItemsPerChannel-comments, budget) + if err != nil { + return nil, "", time.Time{}, FeedbackCoverage{}, err + } + thread.Comments = append(thread.Comments, more...) + comments += len(more) + thread.Truncated = truncated + } + thread.Truncated = thread.Truncated || len(thread.Comments) < thread.TotalCount + } if !pr.Threads.PageInfo.HasNextPage { - return items, head, updated, FeedbackCoverage{Complete: true, Fetched: len(items), Total: total}, nil + coverage := FeedbackCoverage{Complete: true, Fetched: len(items), Total: total} + for _, thread := range items { + if thread.Truncated { + coverage.Complete, coverage.Reason = false, "item_limit_reached" + break + } + } + return items, head, updated, coverage, nil } cursor = pr.Threads.PageInfo.EndCursor } @@ -400,16 +418,16 @@ func appendFeedbackComments(dst []FeedbackComment, comments []feedbackCommentNod return dst } -func (c *Client) pageReviewThreadComments(ctx context.Context, id, cursor string, budget *RequestBudget) ([]FeedbackComment, error) { +func (c *Client) pageReviewThreadComments(ctx context.Context, id, cursor string, limit int, budget *RequestBudget) ([]FeedbackComment, bool, error) { var items []FeedbackComment - for { + for len(items) < limit { if err := budget.Take(); err != nil { - return nil, err + return nil, false, err } - body := graphQLRequest{Query: reviewThreadCommentsQuery, Variables: map[string]any{"id": id, "first": 100, "after": optionalGraphQLCursor(cursor)}} + body := graphQLRequest{Query: reviewThreadCommentsQuery, Variables: map[string]any{"id": id, "first": min(100, limit-len(items)), "after": optionalGraphQLCursor(cursor)}} req, err := c.gh.NewRequest(ctx, http.MethodPost, "graphql", body) if err != nil { - return nil, err + return nil, false, err } req = markReplayableRead(req) var envelope struct { @@ -423,17 +441,18 @@ func (c *Client) pageReviewThreadComments(ctx context.Context, id, cursor string } `json:"errors"` } if _, err := c.gh.Do(req, &envelope); err != nil { - return nil, classifyError(err) + return nil, false, classifyError(err) } if len(envelope.Errors) > 0 { - return nil, fmt.Errorf("github graphql: %s", envelope.Errors[0].Message) + return nil, false, fmt.Errorf("github graphql: %s", envelope.Errors[0].Message) } items = appendFeedbackComments(items, envelope.Data.Node.Comments.Nodes) if !envelope.Data.Node.Comments.PageInfo.HasNextPage { - return items, nil + return items, false, nil } cursor = envelope.Data.Node.Comments.PageInfo.EndCursor } + return items, true, nil } type CIFailureOptions struct { diff --git a/internal/github/pull_request_workflows_test.go b/internal/github/pull_request_workflows_test.go index c936018..bc11ab6 100644 --- a/internal/github/pull_request_workflows_test.go +++ b/internal/github/pull_request_workflows_test.go @@ -86,6 +86,48 @@ func TestGetPullRequestFeedbackEnforcesTotalRequestBudget(t *testing.T) { } } +func TestGetPullRequestFeedbackCapsNestedReviewThreadComments(t *testing.T) { + requests := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests++ + switch { + case r.Method == http.MethodGet && r.URL.Path == "/api/v3/repos/acme/project/pulls/7": + writeJSON(w, map[string]any{"updated_at": "2026-07-30T10:00:00Z", "head": map[string]any{"sha": "head-7"}}) + case r.Method == http.MethodPost && r.URL.Path == "/api/v3/graphql": + var request struct { + Variables map[string]any `json:"variables"` + } + if err := json.NewDecoder(r.Body).Decode(&request); err != nil { + t.Fatalf("decode GraphQL request: %v", err) + } + if request.Variables["commentFirst"] != float64(1) { + t.Fatalf("commentFirst = %#v, want 1", request.Variables["commentFirst"]) + } + writeJSON(w, map[string]any{"data": map[string]any{"repository": map[string]any{"pullRequest": map[string]any{ + "headRefOid": "head-7", "updatedAt": "2026-07-30T10:00:00Z", + "reviewThreads": map[string]any{"totalCount": 2, "pageInfo": map[string]any{"hasNextPage": false}, "nodes": []any{ + map[string]any{"id": "T1", "comments": map[string]any{"totalCount": 2, "nodes": []any{map[string]any{"id": "C1", "databaseId": 1}}, "pageInfo": map[string]any{"hasNextPage": true, "endCursor": "next"}}}, + map[string]any{"id": "T2", "comments": map[string]any{"totalCount": 1, "nodes": []any{map[string]any{"id": "C2", "databaseId": 2}}, "pageInfo": map[string]any{"hasNextPage": false}}}, + }}, + }}}}) + default: + http.Error(w, r.Method+" "+r.URL.Path, http.StatusNotFound) + } + })) + defer srv.Close() + + got, err := newTestClient(t, srv, nil).GetPullRequestFeedback(context.Background(), "acme", "project", 7, PullRequestFeedbackOptions{ + Channels: []string{"review_threads"}, ThreadState: "all", MaxItemsPerChannel: 2, + }, NewRequestBudget(2)) + if err != nil { + t.Fatal(err) + } + coverage := got.Coverage["review_threads"] + if requests != 2 || coverage.Complete || coverage.Reason != "item_limit_reached" || len(got.ReviewThreads) != 2 || !got.ReviewThreads[0].Truncated || len(got.ReviewThreads[0].Comments) != 1 || len(got.ReviewThreads[1].Comments) != 1 { + t.Fatalf("requests=%d coverage=%+v threads=%+v", requests, coverage, got.ReviewThreads) + } +} + func TestGetPullRequestCINormalizesProvidersAndBoundsLogs(t *testing.T) { var baseURL string logUsedConfiguredTransport := false diff --git a/internal/mcpadapter/runner.go b/internal/mcpadapter/runner.go index 9ada3d8..9c922ff 100644 --- a/internal/mcpadapter/runner.go +++ b/internal/mcpadapter/runner.go @@ -26,6 +26,9 @@ func (r *Runner) Run(ctx context.Context, opts contracts.MCPOptions) error { if opts.Transport != "stdio" { return fmt.Errorf("unsupported mcp transport %q", opts.Transport) } + if r == nil || r.service == nil { + return fmt.Errorf("mcp service is required") + } newServer := mcpserver.New if opts.ReadOnly { newServer = mcpserver.NewReadOnly diff --git a/internal/mcpadapter/runner_test.go b/internal/mcpadapter/runner_test.go index aa5defc..d35ff7e 100644 --- a/internal/mcpadapter/runner_test.go +++ b/internal/mcpadapter/runner_test.go @@ -14,3 +14,10 @@ func TestRunnerRejectsUnsupportedTransportBeforeUsingService(t *testing.T) { t.Fatalf("run error = %v", err) } } + +func TestRunnerRejectsMissingService(t *testing.T) { + err := New(nil, "test").Run(context.Background(), contracts.MCPOptions{Transport: "stdio"}) + if err == nil || !strings.Contains(err.Error(), "mcp service is required") { + t.Fatalf("run error = %v", err) + } +} diff --git a/internal/mcpcontract/github_acquisition_contracts.go b/internal/mcpcontract/github_acquisition_contracts.go index 3b47fd0..5722ad4 100644 --- a/internal/mcpcontract/github_acquisition_contracts.go +++ b/internal/mcpcontract/github_acquisition_contracts.go @@ -14,15 +14,14 @@ type GitHubRateOutput struct { // The repository is required; live code search is intentionally not part of // this operation. type SearchGitHubThreadsInput struct { - Owner string `json:"owner" jsonschema:"GitHub repository owner"` - Repo string `json:"repo" jsonschema:"GitHub repository name"` - Query string `json:"query" jsonschema:"User search text or GitHub issue-search qualifiers"` - Kind string `json:"kind,omitempty" jsonschema:"Optional issue or pull_request filter"` - State string `json:"state,omitempty" jsonschema:"Optional open, closed, or all state filter"` - Sort string `json:"sort,omitempty" jsonschema:"Optional GitHub issue-search sort: comments, created, updated, or reactions"` - Order string `json:"order,omitempty" jsonschema:"Optional asc or desc order"` - Page int `json:"page,omitempty" jsonschema:"GitHub result page from 1 to 1000"` - Limit int `json:"limit,omitempty" jsonschema:"Results per page from 1 to 100"` + Repository RepositoryRef `json:"repository" jsonschema:"GitHub repository scope"` + Query string `json:"query" jsonschema:"User search text or GitHub issue-search qualifiers"` + Kind string `json:"kind,omitempty" jsonschema:"Optional issue or pull_request filter"` + State string `json:"state,omitempty" jsonschema:"Optional open, closed, or all state filter"` + Sort string `json:"sort,omitempty" jsonschema:"Optional GitHub issue-search sort: comments, created, updated, or reactions"` + Order string `json:"order,omitempty" jsonschema:"Optional asc or desc order"` + Page int `json:"page,omitempty" jsonschema:"GitHub result page from 1 to 1000"` + Limit int `json:"limit,omitempty" jsonschema:"Results per page from 1 to 100"` } // SearchGitHubThreadsOutput is the compact live result. The complete ordered @@ -114,8 +113,7 @@ type GitHubAcquisitionProvenance struct { // ref. Named refs are resolved before content is read and are not authoritative // provenance. type ReadSourceFilesInput struct { - Owner string `json:"owner" jsonschema:"GitHub repository owner"` - Repo string `json:"repo" jsonschema:"GitHub repository name"` + Repository RepositoryRef `json:"repository" jsonschema:"GitHub repository scope"` Ref string `json:"ref" jsonschema:"Commit SHA, branch, or tag; resolved commit is authoritative"` Files []SourceFileRequest `json:"files" jsonschema:"Ordered repository-relative files with optional inclusive line ranges"` PerFileBytes int `json:"per_file_bytes,omitempty" jsonschema:"Maximum decoded bytes per file from 1 to 1048576"` diff --git a/internal/mcpcontract/operation_contracts.go b/internal/mcpcontract/operation_contracts.go index fb2d3d8..272a954 100644 --- a/internal/mcpcontract/operation_contracts.go +++ b/internal/mcpcontract/operation_contracts.go @@ -75,6 +75,7 @@ type JobArtifactReference struct { References []string `json:"references,omitempty" jsonschema:"Bounded exact repository, thread, or pull-request references produced by the job"` ReferencesTruncated bool `json:"references_truncated,omitempty" jsonschema:"Whether more exact references exist than this bounded response includes"` Failures []JobArtifactFailure `json:"failures,omitempty" jsonschema:"Bounded per-reference outcomes that require retry or recovery"` + FailuresTruncated bool `json:"failures_truncated,omitempty" jsonschema:"Whether more failed outcomes exist than this bounded response includes"` CodeIndex *CodeIndexArtifact `json:"code_index,omitempty" jsonschema:"Revision-bound indexed-commit artifact"` Status string `json:"status,omitempty" jsonschema:"Artifact completeness status"` DiscoveryStatus string `json:"discovery_status,omitempty"` diff --git a/internal/mcpcontract/recovery_action_test.go b/internal/mcpcontract/recovery_action_test.go index a9ff68e..c2a8af0 100644 --- a/internal/mcpcontract/recovery_action_test.go +++ b/internal/mcpcontract/recovery_action_test.go @@ -18,3 +18,10 @@ func TestRecoveryActionOwnsArgumentsForItsDiscriminator(t *testing.T) { t.Fatalf("recovery action = %s (%+v)", encoded, action) } } + +func TestRecoveryActionSupportsManifestReplay(t *testing.T) { + action := RecoveryAction(ExportManifestInput{OpportunityID: "opp-1", WorkspaceID: "ws-1"}) + if action.Type != "export_manifest" || action.ExportManifest == nil || action.ExportManifest.OpportunityID != "opp-1" { + t.Fatalf("manifest recovery action = %+v", action) + } +} diff --git a/internal/mcpcontract/scalable_contracts.go b/internal/mcpcontract/scalable_contracts.go index f4da2e3..e15f126 100644 --- a/internal/mcpcontract/scalable_contracts.go +++ b/internal/mcpcontract/scalable_contracts.go @@ -66,10 +66,11 @@ type ToolCall struct { FindRelatedWork *FindRelatedWorkInput `json:"find_related_work,omitempty"` ListConcerns *ListConcernsInput `json:"list_concerns,omitempty"` ListPortfolio *ListPullRequestPortfolioInput `json:"list_pull_request_portfolio,omitempty"` + ExportManifest *ExportManifestInput `json:"export_manifest,omitempty"` } type recoveryActionInput interface { - GetJobsInput | GetRepositoriesInput | EnsureCoverageInput | SyncRepositoryContextInput | SyncThreadsInput | HydrateThreadsInput | SyncPortfolioInput | SyncPullRequestFeedbackInput | IndexPullRequestFeedbackInput | SyncCIFailuresInput | DeepWikiInput | IndexRepositoriesInput | FindClustersInput | FindNeighborsInput | RankOpportunitiesInput | MineRepositoryFixPatternsInput | PreviewRepositoryFixPatternsInput | SearchGitHubRepositoriesInput | SearchGitHubThreadsInput | SearchCodeInput | ReadSourceFilesInput | InspectCommitChangesInput | CheckMergeConflictsInput | FindRelatedWorkInput | ListConcernsInput | ListPullRequestPortfolioInput + GetJobsInput | GetRepositoriesInput | EnsureCoverageInput | SyncRepositoryContextInput | SyncThreadsInput | HydrateThreadsInput | SyncPortfolioInput | SyncPullRequestFeedbackInput | IndexPullRequestFeedbackInput | SyncCIFailuresInput | DeepWikiInput | IndexRepositoriesInput | FindClustersInput | FindNeighborsInput | RankOpportunitiesInput | MineRepositoryFixPatternsInput | PreviewRepositoryFixPatternsInput | SearchGitHubRepositoriesInput | SearchGitHubThreadsInput | SearchCodeInput | ReadSourceFilesInput | InspectCommitChangesInput | CheckMergeConflictsInput | FindRelatedWorkInput | ListConcernsInput | ListPullRequestPortfolioInput | ExportManifestInput } // RecoveryAction derives the action discriminator from a concrete input type, @@ -128,6 +129,8 @@ func RecoveryAction[T recoveryActionInput](input T) ToolCall { return ToolCall{Type: "list_concerns", ListConcerns: &value} case ListPullRequestPortfolioInput: return ToolCall{Type: "list_pull_request_portfolio", ListPortfolio: &value} + case ExportManifestInput: + return ToolCall{Type: "export_manifest", ExportManifest: &value} default: panic("unreachable recovery action input") } @@ -388,13 +391,14 @@ type HydrateThreadsInput struct { // health refresh. The primitive sync tools remain available in the portfolio // profile for specialized recovery. type SyncPortfolioInput struct { - Selection string `json:"selection" jsonschema:"Selection mode: authored or explicit"` - PullRequests []ThreadRef `json:"pull_requests,omitempty" jsonschema:"One to 100 exact pull requests in explicit mode"` - State string `json:"state,omitempty" jsonschema:"open, closed, or all; defaults to open"` - UpdatedAfter string `json:"updated_after,omitempty" jsonschema:"Optional RFC 3339 lower bound for authored-PR discovery"` - Limit int `json:"limit,omitempty" jsonschema:"Maximum pull requests to discover and refresh from 1 to 100; defaults to 100"` - DiscoveryMaxRequests int `json:"discovery_max_requests,omitempty" jsonschema:"Maximum GitHub requests for identity and authored-PR discovery from 2 to 1000"` - StatusMaxPages int `json:"status_max_pages,omitempty" jsonschema:"Maximum pages per pull-request health facet from 1 to 20; defaults to 3"` + Selection string `json:"selection" jsonschema:"Selection mode: authored or explicit"` + Repository *RepositoryRef `json:"repository,omitempty" jsonschema:"Optional repository scope for authored discovery"` + PullRequests []ThreadRef `json:"pull_requests,omitempty" jsonschema:"One to 100 exact pull requests in explicit mode"` + State string `json:"state,omitempty" jsonschema:"open, closed, or all; defaults to open"` + UpdatedAfter string `json:"updated_after,omitempty" jsonschema:"Optional RFC 3339 lower bound for authored-PR discovery"` + Limit int `json:"limit,omitempty" jsonschema:"Maximum pull requests to discover and refresh from 1 to 100; defaults to 100"` + DiscoveryMaxRequests int `json:"discovery_max_requests,omitempty" jsonschema:"Maximum GitHub requests for identity and authored-PR discovery from 2 to 1000"` + StatusMaxPages int `json:"status_max_pages,omitempty" jsonschema:"Maximum pages per pull-request health facet from 1 to 20; defaults to 3"` } // ContributionPreflightInput describes one prospective contribution before @@ -502,12 +506,13 @@ type SyncCIFailuresInput struct { // ListPullRequestPortfolioInput filters and bounds the stored pull-request portfolio. type ListPullRequestPortfolioInput struct { - Authors []string `json:"authors,omitempty" jsonschema:"Zero or one author login"` - PullRequests []ThreadRef `json:"pull_requests,omitempty" jsonschema:"One to 100 exact pull requests; cannot be combined with authors, state, or limit"` - State string `json:"state,omitempty" jsonschema:"open, closed, or all"` - Limit int `json:"limit,omitempty" jsonschema:"Maximum pull requests from 1 to 100; defaults to 20"` - View string `json:"view,omitempty" jsonschema:"compact or full; defaults to compact"` - SnapshotToken string `json:"snapshot_token,omitempty" jsonschema:"Optional immutable corpus snapshot token from a previous offline read"` + Repository *RepositoryRef `json:"repository,omitempty" jsonschema:"Optional repository scope for authored portfolio reads"` + Authors []string `json:"authors,omitempty" jsonschema:"Zero or one author login"` + PullRequests []ThreadRef `json:"pull_requests,omitempty" jsonschema:"One to 100 exact pull requests; cannot be combined with authors, state, or limit"` + State string `json:"state,omitempty" jsonschema:"open, closed, or all"` + Limit int `json:"limit,omitempty" jsonschema:"Maximum pull requests from 1 to 100; defaults to 20"` + View string `json:"view,omitempty" jsonschema:"compact or full; defaults to compact"` + SnapshotToken string `json:"snapshot_token,omitempty" jsonschema:"Optional immutable corpus snapshot token from a previous offline read"` } // PullRequestPortfolioItem contains source-backed PR facts and a deterministic diff --git a/internal/mcpserver/acquisition_resources_test.go b/internal/mcpserver/acquisition_resources_test.go index d189285..b88846c 100644 --- a/internal/mcpserver/acquisition_resources_test.go +++ b/internal/mcpserver/acquisition_resources_test.go @@ -5,6 +5,7 @@ import ( "strings" "testing" + "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/morluto/gitcontribute/internal/mcpcontract" ) @@ -126,3 +127,61 @@ func TestAcquisitionToolsExposeTheirSideEffectBoundaries(t *testing.T) { } } } + +func TestAcquisitionToolsReturnOnlyResourceLinksAndRequireNestedRepository(t *testing.T) { + reader := &acquisitionCapabilityReader{acquisitionArtifactReader: &acquisitionArtifactReader{fakeReader: &fakeReader{searchStarted: make(chan struct{})}}} + client, closeSessions := connect(t, reader) + defer closeSessions() + + tests := []struct { + tool string + args map[string]any + uri string + }{ + { + tool: mcpcontract.ToolSearchGitHubThreads, + args: map[string]any{"repository": map[string]any{"owner": "acme", "repo": "rocket"}, "query": "cache"}, + uri: "gitcontribute://artifact/github-thread-search/" + testArtifactDigest, + }, + { + tool: mcpcontract.ToolReadSourceFiles, + args: map[string]any{"repository": map[string]any{"owner": "acme", "repo": "rocket"}, "ref": "main", "files": []any{map[string]any{"path": "README.md"}}}, + uri: "gitcontribute://artifact/source-bundle/" + testArtifactDigest, + }, + } + for _, tt := range tests { + t.Run(tt.tool, func(t *testing.T) { + result, err := client.CallTool(context.Background(), &mcp.CallToolParams{Name: tt.tool, Arguments: tt.args}) + if err != nil || result == nil || result.IsError || len(result.Content) != 1 { + t.Fatalf("call %s: result=%+v err=%v", tt.tool, result, err) + } + link, ok := result.Content[0].(*mcp.ResourceLink) + if !ok || link.URI != tt.uri || strings.Contains(strings.ToLower(link.Description), "codex") { + t.Fatalf("host-neutral resource link = %#v", result.Content[0]) + } + resource, err := client.ReadResource(context.Background(), &mcp.ReadResourceParams{URI: link.URI}) + if err != nil || len(resource.Contents) != 1 || resource.Contents[0].Text == "" { + t.Fatalf("read returned URI: resource=%+v err=%v", resource, err) + } + }) + } + + for _, tt := range tests { + legacy := make(map[string]any, len(tt.args)+1) + for key, value := range tt.args { + if key != "repository" { + legacy[key] = value + } + } + legacy["owner"], legacy["repo"] = "acme", "rocket" + result, err := client.CallTool(context.Background(), &mcp.CallToolParams{Name: tt.tool, Arguments: legacy}) + if err == nil && result != nil && !result.IsError { + t.Errorf("legacy flat repository arguments were accepted by %s: %#v", tt.tool, result) + } + } + + result, err := client.CallTool(context.Background(), &mcp.CallToolParams{Name: mcpcontract.ToolSearchGitHubThreads, Arguments: map[string]any{"repository": map[string]any{"owner": "", "repo": "rocket"}, "query": "cache"}}) + if err == nil && result != nil && !result.IsError { + t.Errorf("malformed nested repository was accepted: %#v", result) + } +} diff --git a/internal/mcpserver/actors.go b/internal/mcpserver/actors.go index 814e67f..1dc5245 100644 --- a/internal/mcpserver/actors.go +++ b/internal/mcpserver/actors.go @@ -3,6 +3,7 @@ package mcpserver import ( "context" "errors" + "strings" "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/morluto/gitcontribute/internal/mcpcontract" @@ -164,6 +165,10 @@ func (s *Server) getActorFacets(ctx context.Context, _ *mcp.CallToolRequest, in return nil, out, err } func (s *Server) searchGitHubUsers(ctx context.Context, _ *mcp.CallToolRequest, in mcpcontract.SearchGitHubUsersInput) (*mcp.CallToolResult, mcpcontract.SearchGitHubUsersOutput, error) { + in.Query = strings.TrimSpace(in.Query) + if in.Query == "" { + return nil, mcpcontract.SearchGitHubUsersOutput{}, mcpcontract.InvalidArgument("query", "is required", map[string]any{"query": "octocat"}) + } reader, ok := s.reader.(GitHubActorOperator) if !ok { return nil, mcpcontract.SearchGitHubUsersOutput{}, errors.New("GitHub user search is not available") diff --git a/internal/mcpserver/actors_test.go b/internal/mcpserver/actors_test.go index ff8460c..d6d6539 100644 --- a/internal/mcpserver/actors_test.go +++ b/internal/mcpserver/actors_test.go @@ -66,3 +66,11 @@ func TestActorCapabilitiesAdvertiseAtomicTools(t *testing.T) { } } } + +func TestSearchGitHubUsersRejectsWhitespaceOnlyQuery(t *testing.T) { + server := &Server{reader: actorCapabilityReader{Reader: &fakeReader{}}} + _, _, err := server.searchGitHubUsers(context.Background(), nil, mcpcontract.SearchGitHubUsersInput{Query: " \t "}) + if err == nil { + t.Fatal("whitespace-only GitHub user search query was accepted") + } +} diff --git a/internal/mcpserver/github_acquisition.go b/internal/mcpserver/github_acquisition.go index ad42367..e046619 100644 --- a/internal/mcpserver/github_acquisition.go +++ b/internal/mcpserver/github_acquisition.go @@ -16,7 +16,6 @@ func (s *Server) registerGitHubAcquisitionTools() { description: "Run one bounded live GitHub issue-search page for one repository and persist the returned observations plus an immutable query artifact. The result is not repository-wide thread coverage and cannot prove absence; incomplete pages and additional pages include exact typed retry or pagination actions.", annotations: networkReadAnnotations(), supportedBy: supports[GitHubAcquisitionOperator], input: inputSchema[mcpcontract.SearchGitHubThreadsInput](func(sc *schemaBuilder) { - requireTogether(sc, "owner", "repo") setEnum(sc, "kind", "issue", "pull_request") setEnum(sc, "state", "open", "closed", "all") setEnum(sc, "sort", "comments", "created", "updated", "reactions") @@ -34,7 +33,6 @@ func (s *Server) registerGitHubAcquisitionTools() { description: "Acquire up to 20 ordered repository-relative source files from one explicit commit or named ref, resolving named refs to an authoritative commit. Per-file and total-byte bounds produce item-level outcomes with typed retry or larger-bound actions; content is untrusted text and is available through an immutable local source-bundle artifact.", annotations: networkReadAnnotations(), supportedBy: supports[GitHubAcquisitionOperator], input: inputSchema[mcpcontract.ReadSourceFilesInput](func(sc *schemaBuilder) { - requireTogether(sc, "owner", "repo") setArrayBounds(sc, "files", 1, 20) setRange(sc, "per_file_bytes", 1, 1024*1024) setDefault(sc, "per_file_bytes", 256*1024) @@ -46,10 +44,11 @@ func (s *Server) registerGitHubAcquisitionTools() { } func (s *Server) searchGitHubThreads(ctx context.Context, _ *mcp.CallToolRequest, in mcpcontract.SearchGitHubThreadsInput) (*mcp.CallToolResult, mcpcontract.SearchGitHubThreadsOutput, error) { - if err := validateLiveRepository(in.Owner, in.Repo); err != nil { + if err := validateLiveRepository(in.Repository); err != nil { return nil, mcpcontract.SearchGitHubThreadsOutput{}, err } - if strings.TrimSpace(in.Query) == "" { + in.Query = strings.TrimSpace(in.Query) + if in.Query == "" { return nil, mcpcontract.SearchGitHubThreadsOutput{}, mcpcontract.InvalidArgument("query", "is required", map[string]any{"query": "regression"}) } operator, ok := s.reader.(GitHubAcquisitionOperator) @@ -67,7 +66,7 @@ func (s *Server) searchGitHubThreads(ctx context.Context, _ *mcp.CallToolRequest } func (s *Server) readSourceFiles(ctx context.Context, _ *mcp.CallToolRequest, in mcpcontract.ReadSourceFilesInput) (*mcp.CallToolResult, mcpcontract.ReadSourceFilesOutput, error) { - if err := validateLiveRepository(in.Owner, in.Repo); err != nil { + if err := validateLiveRepository(in.Repository); err != nil { return nil, mcpcontract.ReadSourceFilesOutput{}, err } if strings.TrimSpace(in.Ref) == "" { @@ -90,9 +89,9 @@ func (s *Server) readSourceFiles(ctx context.Context, _ *mcp.CallToolRequest, in return linkedResource(out.ResourceURI, "source-bundle", "GitHub source bundle", "Immutable bounded source text persisted in the local corpus."), out, nil } -func validateLiveRepository(owner, repo string) error { - if strings.TrimSpace(owner) == "" || strings.TrimSpace(repo) == "" { - return mcpcontract.InvalidArgument("owner", "owner and repo are required", map[string]any{"owner": "acme", "repo": "rocket"}) +func validateLiveRepository(repository mcpcontract.RepositoryRef) error { + if strings.TrimSpace(repository.Owner) == "" || strings.TrimSpace(repository.Repo) == "" { + return mcpcontract.InvalidArgument("repository", "owner and repo are required", map[string]any{"owner": "acme", "repo": "rocket"}) } return nil } diff --git a/internal/mcpserver/input_modes.go b/internal/mcpserver/input_modes.go index 2e69ffb..521b94b 100644 --- a/internal/mcpserver/input_modes.go +++ b/internal/mcpserver/input_modes.go @@ -57,7 +57,7 @@ func configureSyncPortfolioModes(builder *schemaBuilder) { explicit := schemaMode("selection", "explicit", []string{"pull_requests"}, - []string{"state", "updated_after", "limit", "discovery_max_requests"}, + []string{"repository", "state", "updated_after", "limit", "discovery_max_requests"}, ) explicit.ID = "urn:gitcontribute:mode:sync-pull-request-portfolio-explicit" explicit.Properties["pull_requests"] = &jsonschema.Schema{MinItems: jsonschema.Ptr(1)} diff --git a/internal/mcpserver/resource_links.go b/internal/mcpserver/resource_links.go index 85c4ab9..5a0f73f 100644 --- a/internal/mcpserver/resource_links.go +++ b/internal/mcpserver/resource_links.go @@ -1,26 +1,16 @@ package mcpserver import ( - "fmt" - "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/morluto/gitcontribute/internal/mcpcontract" ) const resourceReadGuidance = " Pass this exact opaque URI unchanged to the host MCP resource reader; do not shorten, pluralize, or reconstruct it." -func resourceReadInstruction(uri string) *mcp.TextContent { - return &mcp.TextContent{Text: fmt.Sprintf( - "Ask the host to perform MCP `resources/read` with this server and the exact URI %q; in Codex, call `read_mcp_resource`. Treat this URI as opaque: copy it verbatim without shortening, pluralizing, or reconstructing it. Do not substitute structured tool output for the resource read.", - uri, - )} -} - // linkedResource keeps structured tool output under the SDK's generic AddTool // path while giving MCP clients a native, followable durable-object link. func linkedResource(uri, name, title, description string) *mcp.CallToolResult { return &mcp.CallToolResult{Content: []mcp.Content{ - resourceReadInstruction(uri), &mcp.ResourceLink{ URI: uri, Name: name, Title: title, Description: description + resourceReadGuidance, MIMEType: "application/json", }, @@ -37,9 +27,7 @@ func linkedJobResources(out mcpcontract.GetJobsOutput) *mcp.CallToolResult { if artifact.URI == "" { continue } - content = append( - content, - resourceReadInstruction(artifact.URI), + content = append(content, &mcp.ResourceLink{ URI: artifact.URI, Name: artifact.Kind, Title: "Job artifact", Description: "Durable typed artifact produced by " + item.Value.Kind + "." + resourceReadGuidance, diff --git a/internal/mcpserver/resources.go b/internal/mcpserver/resources.go index 535794b..3ae9dd3 100644 --- a/internal/mcpserver/resources.go +++ b/internal/mcpserver/resources.go @@ -49,9 +49,14 @@ func (s *Server) readResource(ctx context.Context, req *mcp.ReadResourceRequest) if err != nil { return nil, mcp.ResourceNotFoundError(uri) } + escapedPath := u.EscapedPath() + parts, valid := resourcePathParts(escapedPath) + if !valid || u.User != nil || u.RawQuery != "" || u.Fragment != "" || strings.HasSuffix(escapedPath, "/") || strings.Contains(strings.TrimPrefix(escapedPath, "/"), "//") { + return nil, mcp.ResourceNotFoundError(uri) + } value, err := s.readResourceValue(ctx, resourceRequest{ uri: uri, scheme: u.Scheme, host: u.Host, - parts: strings.Split(strings.Trim(u.Path, "/"), "/"), + parts: parts, }) if isNotFound(err) { return nil, mcp.ResourceNotFoundError(uri) @@ -68,6 +73,26 @@ func (s *Server) readResource(ctx context.Context, req *mcp.ReadResourceRequest) }}}, nil } +// resourcePathParts preserves escaped path separators inside opaque resource +// IDs. url.URL.Path is already decoded, so splitting it would turn one +// percent-escaped ID into multiple routing segments. +func resourcePathParts(escapedPath string) ([]string, bool) { + escapedPath = strings.TrimPrefix(escapedPath, "/") + if escapedPath == "" { + return nil, false + } + rawParts := strings.Split(escapedPath, "/") + parts := make([]string, len(rawParts)) + for i, rawPart := range rawParts { + part, err := url.PathUnescape(rawPart) + if err != nil { + return nil, false + } + parts[i] = part + } + return parts, true +} + type resourceRequest struct { uri string scheme string @@ -147,14 +172,11 @@ func (s *Server) readActorResource(ctx context.Context, req resourceRequest) (an if !ok || (len(req.parts) != 1 && (len(req.parts) != 3 || req.parts[1] != "facet")) || strings.TrimSpace(req.parts[0]) == "" { return nil, mcp.ResourceNotFoundError(req.uri) } - actorID, err := url.PathUnescape(req.parts[0]) - if err != nil { - return nil, mcp.ResourceNotFoundError(req.uri) - } + actorID := req.parts[0] facet := "" if len(req.parts) == 3 { - facet, err = url.PathUnescape(req.parts[2]) - if err != nil || strings.TrimSpace(facet) == "" { + facet = req.parts[2] + if strings.TrimSpace(facet) == "" { return nil, mcp.ResourceNotFoundError(req.uri) } } @@ -221,14 +243,8 @@ func (s *Server) readPullRequestFeedbackResource(ctx context.Context, req resour if len(req.parts) != 5 || strings.TrimSpace(req.parts[3]) == "" || strings.TrimSpace(req.parts[4]) == "" { return nil, mcp.ResourceNotFoundError(req.uri) } - channel, err := url.PathUnescape(req.parts[3]) - if err != nil { - return nil, mcp.ResourceNotFoundError(req.uri) - } - feedbackID, err := url.PathUnescape(req.parts[4]) - if err != nil { - return nil, mcp.ResourceNotFoundError(req.uri) - } + channel := req.parts[3] + feedbackID := req.parts[4] return reader.PullRequestFeedbackItemResource(ctx, req.parts[0], req.parts[1], number, channel, feedbackID) } diff --git a/internal/mcpserver/scalable.go b/internal/mcpserver/scalable.go index 9cf6902..7253fb6 100644 --- a/internal/mcpserver/scalable.go +++ b/internal/mcpserver/scalable.go @@ -276,7 +276,7 @@ func (s *Server) registerScalable() { }), output: outputSchema[mcpcontract.FixPatternReport]("Bounded offline fix-pattern analysis; never persisted."), handler: s.previewRepositoryFixPatterns, }) - addCatalogTool(s, catalogTool[mcpcontract.SyncPortfolioInput, mcpcontract.JobReference]{name: mcpcontract.ToolSyncPortfolio, title: "Synchronize a pull-request portfolio", description: "selection is required: use authored only for the authenticated user's PR portfolio, or explicit with 1-100 exact pull_requests. This tool is not repository-wide comment discovery; for all feedback by a reviewer use github.index_pull_request_feedback, jobs.get, and corpus.search_pull_request_feedback with feedback_author. Refreshes PR details, merge state, checks, review state, unresolved threads, merge queue, files, and closing issues in one durable job; incomplete discovery is surfaced with a typed retry action.", annotations: networkReadAnnotations(), supportedBy: supports[GitHubOperator], input: inputSchema[mcpcontract.SyncPortfolioInput](func(sc *schemaBuilder) { + addCatalogTool(s, catalogTool[mcpcontract.SyncPortfolioInput, mcpcontract.JobReference]{name: mcpcontract.ToolSyncPortfolio, title: "Synchronize a pull-request portfolio", description: "selection is required: use authored only for the authenticated user's PR portfolio, optionally scoped to one repository, or explicit with 1-100 exact pull_requests. This tool is not repository-wide comment discovery; for all feedback by a reviewer use github.index_pull_request_feedback, jobs.get, and corpus.search_pull_request_feedback with feedback_author. Refreshes PR details, merge state, checks, review state, unresolved threads, merge queue, files, and closing issues in one durable job; incomplete discovery is surfaced with a typed retry action.", annotations: networkReadAnnotations(), supportedBy: supports[GitHubOperator], input: inputSchema[mcpcontract.SyncPortfolioInput](func(sc *schemaBuilder) { setEnum(sc, "selection", "authored", "explicit") setArrayBounds(sc, "pull_requests", 1, 100) constrainPullRequestRefs(sc, "pull_requests") @@ -328,7 +328,7 @@ func (s *Server) registerScalable() { setRange(sc, "max_log_bytes_per_job", 1024, 1048576) setRange(sc, "max_requests", 1, 1000) }), output: outputSchema[mcpcontract.JobReference]("Reference to a bounded CI diagnostics job."), handler: s.syncCIFailures}) - addCatalogTool(s, catalogTool[mcpcontract.ListPullRequestPortfolioInput, mcpcontract.ListPullRequestPortfolioOutput]{name: mcpcontract.ToolListPullRequestPortfolio, title: "List pull requests that need contributor attention", description: "List stored authored pull requests with deterministic attention from lifecycle, checks, review conversations, merge state, queue, and freshness. This offline read reports incomplete facets as unknown; each incomplete item includes an exact typed sync_portfolio recovery action, and truncated pages include a typed next-page action.", annotations: readOnly, supportedBy: supports[PortfolioReader], input: inputSchema[mcpcontract.ListPullRequestPortfolioInput](func(sc *schemaBuilder) { + addCatalogTool(s, catalogTool[mcpcontract.ListPullRequestPortfolioInput, mcpcontract.ListPullRequestPortfolioOutput]{name: mcpcontract.ToolListPullRequestPortfolio, title: "List pull requests that need contributor attention", description: "List stored authored pull requests, optionally scoped to one repository, with deterministic attention from lifecycle, checks, review conversations, merge state, queue, and freshness. This offline read reports incomplete facets as unknown; each incomplete item includes an exact typed sync_portfolio recovery action, and truncated pages include a typed next-page action.", annotations: readOnly, supportedBy: supports[PortfolioReader], input: inputSchema[mcpcontract.ListPullRequestPortfolioInput](func(sc *schemaBuilder) { setArrayBounds(sc, "authors", 0, 1) setEnum(sc, "state", "open", "closed", "all") setRange(sc, "limit", 1, 100) @@ -647,6 +647,14 @@ func (s *Server) previewRepositoryFixPatterns(ctx context.Context, _ *mcp.CallTo return nil, out, err } func (s *Server) syncPortfolio(ctx context.Context, _ *mcp.CallToolRequest, in mcpcontract.SyncPortfolioInput) (*mcp.CallToolResult, mcpcontract.JobReference, error) { + if in.Repository != nil { + if err := validateLiveRepository(*in.Repository); err != nil { + return nil, mcpcontract.JobReference{}, err + } + if in.Selection == "explicit" { + return nil, mcpcontract.JobReference{}, mcpcontract.InvalidArgument("repository", "is only valid for authored selection", nil) + } + } if in.Selection == "authored" && in.State == "" { in.State = "open" } diff --git a/internal/mcpserver/server.go b/internal/mcpserver/server.go index a1290e7..272a242 100644 --- a/internal/mcpserver/server.go +++ b/internal/mcpserver/server.go @@ -466,6 +466,7 @@ func (s *Server) register() { func boolPtr(v bool) *bool { return &v } func (s *Server) searchCode(ctx context.Context, _ *mcp.CallToolRequest, in mcpcontract.SearchCodeInput) (*mcp.CallToolResult, mcpcontract.SearchCodeOutput, error) { + in.Query = strings.TrimSpace(in.Query) if in.Query == "" { return nil, mcpcontract.SearchCodeOutput{}, mcpcontract.InvalidArgument("query", "is required", map[string]any{"query": "MIDI"}) } diff --git a/internal/mcpserver/server_contract_test.go b/internal/mcpserver/server_contract_test.go index 903defc..016dbe6 100644 --- a/internal/mcpserver/server_contract_test.go +++ b/internal/mcpserver/server_contract_test.go @@ -182,27 +182,12 @@ func TestDurableToolResultsIncludeSDKResourceLinks(t *testing.T) { if result.StructuredContent == nil { t.Fatal("resource-linked tool lost SDK-populated structured content") } - if len(result.Content) != 2 { + if len(result.Content) != 1 { t.Fatalf("resource-linked content = %+v", result.Content) } - instruction, ok := result.Content[0].(*mcp.TextContent) - if !ok { - t.Fatalf("resource instruction = %#v", result.Content[0]) - } - for _, phrase := range []string{ - "perform MCP `resources/read` with this server", - "in Codex, call `read_mcp_resource`", - `exact URI "gitcontribute://investigation/inv-1"`, - "copy it verbatim without shortening, pluralizing, or reconstructing it", - "Do not substitute structured tool output for the resource read", - } { - if !strings.Contains(instruction.Text, phrase) { - t.Errorf("resource instruction missing %q: %q", phrase, instruction.Text) - } - } - link, ok := result.Content[1].(*mcp.ResourceLink) + link, ok := result.Content[0].(*mcp.ResourceLink) if !ok || link.URI != "gitcontribute://investigation/inv-1" || link.MIMEType != "application/json" { - t.Fatalf("resource link = %#v", result.Content[1]) + t.Fatalf("resource link = %#v", result.Content[0]) } for _, phrase := range []string{"exact opaque URI unchanged", "do not shorten, pluralize, or reconstruct it"} { if !strings.Contains(link.Description, phrase) { @@ -211,6 +196,22 @@ func TestDurableToolResultsIncludeSDKResourceLinks(t *testing.T) { } } +func TestJobArtifactResultsContainOnlyResourceLinks(t *testing.T) { + result := linkedJobResources(mcpcontract.GetJobsOutput{Items: []mcpcontract.BatchItem[mcpcontract.GetJobOutput]{{ + Key: "job-1", + Value: &mcpcontract.GetJobOutput{Kind: "sync_portfolio", Artifacts: []mcpcontract.JobArtifactReference{{ + Kind: "portfolio", URI: "gitcontribute://artifact/github-thread-search/test", + }}}, + }}}) + if result == nil || len(result.Content) != 1 { + t.Fatalf("job artifact content = %+v", result) + } + link, ok := result.Content[0].(*mcp.ResourceLink) + if !ok || strings.Contains(strings.ToLower(link.Description), "codex") || !strings.Contains(link.Description, "exact opaque URI unchanged") { + t.Fatalf("job artifact link = %#v", result.Content[0]) + } +} + func TestDurableProducerReferencesRoundTripThroughResources(t *testing.T) { client, closeSessions := connect(t, &fakeReader{searchStarted: make(chan struct{})}) defer closeSessions() @@ -268,12 +269,12 @@ func TestDurableProducerReferencesRoundTripThroughResources(t *testing.T) { if ref.URI != tt.uri || ref.Kind != tt.kind || ref.ID == "" { t.Fatalf("reference = %+v, want kind=%q uri=%q", ref, tt.kind, tt.uri) } - if len(result.Content) != 2 { + if len(result.Content) != 1 { t.Fatalf("content = %+v", result.Content) } - link, ok := result.Content[1].(*mcp.ResourceLink) + link, ok := result.Content[0].(*mcp.ResourceLink) if !ok || link.URI != tt.uri { - t.Fatalf("resource link = %#v", result.Content[1]) + t.Fatalf("resource link = %#v", result.Content[0]) } resource, err := client.ReadResource(context.Background(), &mcp.ReadResourceParams{URI: tt.uri}) if err != nil { diff --git a/internal/mcpserver/server_input_resources_test.go b/internal/mcpserver/server_input_resources_test.go new file mode 100644 index 0000000..178c562 --- /dev/null +++ b/internal/mcpserver/server_input_resources_test.go @@ -0,0 +1,129 @@ +package mcpserver + +import ( + "context" + "errors" + "slices" + "testing" + + "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/morluto/gitcontribute/internal/mcpcontract" +) + +func TestRepositoryResourceAndNotFound(t *testing.T) { + client, closeSessions := connect(t, &fakeReader{searchStarted: make(chan struct{})}) + defer closeSessions() + + result, err := client.ReadResource(context.Background(), &mcp.ReadResourceParams{ + URI: "gitcontribute://repository/acme/rocket", + }) + if err != nil { + t.Fatalf("read repository: %v", err) + } + if len(result.Contents) != 1 || result.Contents[0].Text == "" { + t.Fatalf("resource result = %+v", result) + } + + _, err = client.ReadResource(context.Background(), &mcp.ReadResourceParams{ + URI: "gitcontribute://thread/acme/rocket/issue/404", + }) + if err == nil { + t.Fatal("expected resource-not-found error") + } + + for _, uri := range []string{ + "gitcontribute://repository/acme/rocket?view=full", + "gitcontribute://repository/acme/rocket#fragment", + "gitcontribute://user@repository/acme/rocket", + "gitcontribute://repository/acme/rocket/", + "gitcontribute://repository/acme//rocket", + } { + if _, err := client.ReadResource(context.Background(), &mcp.ReadResourceParams{URI: uri}); err == nil { + t.Fatalf("non-canonical resource URI %q was accepted", uri) + } + } +} + +type feedbackResourceCapture struct { + *fakeReader + channel string + feedbackID string +} + +func (*feedbackResourceCapture) PullRequestFeedbackResource(context.Context, string, string, int) (map[string]any, error) { + return nil, errors.New("unexpected pull-request feedback resource") +} + +func (r *feedbackResourceCapture) PullRequestFeedbackItemResource(_ context.Context, _ string, _ string, _ int, channel, feedbackID string) (map[string]any, error) { + r.channel = channel + r.feedbackID = feedbackID + return map[string]any{"schema_version": "gitcontribute.pull-request-feedback-item.v1"}, nil +} + +func (*feedbackResourceCapture) CIFailureResource(context.Context, string, string, int) (map[string]any, error) { + return nil, errors.New("unexpected CI failure resource") +} + +func (*feedbackResourceCapture) CIJobLogResource(context.Context, string, string, int, int64) (map[string]any, error) { + return nil, errors.New("unexpected CI job log resource") +} + +func TestResourcePathPartsPreservesEscapedOpaqueIDs(t *testing.T) { + parts, ok := resourcePathParts("/acme/rocket/7/inline_comments/comment%2Fwith%20space") + if !ok { + t.Fatal("resourcePathParts rejected a valid escaped path") + } + want := []string{"acme", "rocket", "7", "inline_comments", "comment/with space"} + if !slices.Equal(parts, want) { + t.Fatalf("resource path parts = %q, want %q", parts, want) + } +} + +func TestResourcePathPartsRejectsMalformedEscapes(t *testing.T) { + if _, ok := resourcePathParts("/acme/rocket/%zz"); ok { + t.Fatal("resourcePathParts accepted a malformed escape") + } + server := &Server{reader: &fakeReader{}} + if _, err := server.readResource(context.Background(), &mcp.ReadResourceRequest{Params: &mcp.ReadResourceParams{URI: "gitcontribute://repository/acme/%zz"}}); err == nil { + t.Fatal("malformed resource URI was accepted") + } +} + +func TestReadResourceRoutesEscapedFeedbackIDAsOneOpaqueSegment(t *testing.T) { + reader := &feedbackResourceCapture{fakeReader: &fakeReader{}} + server := &Server{reader: reader} + result, err := server.readResource(context.Background(), &mcp.ReadResourceRequest{Params: &mcp.ReadResourceParams{URI: "gitcontribute://pull-request-feedback/acme/rocket/7/inline_comments/comment%2Fwith%20space"}}) + if err != nil { + t.Fatalf("read escaped feedback resource: %v", err) + } + if len(result.Contents) != 1 || reader.channel != "inline_comments" || reader.feedbackID != "comment/with space" { + t.Fatalf("escaped feedback resource routed as channel=%q feedback_id=%q result=%+v", reader.channel, reader.feedbackID, result) + } +} + +func TestSearchCodeRejectsWhitespaceOnlyQuery(t *testing.T) { + server := &Server{reader: &fakeReader{}} + _, _, err := server.searchCode(context.Background(), nil, mcpcontract.SearchCodeInput{Query: " \t "}) + if err == nil { + t.Fatal("whitespace-only code search query was accepted") + } +} + +func TestSearchThreadsRejectsWhitespaceOnlyQuery(t *testing.T) { + server := &Server{reader: &fakeReader{}} + _, _, err := server.searchThreads(context.Background(), nil, SearchThreadsInput{Query: " \t "}) + if err == nil { + t.Fatal("whitespace-only thread search query was accepted") + } +} + +func TestSearchRepositoriesNormalizesOptionalQuery(t *testing.T) { + server := &Server{reader: &fakeReader{}} + _, out, err := server.searchRepositories(context.Background(), nil, mcpcontract.SearchRepositoriesInput{Query: " \t "}) + if err != nil { + t.Fatalf("search repositories: %v", err) + } + if out.Query != "" { + t.Fatalf("repository query was not normalized: %q", out.Query) + } +} diff --git a/internal/mcpserver/server_test.go b/internal/mcpserver/server_test.go index 066f0c1..5805b0a 100644 --- a/internal/mcpserver/server_test.go +++ b/internal/mcpserver/server_test.go @@ -515,28 +515,6 @@ func TestReadOnlyToolsReturnStructuredOutput(t *testing.T) { } } -func TestRepositoryResourceAndNotFound(t *testing.T) { - client, closeSessions := connect(t, &fakeReader{searchStarted: make(chan struct{})}) - defer closeSessions() - - result, err := client.ReadResource(context.Background(), &mcp.ReadResourceParams{ - URI: "gitcontribute://repository/acme/rocket", - }) - if err != nil { - t.Fatalf("read repository: %v", err) - } - if len(result.Contents) != 1 || result.Contents[0].Text == "" { - t.Fatalf("resource result = %+v", result) - } - - _, err = client.ReadResource(context.Background(), &mcp.ReadResourceParams{ - URI: "gitcontribute://thread/acme/rocket/issue/404", - }) - if err == nil { - t.Fatal("expected resource-not-found error") - } -} - func TestInvestigationOpportunityEvidenceResources(t *testing.T) { client, closeSessions := connect(t, &fakeReader{searchStarted: make(chan struct{})}) defer closeSessions() diff --git a/internal/mcpserver/v1.go b/internal/mcpserver/v1.go index 1397507..2c4e92d 100644 --- a/internal/mcpserver/v1.go +++ b/internal/mcpserver/v1.go @@ -248,6 +248,7 @@ func (s *Server) registerV1() { } func (s *Server) searchRepositories(ctx context.Context, _ *mcp.CallToolRequest, in mcpcontract.SearchRepositoriesInput) (*mcp.CallToolResult, mcpcontract.SearchRepositoriesOutput, error) { + in.Query = strings.TrimSpace(in.Query) if in.Limit == 0 { in.Limit = 20 } @@ -265,6 +266,7 @@ func (s *Server) searchRepositories(ctx context.Context, _ *mcp.CallToolRequest, } func (s *Server) searchThreads(ctx context.Context, _ *mcp.CallToolRequest, in SearchThreadsInput) (*mcp.CallToolResult, mcpcontract.SearchOutput, error) { + in.Query = strings.TrimSpace(in.Query) if in.Query == "" { return nil, mcpcontract.SearchOutput{}, mcpcontract.InvalidArgument("query", "is required", map[string]any{"query": "music"}) } diff --git a/internal/terminalinstall/npm.go b/internal/terminalinstall/npm.go index d12ed5f..e04a684 100644 --- a/internal/terminalinstall/npm.go +++ b/internal/terminalinstall/npm.go @@ -4,28 +4,36 @@ package terminalinstall import ( "context" + "errors" "fmt" "os" "os/exec" "path/filepath" + "regexp" "runtime" "strings" + + "github.com/morluto/gitcontribute/internal/redaction" ) -// GlobalNPM installs packageSpec into npm's global prefix and returns the -// verified command path. The caller must obtain explicit user authorization -// before calling it: this function executes npm and may access the registry and -// mutate files outside GitContribute's application directories. +// GlobalNPM installs the specified GitContribute release into npm's global +// prefix and returns the verified command path. The caller must obtain explicit +// user authorization before calling it: this function executes npm and may +// access the registry and mutate files outside GitContribute's application +// directories. // // A successful npm exit is not sufficient. GlobalNPM resolves npm's actual // prefix and verifies the platform-specific command shim so MCP registration // never records an assumed or missing path. It does not modify shell startup // files or the parent process's PATH. func GlobalNPM(ctx context.Context, packageSpec string) (string, error) { + if err := validatePackageSpec(packageSpec); err != nil { + return "", err + } // CommandContext passes arguments directly to npm without a shell. The - // application caller supplies an exact package name plus a validated release - // version, so packageSpec cannot introduce npm flags or another package. - // #nosec G204 -- no shell is involved and packageSpec is validated upstream. + // package specification is constrained here rather than trusting every + // caller to prevent npm flags or another package from being installed. + // #nosec G204 -- no shell is involved and packageSpec is validated above. output, err := exec.CommandContext(ctx, "npm", "install", "--global", packageSpec).CombinedOutput() if err != nil { return "", commandFailure("install persistent CLI", output, err) @@ -45,8 +53,17 @@ func GlobalNPM(ctx context.Context, packageSpec string) (string, error) { return commandPath, nil } +var gitContributePackageSpec = regexp.MustCompile(`^gitcontribute@(?:latest|[0-9]+\.[0-9]+\.[0-9]+(?:-[0-9A-Za-z.-]+)?(?:\+[0-9A-Za-z.-]+)?)$`) + +func validatePackageSpec(packageSpec string) error { + if !gitContributePackageSpec.MatchString(packageSpec) { + return errors.New("package specification must select a GitContribute release") + } + return nil +} + func commandFailure(action string, output []byte, err error) error { - detail := strings.TrimSpace(string(output)) + detail := redaction.String(strings.TrimSpace(string(output))) if detail == "" { return fmt.Errorf("%s: %w", action, err) } diff --git a/internal/terminalinstall/npm_test.go b/internal/terminalinstall/npm_test.go index ffdf884..aaaf2ba 100644 --- a/internal/terminalinstall/npm_test.go +++ b/internal/terminalinstall/npm_test.go @@ -6,6 +6,34 @@ import ( "testing" ) +func TestGitContributePackageSpec(t *testing.T) { + valid := []string{ + "gitcontribute@latest", + "gitcontribute@1.2.3", + "gitcontribute@1.2.3-rc.1", + "gitcontribute@1.2.3+build.4", + } + for _, packageSpec := range valid { + if err := validatePackageSpec(packageSpec); err != nil { + t.Errorf("valid package spec rejected: %q: %v", packageSpec, err) + } + } + + invalid := []string{ + "", + "gitcontribute", + "gitcontribute@v1.2.3", + "gitcontribute@1.2", + "gitcontribute@--ignore-scripts", + "other-package@1.2.3", + } + for _, packageSpec := range invalid { + if err := validatePackageSpec(packageSpec); err == nil { + t.Errorf("invalid package spec accepted: %q", packageSpec) + } + } +} + func TestCommandFailureIncludesOutputWithoutDroppingCause(t *testing.T) { cause := errors.New("exit status 1") err := commandFailure("install persistent CLI", []byte("permission denied\n"), cause) @@ -20,3 +48,11 @@ func TestCommandFailureOmitsEmptyOutput(t *testing.T) { t.Fatalf("command failure = %q", err) } } + +func TestCommandFailureRedactsCredentialLikeOutput(t *testing.T) { + secret := "github_pat_" + strings.Repeat("a", 22) + err := commandFailure("install persistent CLI", []byte("npm ERR! token="+secret), errors.New("failed")) + if strings.Contains(err.Error(), secret) || !strings.Contains(err.Error(), "[REDACTED]") { + t.Fatalf("command failure exposed credential-like output: %q", err) + } +} diff --git a/internal/workspace/exec_runner_test.go b/internal/workspace/exec_runner_test.go new file mode 100644 index 0000000..63cbc16 --- /dev/null +++ b/internal/workspace/exec_runner_test.go @@ -0,0 +1,19 @@ +package workspace + +import ( + "context" + "runtime" + "strings" + "testing" +) + +func TestExecRunnerRedactsCredentialLikeStderr(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("test uses a POSIX shell to produce controlled stderr") + } + secret := "github_pat_" + strings.Repeat("a", 22) + _, err := (execRunner{}).Run(context.Background(), "sh", "-c", "printf '%s\\n' \"token=$1\" >&2; exit 1", "sh", secret) + if err == nil || strings.Contains(err.Error(), secret) || !strings.Contains(err.Error(), "[REDACTED]") { + t.Fatalf("runner error exposed credential-like stderr: %v", err) + } +} diff --git a/internal/workspace/workspace.go b/internal/workspace/workspace.go index d9ceeaf..ea2979c 100644 --- a/internal/workspace/workspace.go +++ b/internal/workspace/workspace.go @@ -15,6 +15,7 @@ import ( "github.com/morluto/gitcontribute/internal/buflimit" "github.com/morluto/gitcontribute/internal/gitremote" + "github.com/morluto/gitcontribute/internal/redaction" ) var ( @@ -67,7 +68,7 @@ func (execRunner) Run(ctx context.Context, name string, args ...string) (string, return stdout.String(), buflimit.ErrOutputLimit } if err != nil { - return "", fmt.Errorf("exec %s: %w (stderr: %s)", name, err, strings.TrimSpace(stderr.String())) + return "", fmt.Errorf("exec %s: %w (stderr: %s)", name, err, redaction.String(strings.TrimSpace(stderr.String()))) } return stdout.String(), nil } @@ -237,7 +238,7 @@ func (m *Manager) Clone(ctx context.Context, remote, name string) error { return ErrMirrorExists } mirrorsDir := filepath.Join(m.root, "mirrors") - if err := os.MkdirAll(mirrorsDir, 0755); err != nil { + if err := os.MkdirAll(mirrorsDir, 0750); err != nil { return fmt.Errorf("create mirrors dir: %w", err) } path := filepath.Join(mirrorsDir, name) @@ -349,14 +350,14 @@ func (m *Manager) Create(ctx context.Context, mirrorName, baseRef, candidateRef, mergeBase = strings.TrimSpace(mergeBase) workDir := filepath.Join(m.root, "workspaces") - if err := os.MkdirAll(workDir, 0755); err != nil { + if err := os.MkdirAll(workDir, 0750); err != nil { return nil, fmt.Errorf("create workspaces dir: %w", err) } path := filepath.Join(workDir, name) // Atomically reserve the final path before asking Git to populate it. This // both serializes concurrent creators and proves that any later cleanup is // limited to a directory created by this invocation. - if err := os.Mkdir(path, 0755); err != nil { + if err := os.Mkdir(path, 0750); err != nil { if errors.Is(err, os.ErrExist) { return nil, ErrExists } @@ -365,9 +366,14 @@ func (m *Manager) Create(ctx context.Context, mirrorName, baseRef, candidateRef, if _, err := m.git(ctx, mi.path, "worktree", "add", "--detach", path, candidateSHA); err != nil { // git worktree add may create a partial directory before - // failing. Clean it up so it does not leak on disk. - _ = os.RemoveAll(path) - return nil, fmt.Errorf("create worktree: %w", err) + // failing. Preserve a cleanup failure so callers know the reserved name + // may still need manual recovery instead of seeing a misleadingly simple + // Git error. + cleanupErr := os.RemoveAll(path) + if cleanupErr != nil { + cleanupErr = fmt.Errorf("remove reserved workspace path: %w", cleanupErr) + } + return nil, errors.Join(fmt.Errorf("create worktree: %w", err), cleanupErr) } st, err := m.status(ctx, path) diff --git a/internal/workspace/workspace_manager_test.go b/internal/workspace/workspace_manager_test.go new file mode 100644 index 0000000..9522c15 --- /dev/null +++ b/internal/workspace/workspace_manager_test.go @@ -0,0 +1,82 @@ +package workspace + +import ( + "context" + "os" + "path/filepath" + "runtime" + "strings" + "testing" +) + +func TestManager_CreateAndInspect(t *testing.T) { + t.Parallel() + ctx := context.Background() + remote, baseSHA, candidateSHA := setupRemote(t) + mgr := newManager(t) + + if err := mgr.Clone(ctx, remote, "origin"); err != nil { + t.Fatal(err) + } + + ws, err := mgr.Create(ctx, "origin", "master", "feature", "ws1") + if err != nil { + t.Fatal(err) + } + + if ws.Remote != remote { + t.Errorf("Remote = %q, want %q", ws.Remote, remote) + } + if ws.BaseSHA != baseSHA { + t.Errorf("BaseSHA = %q, want %q", ws.BaseSHA, baseSHA) + } + if ws.CandidateSHA != candidateSHA { + t.Errorf("CandidateSHA = %q, want %q", ws.CandidateSHA, candidateSHA) + } + if ws.MergeBase != baseSHA { + t.Errorf("MergeBase = %q, want %q", ws.MergeBase, baseSHA) + } + + if _, err := os.Stat(ws.Path); err != nil { + t.Errorf("workspace path does not exist: %v", err) + } + if runtime.GOOS != "windows" { + for _, path := range []string{ + filepath.Join(mgr.root, "mirrors"), + filepath.Join(mgr.root, "workspaces"), + ws.Path, + } { + info, err := os.Stat(path) + if err != nil { + t.Fatalf("stat managed path %q: %v", path, err) + } + if info.Mode().Perm()&0o027 != 0 { + t.Errorf("managed path %q permissions = %04o, want no group write or world access", path, info.Mode().Perm()) + } + } + } + + mergeBase, err := mgr.MergeBase(ctx, "ws1") + if err != nil { + t.Fatal(err) + } + if mergeBase != baseSHA { + t.Fatalf("MergeBase() = %q, want %q", mergeBase, baseSHA) + } + + diff, err := mgr.Diff(ctx, "ws1") + if err != nil { + t.Fatal(err) + } + if !strings.Contains(diff, "feature.txt") { + t.Fatalf("diff does not contain feature.txt:\n%s", diff) + } + + got, ok := mgr.Get("ws1") + if !ok || got.Name != "ws1" { + t.Fatalf("Get(ws1) = (%v, %v)", got, ok) + } + if len(mgr.List()) != 1 { + t.Fatalf("List() = %d items, want 1", len(mgr.List())) + } +} diff --git a/internal/workspace/workspace_test.go b/internal/workspace/workspace_test.go index 8a6382c..0cad09d 100644 --- a/internal/workspace/workspace_test.go +++ b/internal/workspace/workspace_test.go @@ -6,6 +6,7 @@ import ( "os" "os/exec" "path/filepath" + "runtime" "strings" "sync" "testing" @@ -140,63 +141,6 @@ func TestManager_CloneAndResolve(t *testing.T) { }) } -func TestManager_CreateAndInspect(t *testing.T) { - t.Parallel() - ctx := context.Background() - remote, baseSHA, candidateSHA := setupRemote(t) - mgr := newManager(t) - - if err := mgr.Clone(ctx, remote, "origin"); err != nil { - t.Fatal(err) - } - - ws, err := mgr.Create(ctx, "origin", "master", "feature", "ws1") - if err != nil { - t.Fatal(err) - } - - if ws.Remote != remote { - t.Errorf("Remote = %q, want %q", ws.Remote, remote) - } - if ws.BaseSHA != baseSHA { - t.Errorf("BaseSHA = %q, want %q", ws.BaseSHA, baseSHA) - } - if ws.CandidateSHA != candidateSHA { - t.Errorf("CandidateSHA = %q, want %q", ws.CandidateSHA, candidateSHA) - } - if ws.MergeBase != baseSHA { - t.Errorf("MergeBase = %q, want %q", ws.MergeBase, baseSHA) - } - - if _, err := os.Stat(ws.Path); err != nil { - t.Errorf("workspace path does not exist: %v", err) - } - - mergeBase, err := mgr.MergeBase(ctx, "ws1") - if err != nil { - t.Fatal(err) - } - if mergeBase != baseSHA { - t.Fatalf("MergeBase() = %q, want %q", mergeBase, baseSHA) - } - - diff, err := mgr.Diff(ctx, "ws1") - if err != nil { - t.Fatal(err) - } - if !strings.Contains(diff, "feature.txt") { - t.Fatalf("diff does not contain feature.txt:\n%s", diff) - } - - got, ok := mgr.Get("ws1") - if !ok || got.Name != "ws1" { - t.Fatalf("Get(ws1) = (%v, %v)", got, ok) - } - if len(mgr.List()) != 1 { - t.Fatalf("List() = %d items, want 1", len(mgr.List())) - } -} - func TestWorkspaceSnapshotBindsStagedUnstagedAndUntrackedContent(t *testing.T) { ctx := context.Background() remote, baseSHA, _ := setupRemote(t) @@ -524,6 +468,58 @@ func TestManager_ConcurrentCreateDoesNotRemoveWinner(t *testing.T) { } } +type failingWorktreeReservationCleanupRunner struct { + workspacesDir string + err error +} + +func (r failingWorktreeReservationCleanupRunner) Run(ctx context.Context, name string, args ...string) (string, error) { + for i := range args { + if args[i] == "worktree" && i+1 < len(args) && args[i+1] == "add" { + if err := os.Chmod(r.workspacesDir, 0500); err != nil { + return "", err + } + return "", r.err + } + } + return execRunner{}.Run(ctx, name, args...) +} + +func TestManagerCreateReportsFailedReservationCleanup(t *testing.T) { + t.Parallel() + if runtime.GOOS == "windows" { + t.Skip("test requires POSIX directory permission semantics") + } + ctx := context.Background() + remote, _, _ := setupRemote(t) + root := t.TempDir() + runnerErr := errors.New("worktree add failed") + mgr, err := NewManager(root, failingWorktreeReservationCleanupRunner{ + workspacesDir: filepath.Join(root, "workspaces"), + err: runnerErr, + }) + if err != nil { + t.Fatal(err) + } + if err := mgr.Clone(ctx, remote, "origin"); err != nil { + t.Fatal(err) + } + + _, err = mgr.Create(ctx, "origin", "master", "feature", "reserved") + if chmodErr := os.Chmod(filepath.Join(root, "workspaces"), 0755); chmodErr != nil { + t.Fatal(chmodErr) + } + if !errors.Is(err, runnerErr) { + t.Fatalf("Create error = %v, want worktree failure", err) + } + if err == nil || !strings.Contains(err.Error(), "remove reserved workspace path") { + t.Fatalf("Create error omitted reservation cleanup failure: %v", err) + } + if _, statErr := os.Stat(filepath.Join(root, "workspaces", "reserved")); statErr != nil { + t.Fatalf("failed reservation was unexpectedly removed: %v", statErr) + } +} + func TestManager_PathContainment(t *testing.T) { t.Parallel() ctx := context.Background() diff --git a/npm/package.test.mjs b/npm/package.test.mjs index 9c2ad95..d0f329a 100644 --- a/npm/package.test.mjs +++ b/npm/package.test.mjs @@ -1,5 +1,5 @@ import assert from "node:assert/strict"; -import { mkdir, mkdtemp, readFile, readdir, rm, writeFile } from "node:fs/promises"; +import { chmod, mkdir, mkdtemp, readFile, readdir, rm, writeFile } from "node:fs/promises"; import { spawnSync } from "node:child_process"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -65,3 +65,130 @@ test("MCP Registry metadata identifies the published package", async () => { transport: { type: "stdio" }, }]); }); + +test("Release Please versions all MCP Registry metadata fields", async () => { + const config = JSON.parse(await readFile(join(root, "release-please-config.json"), "utf8")); + const registryVersionFields = config.packages["."]["extra-files"].filter(({ path }) => path === "server.json"); + assert.deepEqual(registryVersionFields, [ + { type: "json", path: "server.json", jsonpath: "$.version" }, + { type: "json", path: "server.json", jsonpath: "$.packages[*].version" }, + ]); +}); + +test("release verifies npm discovery before publishing MCP Registry metadata", async () => { + const workflow = await readFile(join(root, ".github", "workflows", "release.yml"), "utf8"); + const npmVerification = workflow.indexOf("Verify npm publication is publicly discoverable"); + const mcpPublication = workflow.indexOf("Publish MCP Registry metadata"); + assert.ok(npmVerification >= 0, "release workflow must verify npm publication"); + assert.ok(mcpPublication > npmVerification, "MCP Registry metadata must follow npm discovery verification"); + assert.match(workflow, /node scripts\/verify-npm-publication\.mjs/); +}); + +test("publication verification requires the public latest tag and a fresh npx runtime", async () => { + const workspace = await mkdtemp(join(tmpdir(), "gitcontribute-publication-check-")); + try { + const client = join(workspace, "registry-client"); + const log = join(workspace, "calls.log"); + await writeFile(client, `#!/usr/bin/env node +const fs = require("node:fs"); +const args = process.argv.slice(2); +fs.appendFileSync(process.env.GITCONTRIBUTE_TEST_CALL_LOG, JSON.stringify(args) + "\\n"); +if (args[0] === "view" && args[2] === "dist-tags.latest") process.stdout.write('"1.2.3"\\n'); +else if (args[0] === "view" && args[1] === "gitcontribute@1.2.3" && args[2] === "version") process.stdout.write('"1.2.3"\\n'); +else if (args[0] === "--yes") process.stdout.write('{"version":"1.2.3"}\\n'); +else process.exitCode = 1; +`); + await chmod(client, 0o755); + + const result = spawnSync(process.execPath, [join(root, "scripts", "verify-npm-publication.mjs"), "1.2.3"], { + encoding: "utf8", + env: { + ...process.env, + GITCONTRIBUTE_NPM_COMMAND: client, + GITCONTRIBUTE_NPX_COMMAND: client, + GITCONTRIBUTE_NPM_PUBLICATION_ATTEMPTS: "1", + GITCONTRIBUTE_TEST_CALL_LOG: log, + }, + }); + assert.equal(result.status, 0, result.stderr || result.stdout); + const calls = (await readFile(log, "utf8")).trim().split("\n").map(JSON.parse); + assert.deepEqual(calls, [ + ["view", "gitcontribute", "dist-tags.latest", "--json", "--prefer-online", "--registry=https://registry.npmjs.org"], + ["view", "gitcontribute@1.2.3", "version", "--json", "--prefer-online", "--registry=https://registry.npmjs.org"], + ["--yes", "--prefer-online", "gitcontribute@latest", "metadata", "--json"], + ]); + } finally { + await rm(workspace, { recursive: true, force: true }); + } +}); + +test("publication verification retries transient registry probe failures", async () => { + const workspace = await mkdtemp(join(tmpdir(), "gitcontribute-publication-retry-")); + try { + const client = join(workspace, "registry-client"); + const state = join(workspace, "attempts"); + await writeFile(client, `#!/usr/bin/env node +const fs = require("node:fs"); +const count = fs.existsSync(process.env.GITCONTRIBUTE_TEST_ATTEMPTS) ? Number(fs.readFileSync(process.env.GITCONTRIBUTE_TEST_ATTEMPTS, "utf8")) : 0; +fs.writeFileSync(process.env.GITCONTRIBUTE_TEST_ATTEMPTS, String(count + 1)); +if (count === 0) process.exitCode = 1; +else if (process.argv[2] === "view" && process.argv[4] === "dist-tags.latest") process.stdout.write('"1.2.3"\\n'); +else if (process.argv[2] === "view" && process.argv[3] === "gitcontribute@1.2.3" && process.argv[4] === "version") process.stdout.write('"1.2.3"\\n'); +else if (process.argv[2] === "--yes") process.stdout.write('{"version":"1.2.3"}\\n'); +else process.exitCode = 1; +`); + await chmod(client, 0o755); + + const result = spawnSync(process.execPath, [join(root, "scripts", "verify-npm-publication.mjs"), "1.2.3"], { + encoding: "utf8", + env: { + ...process.env, + GITCONTRIBUTE_NPM_COMMAND: client, + GITCONTRIBUTE_NPX_COMMAND: client, + GITCONTRIBUTE_NPM_PUBLICATION_ATTEMPTS: "2", + GITCONTRIBUTE_NPM_PUBLICATION_DELAY_MS: "1", + GITCONTRIBUTE_TEST_ATTEMPTS: state, + }, + }); + assert.equal(result.status, 0, result.stderr || result.stdout); + assert.equal(await readFile(state, "utf8"), "4"); + } finally { + await rm(workspace, { recursive: true, force: true }); + } +}); + +test("publication verification times out a hung probe before retrying", async () => { + const workspace = await mkdtemp(join(tmpdir(), "gitcontribute-publication-timeout-")); + try { + const client = join(workspace, "registry-client"); + const state = join(workspace, "attempts"); + await writeFile(client, `#!/usr/bin/env node +const fs = require("node:fs"); +const count = fs.existsSync(process.env.GITCONTRIBUTE_TEST_ATTEMPTS) ? Number(fs.readFileSync(process.env.GITCONTRIBUTE_TEST_ATTEMPTS, "utf8")) : 0; +fs.writeFileSync(process.env.GITCONTRIBUTE_TEST_ATTEMPTS, String(count + 1)); +if (count === 0) setInterval(() => {}, 1_000); +else if (process.argv[2] === "view" && process.argv[4] === "dist-tags.latest") process.stdout.write('"1.2.3"\\n'); +else if (process.argv[2] === "view" && process.argv[3] === "gitcontribute@1.2.3" && process.argv[4] === "version") process.stdout.write('"1.2.3"\\n'); +else if (process.argv[2] === "--yes") process.stdout.write('{"version":"1.2.3"}\\n'); +else process.exitCode = 1; +`); + await chmod(client, 0o755); + + const result = spawnSync(process.execPath, [join(root, "scripts", "verify-npm-publication.mjs"), "1.2.3"], { + encoding: "utf8", + env: { + ...process.env, + GITCONTRIBUTE_NPM_COMMAND: client, + GITCONTRIBUTE_NPX_COMMAND: client, + GITCONTRIBUTE_NPM_PUBLICATION_ATTEMPTS: "2", + GITCONTRIBUTE_NPM_PUBLICATION_DELAY_MS: "1", + GITCONTRIBUTE_NPM_PUBLICATION_PROBE_TIMEOUT_MS: "500", + GITCONTRIBUTE_TEST_ATTEMPTS: state, + }, + }); + assert.equal(result.status, 0, result.stderr || result.stdout); + assert.equal(await readFile(state, "utf8"), "4"); + } finally { + await rm(workspace, { recursive: true, force: true }); + } +}); diff --git a/package.json b/package.json index 644ae5e..f6e08de 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "gitcontribute", - "version": "1.1.0", + "version": "2.0.0", "mcpName": "io.github.morluto/gitcontribute", "description": "Local-first GitHub contribution research workbench", "license": "MIT", diff --git a/release-please-config.json b/release-please-config.json index f85b37a..14337e3 100644 --- a/release-please-config.json +++ b/release-please-config.json @@ -7,6 +7,16 @@ "type": "json", "path": "package.json", "jsonpath": "$.version" + }, + { + "type": "json", + "path": "server.json", + "jsonpath": "$.version" + }, + { + "type": "json", + "path": "server.json", + "jsonpath": "$.packages[*].version" } ] } diff --git a/scripts/verify-npm-publication.mjs b/scripts/verify-npm-publication.mjs new file mode 100644 index 0000000..1f03adc --- /dev/null +++ b/scripts/verify-npm-publication.mjs @@ -0,0 +1,74 @@ +import { spawn } from "node:child_process"; + +const expectedVersion = process.argv[2]; +if (!expectedVersion) throw new Error("expected version argument is required"); + +const registry = "https://registry.npmjs.org"; +const attempts = positiveInteger("GITCONTRIBUTE_NPM_PUBLICATION_ATTEMPTS", 10, 30); +const delayMS = positiveInteger("GITCONTRIBUTE_NPM_PUBLICATION_DELAY_MS", 6_000, 60_000); +const probeTimeoutMS = positiveInteger("GITCONTRIBUTE_NPM_PUBLICATION_PROBE_TIMEOUT_MS", 30_000, 120_000); +const npm = process.env.GITCONTRIBUTE_NPM_COMMAND || "npm"; +const npx = process.env.GITCONTRIBUTE_NPX_COMMAND || "npx"; + +for (let attempt = 1; attempt <= attempts; attempt += 1) { + try { + const latest = await output(npm, ["view", "gitcontribute", "dist-tags.latest", "--json", "--prefer-online", `--registry=${registry}`]); + const published = await output(npm, ["view", `gitcontribute@${expectedVersion}`, "version", "--json", "--prefer-online", `--registry=${registry}`]); + if (jsonString(latest) === expectedVersion && jsonString(published) === expectedVersion) { + const metadata = await output(npx, ["--yes", "--prefer-online", "gitcontribute@latest", "metadata", "--json"]); + if (JSON.parse(metadata).version === expectedVersion) { + console.log(`npm release ${expectedVersion} is publicly discoverable`); + process.exit(0); + } + } + } catch { + // Registry propagation and fresh npx resolution are expected to be + // transient immediately after publication. The bounded retry loop owns + // those probes as one operation. + } + if (attempt < attempts) await new Promise((resolve) => setTimeout(resolve, delayMS)); +} + +throw new Error(`npm release ${expectedVersion} did not become publicly discoverable after ${attempts} attempts`); + +function positiveInteger(name, fallback, maximum) { + const value = process.env[name]; + if (value === undefined || value === "") return fallback; + if (!/^[1-9][0-9]*$/.test(value)) throw new Error(`${name} must be a positive integer`); + const parsed = Number(value); + if (!Number.isSafeInteger(parsed) || parsed > maximum) throw new Error(`${name} must be at most ${maximum}`); + return parsed; +} + +function jsonString(value) { + try { + const parsed = JSON.parse(value); + return typeof parsed === "string" ? parsed : ""; + } catch { + return ""; + } +} + +function output(command, args) { + return new Promise((resolve, reject) => { + const child = spawn(command, args, { stdio: ["ignore", "pipe", "ignore"] }); + let stdout = ""; + let forceKill; + const timeout = setTimeout(() => { + child.kill("SIGTERM"); + forceKill = setTimeout(() => child.kill("SIGKILL"), 1_000); + }, probeTimeoutMS); + const finish = (callback, value) => { + clearTimeout(timeout); + clearTimeout(forceKill); + callback(value); + }; + child.stdout.setEncoding("utf8"); + child.stdout.on("data", (chunk) => (stdout += chunk)); + child.on("error", (error) => finish(reject, error)); + child.on("close", (code, signal) => { + if (code === 0) return finish(resolve, stdout.trim()); + finish(reject, new Error(`${command} exited with code ${code ?? "null"}${signal ? ` (${signal})` : ""}`)); + }); + }); +} diff --git a/server.json b/server.json index 4c66a1a..ba27dc5 100644 --- a/server.json +++ b/server.json @@ -6,12 +6,12 @@ "url": "https://github.com/morluto/gitcontribute", "source": "github" }, - "version": "1.0.0", + "version": "2.0.0", "packages": [ { "registryType": "npm", "identifier": "gitcontribute", - "version": "1.0.0", + "version": "2.0.0", "transport": { "type": "stdio" }