From e5919c79989d4d15932c4711571406bdd89548aa Mon Sep 17 00:00:00 2001 From: harshitsinghbhandari <24b4506@iitb.ac.in> Date: Sat, 30 May 2026 13:11:15 +0530 Subject: [PATCH 1/4] feat(tracker): Tracker port + GitHub adapter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reference implementation; GitLab and Linear follow in separate PRs. Issue observer loop (poll + ApplyTrackerFacts) is deferred to #35. Three-layer split mirrors the SCM layout adil is landing in PR #28: - domain/tracker.go — value types (TrackerProvider, TrackerID, NormalizedIssueState, Issue) - ports/tracker.go — the Tracker interface - adapters/tracker/github/ — REST-backed adapter v1 is write-mostly: Get, Comment, Transition. No cache, no inflight dedup, no polling. State mapping is documented in the package doc and exercised by table-driven tests against an httptest fake — no real GitHub traffic from CI. Co-Authored-By: Claude Opus 4.7 --- .../internal/adapters/tracker/github/auth.go | 52 ++ .../internal/adapters/tracker/github/doc.go | 50 ++ .../adapters/tracker/github/tracker.go | 458 +++++++++++++++ .../adapters/tracker/github/tracker_test.go | 540 ++++++++++++++++++ backend/internal/domain/tracker.go | 49 ++ backend/internal/ports/tracker.go | 25 + 6 files changed, 1174 insertions(+) create mode 100644 backend/internal/adapters/tracker/github/auth.go create mode 100644 backend/internal/adapters/tracker/github/doc.go create mode 100644 backend/internal/adapters/tracker/github/tracker.go create mode 100644 backend/internal/adapters/tracker/github/tracker_test.go create mode 100644 backend/internal/domain/tracker.go create mode 100644 backend/internal/ports/tracker.go diff --git a/backend/internal/adapters/tracker/github/auth.go b/backend/internal/adapters/tracker/github/auth.go new file mode 100644 index 000000000..9aa810dff --- /dev/null +++ b/backend/internal/adapters/tracker/github/auth.go @@ -0,0 +1,52 @@ +package github + +import ( + "context" + "errors" + "os" + "strings" +) + +// TokenSource yields a GitHub bearer token on demand. It is intentionally +// tiny so tests can inject a static token and production can layer env-var or +// gh-CLI fallbacks behind the same surface. The Tracker calls Token once at +// construction (fail-fast) and again per request (so rotated tokens are +// picked up without restart). +type TokenSource interface { + Token(ctx context.Context) (string, error) +} + +// ErrNoToken is returned when no token source could yield a non-empty token. +var ErrNoToken = errors.New("github tracker: no token configured") + +// StaticTokenSource is a literal token, typically used in tests. +type StaticTokenSource string + +func (s StaticTokenSource) Token(context.Context) (string, error) { + t := strings.TrimSpace(string(s)) + if t == "" { + return "", ErrNoToken + } + return t, nil +} + +// EnvTokenSource reads the first non-empty value from the listed env vars, +// falling back to GITHUB_TOKEN. The order matters: a project-configured +// token (e.g. AO_GITHUB_TOKEN) should be preferred over the global default, +// matching the pattern PR #28 uses on the SCM side so both adapters honor +// the same precedence. +type EnvTokenSource struct { + EnvVars []string +} + +func (s EnvTokenSource) Token(context.Context) (string, error) { + for _, name := range s.EnvVars { + if v := strings.TrimSpace(os.Getenv(name)); v != "" { + return v, nil + } + } + if v := strings.TrimSpace(os.Getenv("GITHUB_TOKEN")); v != "" { + return v, nil + } + return "", ErrNoToken +} diff --git a/backend/internal/adapters/tracker/github/doc.go b/backend/internal/adapters/tracker/github/doc.go new file mode 100644 index 000000000..98bda7c97 --- /dev/null +++ b/backend/internal/adapters/tracker/github/doc.go @@ -0,0 +1,50 @@ +// Package github implements the ports.Tracker outbound port for GitHub +// Issues. v1 is write-mostly: Get returns a normalized Issue snapshot, +// Comment posts an issue comment, and Transition projects the cross-provider +// state vocabulary onto GitHub's open/closed + state_reason + labels surface. +// There is no observer loop or cache — those arrive with issue #35. +// +// # Normalized state mapping +// +// GitHub Issues only have two native states (open, closed) plus a +// state_reason on closed issues (completed, not_planned, reopened). The +// orchestrator's lifecycle vocabulary is richer, so the adapter uses two +// well-known labels — "in-progress" and "in-review" — to project the extra +// states onto open issues. +// +// Normalized state | GitHub API calls performed by Transition +// -----------------+------------------------------------------------------- +// open | PATCH state=open; DELETE labels {in-progress,in-review} +// in_progress | PATCH state=open; POST label in-progress; +// | DELETE label in-review +// review | PATCH state=open; POST label in-review; +// | DELETE label in-progress +// done | PATCH state=closed,state_reason=completed; +// | DELETE labels {in-progress,in-review} +// cancelled | PATCH state=closed,state_reason=not_planned; +// | DELETE labels {in-progress,in-review} +// +// Reverse mapping (Get): GitHub state=closed maps to done if state_reason is +// completed or empty, and to cancelled if state_reason is not_planned. For +// open issues, an "in-review" label wins over "in-progress" (the workflow is +// progress -> review -> done), and the absence of both maps to open. +// +// # Label hygiene and partial failures +// +// DELETE on a label that the issue does not carry returns 404; Transition +// treats that as success so the operation is idempotent. +// +// Transition issues 2-3 HTTP requests sequentially (PATCH, optional POST +// label, DELETE label) and is NOT atomic. If the PATCH succeeds but a +// subsequent label call fails, the issue is left in an intermediate state +// (e.g. closed without the status label cleared). Re-invoking Transition +// with the same target state is safe and converges — callers should treat +// the operation as eventually-consistent and retry on transport errors. +// +// # Out of scope +// +// - No webhook receiver, no polling goroutine, no fact projection into LCM +// (see issue #35 for the observer-loop work). +// - No richer per-provider metadata on Issue (milestones, project boards, +// reactions); the port only carries fields all three v1 providers can fill. +package github diff --git a/backend/internal/adapters/tracker/github/tracker.go b/backend/internal/adapters/tracker/github/tracker.go new file mode 100644 index 000000000..62d8c8904 --- /dev/null +++ b/backend/internal/adapters/tracker/github/tracker.go @@ -0,0 +1,458 @@ +package github + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "fmt" + "io" + "net/http" + "net/url" + "strconv" + "strings" + "time" + + "github.com/aoagents/agent-orchestrator/backend/internal/domain" + "github.com/aoagents/agent-orchestrator/backend/internal/ports" +) + +const ( + defaultBaseURL = "https://api.github.com" + defaultUserAgent = "ao-agent-orchestrator/tracker-github" + + labelInProgress = "in-progress" + labelInReview = "in-review" + + stateOpenGH = "open" + stateClosedGH = "closed" + reasonComplete = "completed" + reasonNotPlan = "not_planned" +) + +// Sentinel errors. Adapter-level callers should match on these via +// errors.Is; the orchestrator's lifecycle code is intentionally insulated +// from raw HTTP status codes. +var ( + ErrNotFound = errors.New("github tracker: issue not found") + ErrRateLimited = errors.New("github tracker: rate limited") + ErrEmptyBody = errors.New("github tracker: comment body is empty") + ErrWrongProvider = errors.New("github tracker: id is not a github tracker id") + ErrUnknownState = errors.New("github tracker: unknown normalized state") + ErrBadID = errors.New("github tracker: malformed native id") +) + +// RateLimitError is returned when GitHub reports the request was rate-limited. +// Callers that want to back off intelligently can extract ResetAt / +// RetryAfter via errors.As; callers that only need the category can use +// errors.Is(err, ErrRateLimited). +type RateLimitError struct { + ResetAt time.Time + RetryAfter time.Duration + Message string +} + +func (e *RateLimitError) Error() string { + if e == nil { + return ErrRateLimited.Error() + } + if e.Message != "" { + return "github tracker: rate limited: " + e.Message + } + return ErrRateLimited.Error() +} + +func (e *RateLimitError) Is(target error) bool { return target == ErrRateLimited } + +// Options configures a Tracker. All fields except Token are optional — +// production code typically sets Token alone; tests inject HTTPClient and +// BaseURL to point at an httptest fake. +type Options struct { + Token TokenSource + HTTPClient *http.Client + BaseURL string + UserAgent string +} + +// Tracker implements ports.Tracker against the GitHub REST API. +// +// Construction performs a fail-fast token presence check (no network call — +// validating the token's authorization scope against GitHub requires a real +// request, and that is the first operation any caller will make anyway). +type Tracker struct { + http *http.Client + tokens TokenSource + baseURL string + userAgent string +} + +// New returns a Tracker. It fails fast when no token can be obtained so +// daemons crash at startup rather than at first issue lookup. +func New(opts Options) (*Tracker, error) { + src := opts.Token + if src == nil { + return nil, ErrNoToken + } + if _, err := src.Token(context.Background()); err != nil { + return nil, err + } + t := &Tracker{ + http: opts.HTTPClient, + tokens: src, + baseURL: opts.BaseURL, + userAgent: opts.UserAgent, + } + if t.http == nil { + t.http = &http.Client{Timeout: 30 * time.Second} + } + if t.baseURL == "" { + t.baseURL = defaultBaseURL + } + if t.userAgent == "" { + t.userAgent = defaultUserAgent + } + return t, nil +} + +// Statically assert Tracker satisfies the port. If this stops compiling, the +// port shape changed and the adapter needs to follow. +var _ ports.Tracker = (*Tracker)(nil) + +// --------------------------------------------------------------------------- +// Get +// --------------------------------------------------------------------------- + +// ghIssue is the subset of fields we read off the REST issue payload. +type ghIssue struct { + Number int `json:"number"` + Title string `json:"title"` + Body string `json:"body"` + State string `json:"state"` + StateReason string `json:"state_reason"` + HTMLURL string `json:"html_url"` + Labels []ghLabel `json:"labels"` + Assignees []ghUser `json:"assignees"` +} + +type ghLabel struct { + Name string `json:"name"` +} + +type ghUser struct { + Login string `json:"login"` +} + +func (t *Tracker) Get(ctx context.Context, id domain.TrackerID) (domain.Issue, error) { + owner, repo, number, err := t.parseID(id) + if err != nil { + return domain.Issue{}, err + } + path := fmt.Sprintf("/repos/%s/%s/issues/%d", owner, repo, number) + + resp, err := t.do(ctx, http.MethodGet, path, nil) + if err != nil { + return domain.Issue{}, err + } + var raw ghIssue + if err := json.Unmarshal(resp, &raw); err != nil { + return domain.Issue{}, fmt.Errorf("github tracker: decode issue: %w", err) + } + labels := make([]string, 0, len(raw.Labels)) + for _, l := range raw.Labels { + labels = append(labels, l.Name) + } + assignees := make([]string, 0, len(raw.Assignees)) + for _, a := range raw.Assignees { + assignees = append(assignees, a.Login) + } + out := domain.Issue{ + // Canonicalize Provider so the returned Issue always re-routes back + // to this adapter, even if the caller built id with a zero Provider. + ID: domain.TrackerID{Provider: domain.TrackerProviderGitHub, Native: id.Native}, + Title: raw.Title, + Body: raw.Body, + State: mapStateFromGitHub(raw.State, raw.StateReason, labels), + URL: raw.HTMLURL, + Labels: labels, + Assignees: assignees, + } + if len(out.Labels) == 0 { + out.Labels = nil + } + if len(out.Assignees) == 0 { + out.Assignees = nil + } + return out, nil +} + +// mapStateFromGitHub projects GitHub's open/closed + state_reason + labels +// surface onto the normalized state. "in-review" wins over "in-progress" +// when both labels are present (the workflow is progress -> review -> done). +func mapStateFromGitHub(state, reason string, labels []string) domain.NormalizedIssueState { + switch strings.ToLower(state) { + case stateClosedGH: + if strings.EqualFold(reason, reasonNotPlan) { + return domain.IssueCancelled + } + return domain.IssueDone + } + var hasProgress, hasReview bool + for _, l := range labels { + switch l { + case labelInProgress: + hasProgress = true + case labelInReview: + hasReview = true + } + } + switch { + case hasReview: + return domain.IssueInReview + case hasProgress: + return domain.IssueInProgress + default: + return domain.IssueOpen + } +} + +// --------------------------------------------------------------------------- +// Comment +// --------------------------------------------------------------------------- + +func (t *Tracker) Comment(ctx context.Context, id domain.TrackerID, body string) error { + if strings.TrimSpace(body) == "" { + return ErrEmptyBody + } + owner, repo, number, err := t.parseID(id) + if err != nil { + return err + } + path := fmt.Sprintf("/repos/%s/%s/issues/%d/comments", owner, repo, number) + _, err = t.do(ctx, http.MethodPost, path, map[string]string{"body": body}) + return err +} + +// --------------------------------------------------------------------------- +// Transition +// --------------------------------------------------------------------------- + +// transitionPlan is the per-target-state list of mutations to apply. Every +// transition issues exactly one PATCH on the issue, optionally adds one +// status label, and removes any other status labels the issue may carry. +type transitionPlan struct { + patch map[string]any + addLabel string // "" means none + removeLabel []string +} + +func planForState(state domain.NormalizedIssueState) (transitionPlan, error) { + switch state { + case domain.IssueOpen: + return transitionPlan{ + patch: map[string]any{"state": stateOpenGH}, + removeLabel: []string{labelInProgress, labelInReview}, + }, nil + case domain.IssueInProgress: + return transitionPlan{ + patch: map[string]any{"state": stateOpenGH}, + addLabel: labelInProgress, + removeLabel: []string{labelInReview}, + }, nil + case domain.IssueInReview: + return transitionPlan{ + patch: map[string]any{"state": stateOpenGH}, + addLabel: labelInReview, + removeLabel: []string{labelInProgress}, + }, nil + case domain.IssueDone: + return transitionPlan{ + patch: map[string]any{"state": stateClosedGH, "state_reason": reasonComplete}, + removeLabel: []string{labelInProgress, labelInReview}, + }, nil + case domain.IssueCancelled: + return transitionPlan{ + patch: map[string]any{"state": stateClosedGH, "state_reason": reasonNotPlan}, + removeLabel: []string{labelInProgress, labelInReview}, + }, nil + default: + return transitionPlan{}, fmt.Errorf("%w: %q", ErrUnknownState, state) + } +} + +func (t *Tracker) Transition(ctx context.Context, id domain.TrackerID, state domain.NormalizedIssueState) error { + plan, err := planForState(state) + if err != nil { + return err + } + owner, repo, number, err := t.parseID(id) + if err != nil { + return err + } + issuePath := fmt.Sprintf("/repos/%s/%s/issues/%d", owner, repo, number) + + // 1. Patch state (+ state_reason for closed transitions). + if _, err := t.do(ctx, http.MethodPatch, issuePath, plan.patch); err != nil { + return err + } + // 2. Add the target status label (no-op when target is open/done/cancelled). + if plan.addLabel != "" { + body := map[string][]string{"labels": {plan.addLabel}} + if _, err := t.do(ctx, http.MethodPost, issuePath+"/labels", body); err != nil { + return err + } + } + // 3. Remove the other status labels. 404 from GitHub means "label is not + // on this issue", which is the success state we want — swallow it so + // the operation is idempotent. + for _, label := range plan.removeLabel { + labelPath := issuePath + "/labels/" + url.PathEscape(label) + if _, err := t.do(ctx, http.MethodDelete, labelPath, nil); err != nil { + if errors.Is(err, ErrNotFound) { + continue + } + return err + } + } + return nil +} + +// --------------------------------------------------------------------------- +// HTTP plumbing +// --------------------------------------------------------------------------- + +func (t *Tracker) do(ctx context.Context, method, path string, body any) ([]byte, error) { + var rdr io.Reader + if body != nil { + b, err := json.Marshal(body) + if err != nil { + return nil, fmt.Errorf("github tracker: encode body: %w", err) + } + rdr = bytes.NewReader(b) + } + req, err := http.NewRequestWithContext(ctx, method, t.baseURL+path, rdr) + if err != nil { + return nil, fmt.Errorf("github tracker: build request: %w", err) + } + if body != nil { + req.Header.Set("Content-Type", "application/json") + } + req.Header.Set("Accept", "application/vnd.github+json") + req.Header.Set("X-GitHub-Api-Version", "2022-11-28") + req.Header.Set("User-Agent", t.userAgent) + tok, err := t.tokens.Token(ctx) + if err != nil { + return nil, err + } + req.Header.Set("Authorization", "Bearer "+tok) + + resp, err := t.http.Do(req) + if err != nil { + return nil, fmt.Errorf("github tracker: %s %s: %w", method, path, err) + } + defer resp.Body.Close() + respBody, _ := io.ReadAll(resp.Body) + if resp.StatusCode >= 200 && resp.StatusCode < 300 { + return respBody, nil + } + return respBody, classifyError(resp, respBody) +} + +func classifyError(resp *http.Response, body []byte) error { + msg := githubMessage(body) + switch resp.StatusCode { + case http.StatusNotFound: + return fmt.Errorf("%w: %s", ErrNotFound, msg) + case http.StatusTooManyRequests: + return rateLimited(resp, msg) + case http.StatusForbidden, http.StatusUnauthorized: + // GitHub returns 403 for primary rate-limit exhaustion, for + // secondary/abuse limits, and for genuine auth failures. Three + // signals disambiguate the rate-limit cases from auth: the primary + // limit sets X-RateLimit-Remaining=0; the secondary/abuse limit + // sets Retry-After (and often omits X-RateLimit-Remaining); and + // either case mentions "rate limit" / "abuse" in the body. + if isRateLimited(resp, msg) { + return rateLimited(resp, msg) + } + } + return fmt.Errorf("github tracker: %d %s", resp.StatusCode, msg) +} + +func isRateLimited(resp *http.Response, msg string) bool { + if rem := resp.Header.Get("X-RateLimit-Remaining"); rem != "" { + if n, err := strconv.Atoi(rem); err == nil && n == 0 { + return true + } + } + if resp.Header.Get("Retry-After") != "" { + return true + } + low := strings.ToLower(msg) + return strings.Contains(low, "rate limit") || strings.Contains(low, "abuse detection") +} + +func rateLimited(resp *http.Response, msg string) error { + e := &RateLimitError{Message: msg} + if reset := resp.Header.Get("X-RateLimit-Reset"); reset != "" { + if sec, err := strconv.ParseInt(reset, 10, 64); err == nil && sec > 0 { + e.ResetAt = time.Unix(sec, 0) + } + } + if ra := resp.Header.Get("Retry-After"); ra != "" { + if sec, err := strconv.Atoi(ra); err == nil && sec >= 0 { + e.RetryAfter = time.Duration(sec) * time.Second + } + } + return e +} + +func githubMessage(body []byte) string { + var p struct { + Message string `json:"message"` + } + if json.Unmarshal(body, &p) == nil && p.Message != "" { + return p.Message + } + return strings.TrimSpace(string(body)) +} + +// --------------------------------------------------------------------------- +// ID parsing +// --------------------------------------------------------------------------- + +func (t *Tracker) parseID(id domain.TrackerID) (owner, repo string, number int, err error) { + // Strict: the Session Manager picks an adapter by Provider, so reaching + // this adapter with a non-github Provider is a routing bug, not user + // input. Empty Provider is treated the same way — it would round-trip + // to an Issue whose ID can't be re-routed. + if id.Provider != domain.TrackerProviderGitHub { + return "", "", 0, fmt.Errorf("%w: provider=%q", ErrWrongProvider, id.Provider) + } + return parseGitHubID(id.Native) +} + +// parseGitHubID accepts "owner/repo#NUM" and returns the three components. +// Forms like "owner/repo/issues/NUM" or bare numbers are intentionally +// rejected so the rest of the system has one canonical id shape. +func parseGitHubID(native string) (owner, repo string, number int, err error) { + hash := strings.IndexByte(native, '#') + if hash < 0 { + return "", "", 0, fmt.Errorf("%w: missing #issue", ErrBadID) + } + repoPart := native[:hash] + numPart := native[hash+1:] + slash := strings.IndexByte(repoPart, '/') + if slash < 0 { + return "", "", 0, fmt.Errorf("%w: missing owner/repo separator", ErrBadID) + } + owner = repoPart[:slash] + repo = repoPart[slash+1:] + if owner == "" || repo == "" { + return "", "", 0, fmt.Errorf("%w: empty owner or repo", ErrBadID) + } + n, parseErr := strconv.Atoi(numPart) + if parseErr != nil || n <= 0 { + return "", "", 0, fmt.Errorf("%w: bad issue number %q", ErrBadID, numPart) + } + return owner, repo, n, nil +} diff --git a/backend/internal/adapters/tracker/github/tracker_test.go b/backend/internal/adapters/tracker/github/tracker_test.go new file mode 100644 index 000000000..44f5a6cd7 --- /dev/null +++ b/backend/internal/adapters/tracker/github/tracker_test.go @@ -0,0 +1,540 @@ +package github + +import ( + "context" + "encoding/json" + "errors" + "io" + "net/http" + "net/http/httptest" + "reflect" + "sort" + "strconv" + "strings" + "sync" + "testing" + "time" + + "github.com/aoagents/agent-orchestrator/backend/internal/domain" +) + +// recordedReq captures one inbound HTTP request so tests can assert against +// the exact GitHub API surface the adapter touched. +type recordedReq struct { + Method string + Path string + Body string +} + +// fakeGH is a programmable httptest.Server that matches requests by +// "METHOD path" and records every call. Unmatched requests fail the test — +// that is the point of TDD here, so an accidental extra call is loud. +type fakeGH struct { + t *testing.T + server *httptest.Server + mu sync.Mutex + requests []recordedReq + handlers map[string]http.HandlerFunc +} + +func newFakeGH(t *testing.T) *fakeGH { + t.Helper() + f := &fakeGH{t: t, handlers: map[string]http.HandlerFunc{}} + f.server = httptest.NewServer(http.HandlerFunc(f.serve)) + t.Cleanup(f.server.Close) + return f +} + +func (f *fakeGH) on(method, path string, h http.HandlerFunc) { + f.handlers[method+" "+path] = h +} + +func (f *fakeGH) serve(w http.ResponseWriter, r *http.Request) { + body, _ := io.ReadAll(r.Body) + f.mu.Lock() + f.requests = append(f.requests, recordedReq{Method: r.Method, Path: r.URL.Path, Body: string(body)}) + f.mu.Unlock() + key := r.Method + " " + r.URL.Path + h, ok := f.handlers[key] + if !ok { + f.t.Errorf("unexpected request: %s", key) + http.Error(w, "no handler", http.StatusNotImplemented) + return + } + r.Body = io.NopCloser(strings.NewReader(string(body))) + h(w, r) +} + +func (f *fakeGH) calls() []recordedReq { + f.mu.Lock() + defer f.mu.Unlock() + out := make([]recordedReq, len(f.requests)) + copy(out, f.requests) + return out +} + +// newTrackerForTest constructs an adapter pointed at the fake server with a +// static dev token. Production code uses EnvTokenSource; tests skip that to +// keep the surface tiny. +func newTrackerForTest(t *testing.T, f *fakeGH) *Tracker { + t.Helper() + tr, err := New(Options{ + BaseURL: f.server.URL, + Token: StaticTokenSource("tkn-test"), + HTTPClient: f.server.Client(), + }) + if err != nil { + t.Fatalf("New: %v", err) + } + return tr +} + +func ctx() context.Context { return context.Background() } + +func TestNewRejectsMissingToken(t *testing.T) { + if _, err := New(Options{Token: StaticTokenSource("")}); !errors.Is(err, ErrNoToken) { + t.Fatalf("New with empty token = %v, want ErrNoToken", err) + } + if _, err := New(Options{}); !errors.Is(err, ErrNoToken) { + t.Fatalf("New with no source = %v, want ErrNoToken", err) + } +} + +func TestParseID(t *testing.T) { + cases := []struct { + name string + native string + wantOwner string + wantRepo string + wantNum int + wantErr bool + }{ + {"happy", "octocat/hello-world#42", "octocat", "hello-world", 42, false}, + {"missing hash", "octocat/hello-world", "", "", 0, true}, + {"missing slash", "octocat#42", "", "", 0, true}, + {"empty owner", "/repo#1", "", "", 0, true}, + {"empty repo", "owner/#1", "", "", 0, true}, + {"non-numeric", "o/r#abc", "", "", 0, true}, + {"zero", "o/r#0", "", "", 0, true}, + {"negative", "o/r#-1", "", "", 0, true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + owner, repo, num, err := parseGitHubID(tc.native) + if tc.wantErr { + if err == nil { + t.Fatalf("expected error, got %s/%s#%d", owner, repo, num) + } + return + } + if err != nil { + t.Fatalf("parse: %v", err) + } + if owner != tc.wantOwner || repo != tc.wantRepo || num != tc.wantNum { + t.Fatalf("got %s/%s#%d, want %s/%s#%d", owner, repo, num, tc.wantOwner, tc.wantRepo, tc.wantNum) + } + }) + } +} + +func TestGet_HappyPath(t *testing.T) { + f := newFakeGH(t) + f.on("GET", "/repos/octocat/hello-world/issues/42", func(w http.ResponseWriter, r *http.Request) { + if got := r.Header.Get("Authorization"); got != "Bearer tkn-test" { + t.Errorf("Authorization = %q, want Bearer tkn-test", got) + } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{ + "number": 42, + "title": "Found a bug", + "body": "It does not work", + "state": "open", + "html_url": "https://github.com/octocat/hello-world/issues/42", + "labels": [{"name":"bug"},{"name":"in-progress"}], + "assignees": [{"login":"alice"},{"login":"bob"}] + }`)) + }) + tr := newTrackerForTest(t, f) + + issue, err := tr.Get(ctx(), domain.TrackerID{Provider: domain.TrackerProviderGitHub, Native: "octocat/hello-world#42"}) + if err != nil { + t.Fatalf("Get: %v", err) + } + want := domain.Issue{ + ID: domain.TrackerID{Provider: domain.TrackerProviderGitHub, Native: "octocat/hello-world#42"}, + Title: "Found a bug", + Body: "It does not work", + State: domain.IssueInProgress, // the "in-progress" label wins over plain "open" + URL: "https://github.com/octocat/hello-world/issues/42", + Labels: []string{"bug", "in-progress"}, + Assignees: []string{"alice", "bob"}, + } + if !reflect.DeepEqual(issue, want) { + t.Fatalf("issue = %#v\nwant %#v", issue, want) + } +} + +func TestGet_StateMappingFromGitHubFields(t *testing.T) { + cases := []struct { + name string + ghState string + ghReason string + labels []string + wantState domain.NormalizedIssueState + }{ + {"plain open", "open", "", nil, domain.IssueOpen}, + {"open with in-progress label", "open", "", []string{"in-progress"}, domain.IssueInProgress}, + {"open with in-review label", "open", "", []string{"in-review"}, domain.IssueInReview}, + {"review wins over progress when both present", "open", "", []string{"in-progress", "in-review"}, domain.IssueInReview}, + {"closed completed", "closed", "completed", nil, domain.IssueDone}, + {"closed not_planned", "closed", "not_planned", nil, domain.IssueCancelled}, + {"closed unknown reason maps to done", "closed", "", nil, domain.IssueDone}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + f := newFakeGH(t) + payload := map[string]any{ + "number": 1, + "title": "t", + "body": "", + "state": tc.ghState, + "html_url": "https://github.com/o/r/issues/1", + } + if tc.ghReason != "" { + payload["state_reason"] = tc.ghReason + } + if tc.labels != nil { + ls := make([]map[string]string, len(tc.labels)) + for i, n := range tc.labels { + ls[i] = map[string]string{"name": n} + } + payload["labels"] = ls + } + b, _ := json.Marshal(payload) + f.on("GET", "/repos/o/r/issues/1", func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write(b) + }) + tr := newTrackerForTest(t, f) + issue, err := tr.Get(ctx(), domain.TrackerID{Provider: domain.TrackerProviderGitHub, Native: "o/r#1"}) + if err != nil { + t.Fatalf("Get: %v", err) + } + if issue.State != tc.wantState { + t.Fatalf("state = %q, want %q", issue.State, tc.wantState) + } + }) + } +} + +func TestGet_NotFound(t *testing.T) { + f := newFakeGH(t) + f.on("GET", "/repos/o/r/issues/1", func(w http.ResponseWriter, r *http.Request) { + http.Error(w, `{"message":"Not Found"}`, http.StatusNotFound) + }) + tr := newTrackerForTest(t, f) + _, err := tr.Get(ctx(), domain.TrackerID{Provider: domain.TrackerProviderGitHub, Native: "o/r#1"}) + if !errors.Is(err, ErrNotFound) { + t.Fatalf("err = %v, want ErrNotFound", err) + } +} + +func TestGet_RateLimited(t *testing.T) { + f := newFakeGH(t) + reset := time.Now().Add(2 * time.Minute).Unix() + f.on("GET", "/repos/o/r/issues/1", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("X-RateLimit-Remaining", "0") + w.Header().Set("X-RateLimit-Reset", strconv.FormatInt(reset, 10)) + http.Error(w, `{"message":"API rate limit exceeded"}`, http.StatusForbidden) + }) + tr := newTrackerForTest(t, f) + _, err := tr.Get(ctx(), domain.TrackerID{Provider: domain.TrackerProviderGitHub, Native: "o/r#1"}) + if !errors.Is(err, ErrRateLimited) { + t.Fatalf("err = %v, want ErrRateLimited", err) + } + var rle *RateLimitError + if !errors.As(err, &rle) { + t.Fatalf("err = %v, want *RateLimitError", err) + } + if got := rle.ResetAt.Unix(); got != reset { + t.Fatalf("ResetAt = %d, want %d", got, reset) + } +} + +// TestGet_SecondaryRateLimit covers the GitHub "abuse detection" +// response — it lacks X-RateLimit-Remaining but sets Retry-After, and the +// body mentions the limit. The classifier must still surface this as +// ErrRateLimited rather than mis-categorizing it as auth failure. +func TestGet_SecondaryRateLimit(t *testing.T) { + f := newFakeGH(t) + f.on("GET", "/repos/o/r/issues/1", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Retry-After", "60") + http.Error(w, `{"message":"You have exceeded a secondary rate limit"}`, http.StatusForbidden) + }) + tr := newTrackerForTest(t, f) + _, err := tr.Get(ctx(), domain.TrackerID{Provider: domain.TrackerProviderGitHub, Native: "o/r#1"}) + if !errors.Is(err, ErrRateLimited) { + t.Fatalf("err = %v, want ErrRateLimited", err) + } + var rle *RateLimitError + if !errors.As(err, &rle) { + t.Fatalf("err = %v, want *RateLimitError", err) + } + if rle.RetryAfter != 60*time.Second { + t.Fatalf("RetryAfter = %v, want 60s", rle.RetryAfter) + } +} + +func TestGet_RejectsWrongProvider(t *testing.T) { + f := newFakeGH(t) + tr := newTrackerForTest(t, f) + _, err := tr.Get(ctx(), domain.TrackerID{Provider: domain.TrackerProviderGitLab, Native: "g/p#1"}) + if !errors.Is(err, ErrWrongProvider) { + t.Fatalf("err = %v, want ErrWrongProvider", err) + } +} + +func TestGet_RejectsEmptyProvider(t *testing.T) { + f := newFakeGH(t) + tr := newTrackerForTest(t, f) + _, err := tr.Get(ctx(), domain.TrackerID{Native: "o/r#1"}) + if !errors.Is(err, ErrWrongProvider) { + t.Fatalf("err = %v, want ErrWrongProvider", err) + } +} + +// TestGet_CanonicalizesProviderOnOutput pins the contract that returned +// Issues always carry domain.TrackerProviderGitHub, so callers can re-route +// without inspecting which adapter they originally talked to. +func TestGet_CanonicalizesProviderOnOutput(t *testing.T) { + f := newFakeGH(t) + f.on("GET", "/repos/o/r/issues/1", func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(`{"number":1,"title":"t","body":"","state":"open","html_url":"https://github.com/o/r/issues/1"}`)) + }) + tr := newTrackerForTest(t, f) + issue, err := tr.Get(ctx(), domain.TrackerID{Provider: domain.TrackerProviderGitHub, Native: "o/r#1"}) + if err != nil { + t.Fatalf("Get: %v", err) + } + if issue.ID.Provider != domain.TrackerProviderGitHub { + t.Fatalf("issue.ID.Provider = %q, want %q", issue.ID.Provider, domain.TrackerProviderGitHub) + } + if issue.ID.Native != "o/r#1" { + t.Fatalf("issue.ID.Native = %q, want o/r#1", issue.ID.Native) + } +} + +func TestComment_HappyPath(t *testing.T) { + f := newFakeGH(t) + f.on("POST", "/repos/o/r/issues/1/comments", func(w http.ResponseWriter, r *http.Request) { + var got struct { + Body string `json:"body"` + } + if err := json.NewDecoder(r.Body).Decode(&got); err != nil { + t.Fatalf("decode: %v", err) + } + if got.Body != "hello world" { + t.Errorf("body = %q, want %q", got.Body, "hello world") + } + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(`{"id":1}`)) + }) + tr := newTrackerForTest(t, f) + if err := tr.Comment(ctx(), domain.TrackerID{Provider: domain.TrackerProviderGitHub, Native: "o/r#1"}, "hello world"); err != nil { + t.Fatalf("Comment: %v", err) + } +} + +// TestComment_PreservesMarkdownBody locks in that we POST the body verbatim +// — no trimming, no escape-and-unescape round trip — so multi-line markdown +// notifications from the SM survive. +func TestComment_PreservesMarkdownBody(t *testing.T) { + f := newFakeGH(t) + body := "## status\n\n- step 1: done\n- step 2: **in progress**\n\n```go\nfmt.Println(\"hi\")\n```\n" + f.on("POST", "/repos/o/r/issues/1/comments", func(w http.ResponseWriter, r *http.Request) { + var got struct { + Body string `json:"body"` + } + if err := json.NewDecoder(r.Body).Decode(&got); err != nil { + t.Fatalf("decode: %v", err) + } + if got.Body != body { + t.Errorf("body = %q, want %q", got.Body, body) + } + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(`{"id":1}`)) + }) + tr := newTrackerForTest(t, f) + if err := tr.Comment(ctx(), domain.TrackerID{Provider: domain.TrackerProviderGitHub, Native: "o/r#1"}, body); err != nil { + t.Fatalf("Comment: %v", err) + } +} + +func TestComment_RejectsEmptyBody(t *testing.T) { + f := newFakeGH(t) + tr := newTrackerForTest(t, f) + for _, body := range []string{"", " ", "\n\t"} { + err := tr.Comment(ctx(), domain.TrackerID{Provider: domain.TrackerProviderGitHub, Native: "o/r#1"}, body) + if !errors.Is(err, ErrEmptyBody) { + t.Fatalf("body %q: err = %v, want ErrEmptyBody", body, err) + } + } + if calls := f.calls(); len(calls) != 0 { + t.Fatalf("unexpected calls on empty body: %#v", calls) + } +} + +// transitionCall is the normalized record of one GH API call made by +// Transition. The tests compare a sorted slice of these against the expected +// call set so we don't depend on call ordering. +type transitionCall struct { + Method string + Path string + // for PATCH /issues/N — JSON keys we care about + State string + StateReason string + // for POST .../labels — labels added + AddLabels []string +} + +func TestTransition_MapsToCorrectGitHubCalls(t *testing.T) { + cases := []struct { + name string + state domain.NormalizedIssueState + want []transitionCall + }{ + { + name: "open clears status labels and reopens", + state: domain.IssueOpen, + want: []transitionCall{ + {Method: "PATCH", Path: "/repos/o/r/issues/1", State: "open"}, + {Method: "DELETE", Path: "/repos/o/r/issues/1/labels/in-progress"}, + {Method: "DELETE", Path: "/repos/o/r/issues/1/labels/in-review"}, + }, + }, + { + name: "in_progress adds in-progress label, removes in-review", + state: domain.IssueInProgress, + want: []transitionCall{ + {Method: "PATCH", Path: "/repos/o/r/issues/1", State: "open"}, + {Method: "POST", Path: "/repos/o/r/issues/1/labels", AddLabels: []string{"in-progress"}}, + {Method: "DELETE", Path: "/repos/o/r/issues/1/labels/in-review"}, + }, + }, + { + name: "review adds in-review label, removes in-progress", + state: domain.IssueInReview, + want: []transitionCall{ + {Method: "PATCH", Path: "/repos/o/r/issues/1", State: "open"}, + {Method: "POST", Path: "/repos/o/r/issues/1/labels", AddLabels: []string{"in-review"}}, + {Method: "DELETE", Path: "/repos/o/r/issues/1/labels/in-progress"}, + }, + }, + { + name: "done closes as completed and cleans status labels", + state: domain.IssueDone, + want: []transitionCall{ + {Method: "PATCH", Path: "/repos/o/r/issues/1", State: "closed", StateReason: "completed"}, + {Method: "DELETE", Path: "/repos/o/r/issues/1/labels/in-progress"}, + {Method: "DELETE", Path: "/repos/o/r/issues/1/labels/in-review"}, + }, + }, + { + name: "cancelled closes as not_planned and cleans status labels", + state: domain.IssueCancelled, + want: []transitionCall{ + {Method: "PATCH", Path: "/repos/o/r/issues/1", State: "closed", StateReason: "not_planned"}, + {Method: "DELETE", Path: "/repos/o/r/issues/1/labels/in-progress"}, + {Method: "DELETE", Path: "/repos/o/r/issues/1/labels/in-review"}, + }, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + f := newFakeGH(t) + // PATCH endpoint returns an updated issue body + f.on("PATCH", "/repos/o/r/issues/1", func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(`{"number":1,"state":"open"}`)) + }) + // label-add endpoint + f.on("POST", "/repos/o/r/issues/1/labels", func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(`[]`)) + }) + // label-remove endpoints — return 404 sometimes to confirm we ignore it + f.on("DELETE", "/repos/o/r/issues/1/labels/in-progress", func(w http.ResponseWriter, r *http.Request) { + http.Error(w, `{"message":"Label does not exist"}`, http.StatusNotFound) + }) + f.on("DELETE", "/repos/o/r/issues/1/labels/in-review", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`[]`)) + }) + tr := newTrackerForTest(t, f) + if err := tr.Transition(ctx(), domain.TrackerID{Provider: domain.TrackerProviderGitHub, Native: "o/r#1"}, tc.state); err != nil { + t.Fatalf("Transition: %v", err) + } + got := normalizeCalls(t, f.calls()) + want := append([]transitionCall(nil), tc.want...) + sortCalls(got) + sortCalls(want) + if !reflect.DeepEqual(got, want) { + t.Fatalf("calls:\n got %#v\n want %#v", got, want) + } + }) + } +} + +func TestTransition_RejectsUnknownState(t *testing.T) { + f := newFakeGH(t) + tr := newTrackerForTest(t, f) + err := tr.Transition(ctx(), domain.TrackerID{Provider: domain.TrackerProviderGitHub, Native: "o/r#1"}, domain.NormalizedIssueState("frobnicated")) + if !errors.Is(err, ErrUnknownState) { + t.Fatalf("err = %v, want ErrUnknownState", err) + } + if calls := f.calls(); len(calls) != 0 { + t.Fatalf("unexpected calls: %#v", calls) + } +} + +// normalizeCalls converts the recordedReq slice into transitionCall records +// the test cases assert against, decoding the PATCH/label-add bodies. +func normalizeCalls(t *testing.T, reqs []recordedReq) []transitionCall { + t.Helper() + out := make([]transitionCall, 0, len(reqs)) + for _, r := range reqs { + tc := transitionCall{Method: r.Method, Path: r.Path} + switch { + case r.Method == "PATCH": + var body struct { + State string `json:"state"` + StateReason string `json:"state_reason"` + } + if r.Body != "" { + if err := json.Unmarshal([]byte(r.Body), &body); err != nil { + t.Fatalf("patch body: %v", err) + } + } + tc.State = body.State + tc.StateReason = body.StateReason + case r.Method == "POST" && strings.HasSuffix(r.Path, "/labels"): + var body struct { + Labels []string `json:"labels"` + } + if r.Body != "" { + if err := json.Unmarshal([]byte(r.Body), &body); err != nil { + t.Fatalf("labels body: %v", err) + } + } + tc.AddLabels = body.Labels + } + out = append(out, tc) + } + return out +} + +func sortCalls(s []transitionCall) { + sort.Slice(s, func(i, j int) bool { + if s[i].Method != s[j].Method { + return s[i].Method < s[j].Method + } + return s[i].Path < s[j].Path + }) +} diff --git a/backend/internal/domain/tracker.go b/backend/internal/domain/tracker.go new file mode 100644 index 000000000..202c6bb17 --- /dev/null +++ b/backend/internal/domain/tracker.go @@ -0,0 +1,49 @@ +package domain + +// TrackerProvider identifies an issue-tracker provider implementation. +// Provider differences (label-driven vs state-machine vs close-reason) are +// absorbed inside each adapter; the rest of the system only sees +// NormalizedIssueState. +type TrackerProvider string + +const ( + TrackerProviderGitHub TrackerProvider = "github" + TrackerProviderGitLab TrackerProvider = "gitlab" + TrackerProviderLinear TrackerProvider = "linear" +) + +// TrackerID identifies a single issue across providers. Native is the +// provider's own canonical form ("owner/repo#123" for GitHub, +// "group/project#456" for GitLab, "ABC-789" for Linear) and is parsed by the +// adapter. Provider is the discriminator the Session Manager uses to pick an +// adapter. +type TrackerID struct { + Provider TrackerProvider `json:"provider"` + Native string `json:"native"` +} + +// NormalizedIssueState is the cross-provider issue-state vocabulary every +// adapter must implement. The closed list is intentional — adding a value +// here is a port-level decision because every adapter must map it. +type NormalizedIssueState string + +const ( + IssueOpen NormalizedIssueState = "open" + IssueInProgress NormalizedIssueState = "in_progress" + IssueInReview NormalizedIssueState = "review" + IssueDone NormalizedIssueState = "done" + IssueCancelled NormalizedIssueState = "cancelled" +) + +// Issue is the minimum projection every tracker can produce. Fields are +// added only when all v1 providers (GitHub, GitLab, Linear) can populate +// them faithfully; richer metadata stays inside provider-specific code paths. +type Issue struct { + ID TrackerID `json:"id"` + Title string `json:"title"` + Body string `json:"body"` + State NormalizedIssueState `json:"state"` + URL string `json:"url"` + Labels []string `json:"labels,omitempty"` + Assignees []string `json:"assignees,omitempty"` +} diff --git a/backend/internal/ports/tracker.go b/backend/internal/ports/tracker.go new file mode 100644 index 000000000..642b1912a --- /dev/null +++ b/backend/internal/ports/tracker.go @@ -0,0 +1,25 @@ +package ports + +import ( + "context" + + "github.com/aoagents/agent-orchestrator/backend/internal/domain" +) + +// Tracker is the outbound port for issue trackers (GitHub Issues, GitLab +// Issues, Linear). v1 is write-mostly: spawn-bootstrap reads with Get, the +// Session Manager posts status updates with Comment, and lifecycle +// transitions (start, hand-off-to-review, close) propagate with Transition. +// There is no observer loop yet; polling and ApplyTrackerFacts arrive with +// issue #35. +// +// All three v1 providers share this interface. Provider differences (label +// vs state machine vs close reason) are absorbed inside each adapter via +// domain.NormalizedIssueState. Fields on domain.Issue exist only when every +// provider can populate them; richer per-provider metadata belongs behind a +// separate port. +type Tracker interface { + Get(ctx context.Context, id domain.TrackerID) (domain.Issue, error) + Comment(ctx context.Context, id domain.TrackerID, body string) error + Transition(ctx context.Context, id domain.TrackerID, state domain.NormalizedIssueState) error +} From 6e4ec499fb905a34099325bbfcd1373a92a6b307 Mon Sep 17 00:00:00 2001 From: harshitsinghbhandari <24b4506@iitb.ac.in> Date: Sat, 30 May 2026 21:53:44 +0530 Subject: [PATCH 2/4] refactor(tracker): drop Comment + Transition; v1 is read-only Get MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Scope correction: mirroring agent lifecycle onto the tracker (status comments, label/state updates) is not wanted in the current rewrite. That work is now tracked as issue #40 and will land once we decide on the opt-out knob, label setup, and Linear's workflow-state fit. Removes from the port and the GitHub adapter: - Comment(ctx, id, body) and ErrEmptyBody - Transition(ctx, id, state) and ErrUnknownState - planForState / transitionPlan and the forward state mapping - reasonComplete constant (only the Get reverse mapping is kept) - 11 tests + the transitionCall normalization helpers Kept (still load-bearing for Get): - All 5 NormalizedIssueState values — Get reports them faithfully when a repo carries the in-progress / in-review labels. - The reverse mapping in mapStateFromGitHub. - RateLimitError with ResetAt + RetryAfter (#35 will use it). Co-Authored-By: Claude Opus 4.7 --- .../internal/adapters/tracker/github/doc.go | 59 ++--- .../adapters/tracker/github/tracker.go | 114 +-------- .../adapters/tracker/github/tracker_test.go | 217 ------------------ backend/internal/ports/tracker.go | 15 +- 4 files changed, 32 insertions(+), 373 deletions(-) diff --git a/backend/internal/adapters/tracker/github/doc.go b/backend/internal/adapters/tracker/github/doc.go index 98bda7c97..f2114334a 100644 --- a/backend/internal/adapters/tracker/github/doc.go +++ b/backend/internal/adapters/tracker/github/doc.go @@ -1,50 +1,31 @@ // Package github implements the ports.Tracker outbound port for GitHub -// Issues. v1 is write-mostly: Get returns a normalized Issue snapshot, -// Comment posts an issue comment, and Transition projects the cross-provider -// state vocabulary onto GitHub's open/closed + state_reason + labels surface. -// There is no observer loop or cache — those arrive with issue #35. +// Issues. v1 is read-only: Get returns a normalized Issue snapshot that the +// Session Manager uses to hydrate the agent prompt during spawn-bootstrap. +// Writing back to the tracker (Comment, Transition) is deferred to issue +// #40; the observer/polling loop is deferred to issue #35. // -// # Normalized state mapping +// # Reverse state mapping // // GitHub Issues only have two native states (open, closed) plus a -// state_reason on closed issues (completed, not_planned, reopened). The -// orchestrator's lifecycle vocabulary is richer, so the adapter uses two -// well-known labels — "in-progress" and "in-review" — to project the extra -// states onto open issues. +// state_reason on closed issues (completed, not_planned, reopened). Get +// projects them onto the normalized state vocabulary as follows: // -// Normalized state | GitHub API calls performed by Transition -// -----------------+------------------------------------------------------- -// open | PATCH state=open; DELETE labels {in-progress,in-review} -// in_progress | PATCH state=open; POST label in-progress; -// | DELETE label in-review -// review | PATCH state=open; POST label in-review; -// | DELETE label in-progress -// done | PATCH state=closed,state_reason=completed; -// | DELETE labels {in-progress,in-review} -// cancelled | PATCH state=closed,state_reason=not_planned; -// | DELETE labels {in-progress,in-review} +// - closed + state_reason=not_planned -> cancelled +// - closed + (completed | empty | other) -> done +// - open + "in-review" label -> review (wins when +// both status labels are present; the workflow is progress -> review) +// - open + "in-progress" label -> in_progress +// - otherwise -> open // -// Reverse mapping (Get): GitHub state=closed maps to done if state_reason is -// completed or empty, and to cancelled if state_reason is not_planned. For -// open issues, an "in-review" label wins over "in-progress" (the workflow is -// progress -> review -> done), and the absence of both maps to open. -// -// # Label hygiene and partial failures -// -// DELETE on a label that the issue does not carry returns 404; Transition -// treats that as success so the operation is idempotent. -// -// Transition issues 2-3 HTTP requests sequentially (PATCH, optional POST -// label, DELETE label) and is NOT atomic. If the PATCH succeeds but a -// subsequent label call fails, the issue is left in an intermediate state -// (e.g. closed without the status label cleared). Re-invoking Transition -// with the same target state is safe and converges — callers should treat -// the operation as eventually-consistent and retry on transport errors. +// The "in-progress" and "in-review" labels are recognized because humans +// (and other tooling) commonly apply them. The adapter does NOT write them +// in v1 — see issue #40 for the write-side work. // // # Out of scope // -// - No webhook receiver, no polling goroutine, no fact projection into LCM -// (see issue #35 for the observer-loop work). +// - No Comment, no Transition (issue #40). +// - No webhook receiver, no polling goroutine, no fact projection into +// LCM (issue #35). // - No richer per-provider metadata on Issue (milestones, project boards, -// reactions); the port only carries fields all three v1 providers can fill. +// reactions); the port only carries fields all v1 providers can fill. package github diff --git a/backend/internal/adapters/tracker/github/tracker.go b/backend/internal/adapters/tracker/github/tracker.go index 62d8c8904..64e443892 100644 --- a/backend/internal/adapters/tracker/github/tracker.go +++ b/backend/internal/adapters/tracker/github/tracker.go @@ -8,7 +8,6 @@ import ( "fmt" "io" "net/http" - "net/url" "strconv" "strings" "time" @@ -21,13 +20,15 @@ const ( defaultBaseURL = "https://api.github.com" defaultUserAgent = "ao-agent-orchestrator/tracker-github" + // Status labels used by humans (and other tooling) on GitHub Issues. + // Get's reverse mapping recognizes them so an externally-labeled issue + // reports as in_progress / review. The adapter does NOT write these + // labels in v1 — see issue #40 for the write-side work. labelInProgress = "in-progress" labelInReview = "in-review" - stateOpenGH = "open" - stateClosedGH = "closed" - reasonComplete = "completed" - reasonNotPlan = "not_planned" + stateClosedGH = "closed" + reasonNotPlan = "not_planned" ) // Sentinel errors. Adapter-level callers should match on these via @@ -36,9 +37,7 @@ const ( var ( ErrNotFound = errors.New("github tracker: issue not found") ErrRateLimited = errors.New("github tracker: rate limited") - ErrEmptyBody = errors.New("github tracker: comment body is empty") ErrWrongProvider = errors.New("github tracker: id is not a github tracker id") - ErrUnknownState = errors.New("github tracker: unknown normalized state") ErrBadID = errors.New("github tracker: malformed native id") ) @@ -215,107 +214,6 @@ func mapStateFromGitHub(state, reason string, labels []string) domain.Normalized } } -// --------------------------------------------------------------------------- -// Comment -// --------------------------------------------------------------------------- - -func (t *Tracker) Comment(ctx context.Context, id domain.TrackerID, body string) error { - if strings.TrimSpace(body) == "" { - return ErrEmptyBody - } - owner, repo, number, err := t.parseID(id) - if err != nil { - return err - } - path := fmt.Sprintf("/repos/%s/%s/issues/%d/comments", owner, repo, number) - _, err = t.do(ctx, http.MethodPost, path, map[string]string{"body": body}) - return err -} - -// --------------------------------------------------------------------------- -// Transition -// --------------------------------------------------------------------------- - -// transitionPlan is the per-target-state list of mutations to apply. Every -// transition issues exactly one PATCH on the issue, optionally adds one -// status label, and removes any other status labels the issue may carry. -type transitionPlan struct { - patch map[string]any - addLabel string // "" means none - removeLabel []string -} - -func planForState(state domain.NormalizedIssueState) (transitionPlan, error) { - switch state { - case domain.IssueOpen: - return transitionPlan{ - patch: map[string]any{"state": stateOpenGH}, - removeLabel: []string{labelInProgress, labelInReview}, - }, nil - case domain.IssueInProgress: - return transitionPlan{ - patch: map[string]any{"state": stateOpenGH}, - addLabel: labelInProgress, - removeLabel: []string{labelInReview}, - }, nil - case domain.IssueInReview: - return transitionPlan{ - patch: map[string]any{"state": stateOpenGH}, - addLabel: labelInReview, - removeLabel: []string{labelInProgress}, - }, nil - case domain.IssueDone: - return transitionPlan{ - patch: map[string]any{"state": stateClosedGH, "state_reason": reasonComplete}, - removeLabel: []string{labelInProgress, labelInReview}, - }, nil - case domain.IssueCancelled: - return transitionPlan{ - patch: map[string]any{"state": stateClosedGH, "state_reason": reasonNotPlan}, - removeLabel: []string{labelInProgress, labelInReview}, - }, nil - default: - return transitionPlan{}, fmt.Errorf("%w: %q", ErrUnknownState, state) - } -} - -func (t *Tracker) Transition(ctx context.Context, id domain.TrackerID, state domain.NormalizedIssueState) error { - plan, err := planForState(state) - if err != nil { - return err - } - owner, repo, number, err := t.parseID(id) - if err != nil { - return err - } - issuePath := fmt.Sprintf("/repos/%s/%s/issues/%d", owner, repo, number) - - // 1. Patch state (+ state_reason for closed transitions). - if _, err := t.do(ctx, http.MethodPatch, issuePath, plan.patch); err != nil { - return err - } - // 2. Add the target status label (no-op when target is open/done/cancelled). - if plan.addLabel != "" { - body := map[string][]string{"labels": {plan.addLabel}} - if _, err := t.do(ctx, http.MethodPost, issuePath+"/labels", body); err != nil { - return err - } - } - // 3. Remove the other status labels. 404 from GitHub means "label is not - // on this issue", which is the success state we want — swallow it so - // the operation is idempotent. - for _, label := range plan.removeLabel { - labelPath := issuePath + "/labels/" + url.PathEscape(label) - if _, err := t.do(ctx, http.MethodDelete, labelPath, nil); err != nil { - if errors.Is(err, ErrNotFound) { - continue - } - return err - } - } - return nil -} - // --------------------------------------------------------------------------- // HTTP plumbing // --------------------------------------------------------------------------- diff --git a/backend/internal/adapters/tracker/github/tracker_test.go b/backend/internal/adapters/tracker/github/tracker_test.go index 44f5a6cd7..23424f023 100644 --- a/backend/internal/adapters/tracker/github/tracker_test.go +++ b/backend/internal/adapters/tracker/github/tracker_test.go @@ -8,7 +8,6 @@ import ( "net/http" "net/http/httptest" "reflect" - "sort" "strconv" "strings" "sync" @@ -322,219 +321,3 @@ func TestGet_CanonicalizesProviderOnOutput(t *testing.T) { t.Fatalf("issue.ID.Native = %q, want o/r#1", issue.ID.Native) } } - -func TestComment_HappyPath(t *testing.T) { - f := newFakeGH(t) - f.on("POST", "/repos/o/r/issues/1/comments", func(w http.ResponseWriter, r *http.Request) { - var got struct { - Body string `json:"body"` - } - if err := json.NewDecoder(r.Body).Decode(&got); err != nil { - t.Fatalf("decode: %v", err) - } - if got.Body != "hello world" { - t.Errorf("body = %q, want %q", got.Body, "hello world") - } - w.WriteHeader(http.StatusCreated) - _, _ = w.Write([]byte(`{"id":1}`)) - }) - tr := newTrackerForTest(t, f) - if err := tr.Comment(ctx(), domain.TrackerID{Provider: domain.TrackerProviderGitHub, Native: "o/r#1"}, "hello world"); err != nil { - t.Fatalf("Comment: %v", err) - } -} - -// TestComment_PreservesMarkdownBody locks in that we POST the body verbatim -// — no trimming, no escape-and-unescape round trip — so multi-line markdown -// notifications from the SM survive. -func TestComment_PreservesMarkdownBody(t *testing.T) { - f := newFakeGH(t) - body := "## status\n\n- step 1: done\n- step 2: **in progress**\n\n```go\nfmt.Println(\"hi\")\n```\n" - f.on("POST", "/repos/o/r/issues/1/comments", func(w http.ResponseWriter, r *http.Request) { - var got struct { - Body string `json:"body"` - } - if err := json.NewDecoder(r.Body).Decode(&got); err != nil { - t.Fatalf("decode: %v", err) - } - if got.Body != body { - t.Errorf("body = %q, want %q", got.Body, body) - } - w.WriteHeader(http.StatusCreated) - _, _ = w.Write([]byte(`{"id":1}`)) - }) - tr := newTrackerForTest(t, f) - if err := tr.Comment(ctx(), domain.TrackerID{Provider: domain.TrackerProviderGitHub, Native: "o/r#1"}, body); err != nil { - t.Fatalf("Comment: %v", err) - } -} - -func TestComment_RejectsEmptyBody(t *testing.T) { - f := newFakeGH(t) - tr := newTrackerForTest(t, f) - for _, body := range []string{"", " ", "\n\t"} { - err := tr.Comment(ctx(), domain.TrackerID{Provider: domain.TrackerProviderGitHub, Native: "o/r#1"}, body) - if !errors.Is(err, ErrEmptyBody) { - t.Fatalf("body %q: err = %v, want ErrEmptyBody", body, err) - } - } - if calls := f.calls(); len(calls) != 0 { - t.Fatalf("unexpected calls on empty body: %#v", calls) - } -} - -// transitionCall is the normalized record of one GH API call made by -// Transition. The tests compare a sorted slice of these against the expected -// call set so we don't depend on call ordering. -type transitionCall struct { - Method string - Path string - // for PATCH /issues/N — JSON keys we care about - State string - StateReason string - // for POST .../labels — labels added - AddLabels []string -} - -func TestTransition_MapsToCorrectGitHubCalls(t *testing.T) { - cases := []struct { - name string - state domain.NormalizedIssueState - want []transitionCall - }{ - { - name: "open clears status labels and reopens", - state: domain.IssueOpen, - want: []transitionCall{ - {Method: "PATCH", Path: "/repos/o/r/issues/1", State: "open"}, - {Method: "DELETE", Path: "/repos/o/r/issues/1/labels/in-progress"}, - {Method: "DELETE", Path: "/repos/o/r/issues/1/labels/in-review"}, - }, - }, - { - name: "in_progress adds in-progress label, removes in-review", - state: domain.IssueInProgress, - want: []transitionCall{ - {Method: "PATCH", Path: "/repos/o/r/issues/1", State: "open"}, - {Method: "POST", Path: "/repos/o/r/issues/1/labels", AddLabels: []string{"in-progress"}}, - {Method: "DELETE", Path: "/repos/o/r/issues/1/labels/in-review"}, - }, - }, - { - name: "review adds in-review label, removes in-progress", - state: domain.IssueInReview, - want: []transitionCall{ - {Method: "PATCH", Path: "/repos/o/r/issues/1", State: "open"}, - {Method: "POST", Path: "/repos/o/r/issues/1/labels", AddLabels: []string{"in-review"}}, - {Method: "DELETE", Path: "/repos/o/r/issues/1/labels/in-progress"}, - }, - }, - { - name: "done closes as completed and cleans status labels", - state: domain.IssueDone, - want: []transitionCall{ - {Method: "PATCH", Path: "/repos/o/r/issues/1", State: "closed", StateReason: "completed"}, - {Method: "DELETE", Path: "/repos/o/r/issues/1/labels/in-progress"}, - {Method: "DELETE", Path: "/repos/o/r/issues/1/labels/in-review"}, - }, - }, - { - name: "cancelled closes as not_planned and cleans status labels", - state: domain.IssueCancelled, - want: []transitionCall{ - {Method: "PATCH", Path: "/repos/o/r/issues/1", State: "closed", StateReason: "not_planned"}, - {Method: "DELETE", Path: "/repos/o/r/issues/1/labels/in-progress"}, - {Method: "DELETE", Path: "/repos/o/r/issues/1/labels/in-review"}, - }, - }, - } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - f := newFakeGH(t) - // PATCH endpoint returns an updated issue body - f.on("PATCH", "/repos/o/r/issues/1", func(w http.ResponseWriter, r *http.Request) { - _, _ = w.Write([]byte(`{"number":1,"state":"open"}`)) - }) - // label-add endpoint - f.on("POST", "/repos/o/r/issues/1/labels", func(w http.ResponseWriter, r *http.Request) { - _, _ = w.Write([]byte(`[]`)) - }) - // label-remove endpoints — return 404 sometimes to confirm we ignore it - f.on("DELETE", "/repos/o/r/issues/1/labels/in-progress", func(w http.ResponseWriter, r *http.Request) { - http.Error(w, `{"message":"Label does not exist"}`, http.StatusNotFound) - }) - f.on("DELETE", "/repos/o/r/issues/1/labels/in-review", func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusOK) - _, _ = w.Write([]byte(`[]`)) - }) - tr := newTrackerForTest(t, f) - if err := tr.Transition(ctx(), domain.TrackerID{Provider: domain.TrackerProviderGitHub, Native: "o/r#1"}, tc.state); err != nil { - t.Fatalf("Transition: %v", err) - } - got := normalizeCalls(t, f.calls()) - want := append([]transitionCall(nil), tc.want...) - sortCalls(got) - sortCalls(want) - if !reflect.DeepEqual(got, want) { - t.Fatalf("calls:\n got %#v\n want %#v", got, want) - } - }) - } -} - -func TestTransition_RejectsUnknownState(t *testing.T) { - f := newFakeGH(t) - tr := newTrackerForTest(t, f) - err := tr.Transition(ctx(), domain.TrackerID{Provider: domain.TrackerProviderGitHub, Native: "o/r#1"}, domain.NormalizedIssueState("frobnicated")) - if !errors.Is(err, ErrUnknownState) { - t.Fatalf("err = %v, want ErrUnknownState", err) - } - if calls := f.calls(); len(calls) != 0 { - t.Fatalf("unexpected calls: %#v", calls) - } -} - -// normalizeCalls converts the recordedReq slice into transitionCall records -// the test cases assert against, decoding the PATCH/label-add bodies. -func normalizeCalls(t *testing.T, reqs []recordedReq) []transitionCall { - t.Helper() - out := make([]transitionCall, 0, len(reqs)) - for _, r := range reqs { - tc := transitionCall{Method: r.Method, Path: r.Path} - switch { - case r.Method == "PATCH": - var body struct { - State string `json:"state"` - StateReason string `json:"state_reason"` - } - if r.Body != "" { - if err := json.Unmarshal([]byte(r.Body), &body); err != nil { - t.Fatalf("patch body: %v", err) - } - } - tc.State = body.State - tc.StateReason = body.StateReason - case r.Method == "POST" && strings.HasSuffix(r.Path, "/labels"): - var body struct { - Labels []string `json:"labels"` - } - if r.Body != "" { - if err := json.Unmarshal([]byte(r.Body), &body); err != nil { - t.Fatalf("labels body: %v", err) - } - } - tc.AddLabels = body.Labels - } - out = append(out, tc) - } - return out -} - -func sortCalls(s []transitionCall) { - sort.Slice(s, func(i, j int) bool { - if s[i].Method != s[j].Method { - return s[i].Method < s[j].Method - } - return s[i].Path < s[j].Path - }) -} diff --git a/backend/internal/ports/tracker.go b/backend/internal/ports/tracker.go index 642b1912a..c4b0240ea 100644 --- a/backend/internal/ports/tracker.go +++ b/backend/internal/ports/tracker.go @@ -7,19 +7,16 @@ import ( ) // Tracker is the outbound port for issue trackers (GitHub Issues, GitLab -// Issues, Linear). v1 is write-mostly: spawn-bootstrap reads with Get, the -// Session Manager posts status updates with Comment, and lifecycle -// transitions (start, hand-off-to-review, close) propagate with Transition. -// There is no observer loop yet; polling and ApplyTrackerFacts arrive with -// issue #35. +// Issues, Linear). v1 is read-only: Get returns a normalized snapshot used +// by spawn-bootstrap to hydrate the agent prompt. Mirroring agent lifecycle +// back onto the tracker (Comment, Transition) is deferred to issue #40, and +// the observer/polling loop is deferred to issue #35. // -// All three v1 providers share this interface. Provider differences (label -// vs state machine vs close reason) are absorbed inside each adapter via +// All v1 providers share this interface. Provider differences (label vs +// state machine vs close reason) are absorbed inside each adapter via // domain.NormalizedIssueState. Fields on domain.Issue exist only when every // provider can populate them; richer per-provider metadata belongs behind a // separate port. type Tracker interface { Get(ctx context.Context, id domain.TrackerID) (domain.Issue, error) - Comment(ctx context.Context, id domain.TrackerID, body string) error - Transition(ctx context.Context, id domain.TrackerID, state domain.NormalizedIssueState) error } From d6cd24583336c1d34418818ae2605df156a94a8b Mon Sep 17 00:00:00 2001 From: harshitsinghbhandari <24b4506@iitb.ac.in> Date: Sat, 30 May 2026 22:04:58 +0530 Subject: [PATCH 3/4] feat(tracker): add List + Preflight to the port and GitHub adapter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Brings the read-side surface up to parity with the legacy TS impl's useful read methods. Closes a gap flagged during scope review. Port additions: - List(ctx, repo, filter) ([]Issue, error) - Preflight(ctx) error Domain additions: TrackerRepo, ListStateFilter (open/closed/""=all), ListFilter (State, Labels, Assignee, Limit). GitHub adapter: - List hits GET /repos/{o}/{r}/issues with query-encoded filter. Defaults: state=all, per_page=30; per_page is capped at 100. PRs are filtered out client-side (GitHub conflates them with issues on that endpoint). - Preflight hits GET /user. Success is cached for the Tracker's lifetime via sync.Mutex + bool; failures are intentionally NOT cached so a transient startup glitch is recoverable. - New ErrAuthFailed sentinel. classifyError now maps 401, and 403 without rate-limit signals, to ErrAuthFailed instead of an opaque error — so Preflight callers can distinguish bad-token from other failures. Pagination beyond the first page is intentionally out of scope for v1 (see doc.go); the observer/polling work in #35 will own that. Tests: 43 pass (was 25). Adds Preflight cache + recovery, List query encoding, PR filtering, repo parser rejection, and ErrAuthFailed classification. Co-Authored-By: Claude Opus 4.7 --- .../internal/adapters/tracker/github/doc.go | 17 +- .../adapters/tracker/github/tracker.go | 185 +++++++++++++-- .../adapters/tracker/github/tracker_test.go | 217 ++++++++++++++++++ backend/internal/domain/tracker.go | 34 +++ backend/internal/ports/tracker.go | 17 +- 5 files changed, 446 insertions(+), 24 deletions(-) diff --git a/backend/internal/adapters/tracker/github/doc.go b/backend/internal/adapters/tracker/github/doc.go index f2114334a..f37c4c90f 100644 --- a/backend/internal/adapters/tracker/github/doc.go +++ b/backend/internal/adapters/tracker/github/doc.go @@ -1,8 +1,17 @@ // Package github implements the ports.Tracker outbound port for GitHub -// Issues. v1 is read-only: Get returns a normalized Issue snapshot that the -// Session Manager uses to hydrate the agent prompt during spawn-bootstrap. +// Issues. v1 is read-only: +// +// - Get returns a normalized snapshot of one issue (spawn-bootstrap +// reads it to hydrate the agent prompt). +// - List returns a filtered slice of issues in a repo (one page, no +// auto-pagination in v1; PRs are filtered out client-side because +// GitHub's /issues endpoint conflates them). +// - Preflight performs a single GET /user against GitHub to verify the +// token is accepted; success is cached for the lifetime of the +// Tracker, failures are not. +// // Writing back to the tracker (Comment, Transition) is deferred to issue -// #40; the observer/polling loop is deferred to issue #35. +// #40. The observer/polling loop is deferred to issue #35. // // # Reverse state mapping // @@ -24,6 +33,8 @@ // # Out of scope // // - No Comment, no Transition (issue #40). +// - No List pagination beyond a single page (callers requesting more than +// 100 results need to wait for the observer/polling work in issue #35). // - No webhook receiver, no polling goroutine, no fact projection into // LCM (issue #35). // - No richer per-provider metadata on Issue (milestones, project boards, diff --git a/backend/internal/adapters/tracker/github/tracker.go b/backend/internal/adapters/tracker/github/tracker.go index 64e443892..8176ebfdc 100644 --- a/backend/internal/adapters/tracker/github/tracker.go +++ b/backend/internal/adapters/tracker/github/tracker.go @@ -8,8 +8,10 @@ import ( "fmt" "io" "net/http" + "net/url" "strconv" "strings" + "sync" "time" "github.com/aoagents/agent-orchestrator/backend/internal/domain" @@ -29,6 +31,11 @@ const ( stateClosedGH = "closed" reasonNotPlan = "not_planned" + + // List pagination — GitHub's per_page maxes at 100. We default to 30 + // (matching the legacy gh CLI default) when the caller passes 0. + defaultListLimit = 30 + maxListLimit = 100 ) // Sentinel errors. Adapter-level callers should match on these via @@ -37,6 +44,7 @@ const ( var ( ErrNotFound = errors.New("github tracker: issue not found") ErrRateLimited = errors.New("github tracker: rate limited") + ErrAuthFailed = errors.New("github tracker: authentication failed") ErrWrongProvider = errors.New("github tracker: id is not a github tracker id") ErrBadID = errors.New("github tracker: malformed native id") ) @@ -75,14 +83,19 @@ type Options struct { // Tracker implements ports.Tracker against the GitHub REST API. // -// Construction performs a fail-fast token presence check (no network call — -// validating the token's authorization scope against GitHub requires a real -// request, and that is the first operation any caller will make anyway). +// Construction performs a fail-fast token presence check (no network call). +// The first Preflight call validates the token against GitHub itself; a +// successful preflight is cached for the lifetime of the Tracker so repeat +// calls are free, while failures are intentionally NOT cached so a +// transient startup glitch doesn't permanently brick the adapter. type Tracker struct { http *http.Client tokens TokenSource baseURL string userAgent string + + preflightMu sync.Mutex + preflightOK bool } // New returns a Tracker. It fails fast when no token can be obtained so @@ -122,15 +135,19 @@ var _ ports.Tracker = (*Tracker)(nil) // --------------------------------------------------------------------------- // ghIssue is the subset of fields we read off the REST issue payload. +// PullRequest is present (non-nil) iff GitHub considers this row a PR — +// the /repos/{o}/{r}/issues endpoint conflates the two. List uses it to +// filter PRs out client-side so the SM never sees a PR number as an issue. type ghIssue struct { - Number int `json:"number"` - Title string `json:"title"` - Body string `json:"body"` - State string `json:"state"` - StateReason string `json:"state_reason"` - HTMLURL string `json:"html_url"` - Labels []ghLabel `json:"labels"` - Assignees []ghUser `json:"assignees"` + Number int `json:"number"` + Title string `json:"title"` + Body string `json:"body"` + State string `json:"state"` + StateReason string `json:"state_reason"` + HTMLURL string `json:"html_url"` + Labels []ghLabel `json:"labels"` + Assignees []ghUser `json:"assignees"` + PullRequest *json.RawMessage `json:"pull_request,omitempty"` } type ghLabel struct { @@ -214,6 +231,113 @@ func mapStateFromGitHub(state, reason string, labels []string) domain.Normalized } } +// --------------------------------------------------------------------------- +// List +// --------------------------------------------------------------------------- + +// List returns issues for a repo, filtered by state/labels/assignee. PRs +// that GitHub's /issues endpoint conflates into the response are filtered +// out client-side. Pagination is intentionally NOT implemented in v1 — +// callers get one page bounded by ListFilter.Limit (default 30, max 100). +func (t *Tracker) List(ctx context.Context, repo domain.TrackerRepo, filter domain.ListFilter) ([]domain.Issue, error) { + if repo.Provider != domain.TrackerProviderGitHub { + return nil, fmt.Errorf("%w: provider=%q", ErrWrongProvider, repo.Provider) + } + owner, repoName, err := parseGitHubRepo(repo.Native) + if err != nil { + return nil, err + } + + q := url.Values{} + switch filter.State { + case domain.ListOpen: + q.Set("state", "open") + case domain.ListClosed: + q.Set("state", "closed") + default: + q.Set("state", "all") + } + if len(filter.Labels) > 0 { + q.Set("labels", strings.Join(filter.Labels, ",")) + } + if filter.Assignee != "" { + q.Set("assignee", filter.Assignee) + } + limit := filter.Limit + if limit <= 0 { + limit = defaultListLimit + } + if limit > maxListLimit { + limit = maxListLimit + } + q.Set("per_page", strconv.Itoa(limit)) + + path := fmt.Sprintf("/repos/%s/%s/issues?%s", owner, repoName, q.Encode()) + resp, err := t.do(ctx, http.MethodGet, path, nil) + if err != nil { + return nil, err + } + var raw []ghIssue + if err := json.Unmarshal(resp, &raw); err != nil { + return nil, fmt.Errorf("github tracker: decode list: %w", err) + } + out := make([]domain.Issue, 0, len(raw)) + for _, r := range raw { + if r.PullRequest != nil { + continue + } + labels := make([]string, 0, len(r.Labels)) + for _, l := range r.Labels { + labels = append(labels, l.Name) + } + assignees := make([]string, 0, len(r.Assignees)) + for _, a := range r.Assignees { + assignees = append(assignees, a.Login) + } + issue := domain.Issue{ + ID: domain.TrackerID{ + Provider: domain.TrackerProviderGitHub, + Native: fmt.Sprintf("%s/%s#%d", owner, repoName, r.Number), + }, + Title: r.Title, + Body: r.Body, + State: mapStateFromGitHub(r.State, r.StateReason, labels), + URL: r.HTMLURL, + Labels: labels, + Assignees: assignees, + } + if len(issue.Labels) == 0 { + issue.Labels = nil + } + if len(issue.Assignees) == 0 { + issue.Assignees = nil + } + out = append(out, issue) + } + return out, nil +} + +// --------------------------------------------------------------------------- +// Preflight +// --------------------------------------------------------------------------- + +// Preflight verifies the configured token is accepted by GitHub by making a +// single GET /user request. A successful check is cached for the lifetime +// of the Tracker; failures are never cached so a transient network glitch +// at startup is recoverable on a subsequent call. +func (t *Tracker) Preflight(ctx context.Context) error { + t.preflightMu.Lock() + defer t.preflightMu.Unlock() + if t.preflightOK { + return nil + } + if _, err := t.do(ctx, http.MethodGet, "/user", nil); err != nil { + return err + } + t.preflightOK = true + return nil +} + // --------------------------------------------------------------------------- // HTTP plumbing // --------------------------------------------------------------------------- @@ -262,16 +386,22 @@ func classifyError(resp *http.Response, body []byte) error { return fmt.Errorf("%w: %s", ErrNotFound, msg) case http.StatusTooManyRequests: return rateLimited(resp, msg) - case http.StatusForbidden, http.StatusUnauthorized: + case http.StatusUnauthorized: + // 401 is unambiguously an auth failure. GitHub never uses 401 for + // rate limiting; that's always 403 or 429. + return fmt.Errorf("%w: %s", ErrAuthFailed, msg) + case http.StatusForbidden: // GitHub returns 403 for primary rate-limit exhaustion, for - // secondary/abuse limits, and for genuine auth failures. Three - // signals disambiguate the rate-limit cases from auth: the primary - // limit sets X-RateLimit-Remaining=0; the secondary/abuse limit - // sets Retry-After (and often omits X-RateLimit-Remaining); and - // either case mentions "rate limit" / "abuse" in the body. + // secondary/abuse limits, and for genuine auth/permission failures. + // Disambiguate by signal: primary limit sets X-RateLimit-Remaining=0; + // secondary/abuse sets Retry-After (often without the Remaining + // header); either case mentions "rate limit" / "abuse" in the body. + // Everything else is an auth/permission failure (token missing the + // right scope, repo not visible to this token, etc). if isRateLimited(resp, msg) { return rateLimited(resp, msg) } + return fmt.Errorf("%w: %s", ErrAuthFailed, msg) } return fmt.Errorf("github tracker: %d %s", resp.StatusCode, msg) } @@ -354,3 +484,24 @@ func parseGitHubID(native string) (owner, repo string, number int, err error) { } return owner, repo, n, nil } + +// parseGitHubRepo accepts "owner/repo" and rejects anything containing +// additional slashes or a "#" segment. Used by List. +func parseGitHubRepo(native string) (owner, repo string, err error) { + if native == "" { + return "", "", fmt.Errorf("%w: empty repo", ErrBadID) + } + slash := strings.IndexByte(native, '/') + if slash < 0 { + return "", "", fmt.Errorf("%w: missing owner/repo separator", ErrBadID) + } + owner = native[:slash] + repo = native[slash+1:] + if owner == "" || repo == "" { + return "", "", fmt.Errorf("%w: empty owner or repo segment", ErrBadID) + } + if strings.ContainsAny(repo, "/#") { + return "", "", fmt.Errorf("%w: invalid repo segment %q", ErrBadID, repo) + } + return owner, repo, nil +} diff --git a/backend/internal/adapters/tracker/github/tracker_test.go b/backend/internal/adapters/tracker/github/tracker_test.go index 23424f023..1c3d7d545 100644 --- a/backend/internal/adapters/tracker/github/tracker_test.go +++ b/backend/internal/adapters/tracker/github/tracker_test.go @@ -321,3 +321,220 @@ func TestGet_CanonicalizesProviderOnOutput(t *testing.T) { t.Fatalf("issue.ID.Native = %q, want o/r#1", issue.ID.Native) } } + +// TestGet_AuthFailed locks in that a 401 (and 403 without rate-limit +// signals) maps to the typed ErrAuthFailed, so callers — especially +// Preflight — can distinguish bad-token from other failures. +func TestGet_AuthFailed(t *testing.T) { + f := newFakeGH(t) + f.on("GET", "/repos/o/r/issues/1", func(w http.ResponseWriter, r *http.Request) { + http.Error(w, `{"message":"Bad credentials"}`, http.StatusUnauthorized) + }) + tr := newTrackerForTest(t, f) + _, err := tr.Get(ctx(), domain.TrackerID{Provider: domain.TrackerProviderGitHub, Native: "o/r#1"}) + if !errors.Is(err, ErrAuthFailed) { + t.Fatalf("err = %v, want ErrAuthFailed", err) + } +} + +// --------------------------------------------------------------------------- +// Preflight +// --------------------------------------------------------------------------- + +func TestPreflight_HappyPath(t *testing.T) { + f := newFakeGH(t) + f.on("GET", "/user", func(w http.ResponseWriter, r *http.Request) { + if got := r.Header.Get("Authorization"); got != "Bearer tkn-test" { + t.Errorf("Authorization = %q", got) + } + _, _ = w.Write([]byte(`{"login":"octocat","id":1}`)) + }) + tr := newTrackerForTest(t, f) + if err := tr.Preflight(ctx()); err != nil { + t.Fatalf("Preflight: %v", err) + } +} + +func TestPreflight_InvalidToken(t *testing.T) { + f := newFakeGH(t) + f.on("GET", "/user", func(w http.ResponseWriter, r *http.Request) { + http.Error(w, `{"message":"Bad credentials"}`, http.StatusUnauthorized) + }) + tr := newTrackerForTest(t, f) + err := tr.Preflight(ctx()) + if !errors.Is(err, ErrAuthFailed) { + t.Fatalf("err = %v, want ErrAuthFailed", err) + } +} + +// TestPreflight_CachesSuccess pins that a successful check is cached so the +// daemon doesn't burn a GET /user on every component start that wants to +// confirm tracker auth. +func TestPreflight_CachesSuccess(t *testing.T) { + f := newFakeGH(t) + f.on("GET", "/user", func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(`{"login":"octocat","id":1}`)) + }) + tr := newTrackerForTest(t, f) + for i := 0; i < 5; i++ { + if err := tr.Preflight(ctx()); err != nil { + t.Fatalf("Preflight #%d: %v", i, err) + } + } + if got := len(f.calls()); got != 1 { + t.Fatalf("HTTP calls = %d, want 1 (success should be cached)", got) + } +} + +// TestPreflight_RetriesAfterFailure pins the recovery property: failures +// must NOT be cached, otherwise a transient network glitch at startup would +// permanently brick the tracker for the lifetime of the daemon. +func TestPreflight_RetriesAfterFailure(t *testing.T) { + f := newFakeGH(t) + var calls int + f.on("GET", "/user", func(w http.ResponseWriter, r *http.Request) { + calls++ + if calls == 1 { + http.Error(w, `{"message":"server exploded"}`, http.StatusInternalServerError) + return + } + _, _ = w.Write([]byte(`{"login":"octocat","id":1}`)) + }) + tr := newTrackerForTest(t, f) + if err := tr.Preflight(ctx()); err == nil { + t.Fatalf("first Preflight expected to fail") + } + if err := tr.Preflight(ctx()); err != nil { + t.Fatalf("second Preflight: %v", err) + } + if got := len(f.calls()); got != 2 { + t.Fatalf("HTTP calls = %d, want 2 (first fail not cached)", got) + } +} + +// --------------------------------------------------------------------------- +// List +// --------------------------------------------------------------------------- + +func TestList_HappyPathAndDefaults(t *testing.T) { + f := newFakeGH(t) + f.on("GET", "/repos/o/r/issues", func(w http.ResponseWriter, r *http.Request) { + q := r.URL.Query() + if got := q.Get("state"); got != "all" { + t.Errorf("state = %q, want all (default)", got) + } + if got := q.Get("per_page"); got != "30" { + t.Errorf("per_page = %q, want 30 (default)", got) + } + _, _ = w.Write([]byte(`[ + {"number":1,"title":"first","body":"b1","state":"open","html_url":"https://github.com/o/r/issues/1","labels":[{"name":"bug"}],"assignees":[]}, + {"number":2,"title":"second","body":"b2","state":"closed","state_reason":"completed","html_url":"https://github.com/o/r/issues/2","labels":[],"assignees":[{"login":"alice"}]} + ]`)) + }) + tr := newTrackerForTest(t, f) + issues, err := tr.List(ctx(), domain.TrackerRepo{Provider: domain.TrackerProviderGitHub, Native: "o/r"}, domain.ListFilter{}) + if err != nil { + t.Fatalf("List: %v", err) + } + if len(issues) != 2 { + t.Fatalf("len = %d, want 2", len(issues)) + } + if issues[0].ID.Native != "o/r#1" || issues[0].State != domain.IssueOpen || issues[0].Title != "first" { + t.Fatalf("issues[0] = %#v", issues[0]) + } + if issues[1].ID.Native != "o/r#2" || issues[1].State != domain.IssueDone || len(issues[1].Assignees) != 1 || issues[1].Assignees[0] != "alice" { + t.Fatalf("issues[1] = %#v", issues[1]) + } +} + +func TestList_FiltersOutPullRequests(t *testing.T) { + f := newFakeGH(t) + f.on("GET", "/repos/o/r/issues", func(w http.ResponseWriter, r *http.Request) { + // GitHub's issues endpoint returns PRs too. We must filter them out + // so the LCM never tries to spawn an agent against a PR number. + _, _ = w.Write([]byte(`[ + {"number":10,"title":"real issue","state":"open","html_url":"https://github.com/o/r/issues/10"}, + {"number":11,"title":"a PR","state":"open","html_url":"https://github.com/o/r/pull/11","pull_request":{"url":"https://api.github.com/repos/o/r/pulls/11"}}, + {"number":12,"title":"another issue","state":"open","html_url":"https://github.com/o/r/issues/12"} + ]`)) + }) + tr := newTrackerForTest(t, f) + issues, err := tr.List(ctx(), domain.TrackerRepo{Provider: domain.TrackerProviderGitHub, Native: "o/r"}, domain.ListFilter{}) + if err != nil { + t.Fatalf("List: %v", err) + } + if len(issues) != 2 { + t.Fatalf("len = %d, want 2 (PR must be filtered out)", len(issues)) + } + if issues[0].ID.Native != "o/r#10" || issues[1].ID.Native != "o/r#12" { + t.Fatalf("kept wrong items: %#v", issues) + } +} + +func TestList_QueryEncoding(t *testing.T) { + cases := []struct { + name string + filter domain.ListFilter + wantQ map[string]string + }{ + { + name: "open + labels + assignee + limit", + filter: domain.ListFilter{State: domain.ListOpen, Labels: []string{"bug", "help wanted"}, Assignee: "alice", Limit: 50}, + wantQ: map[string]string{"state": "open", "labels": "bug,help wanted", "assignee": "alice", "per_page": "50"}, + }, + { + name: "closed only", + filter: domain.ListFilter{State: domain.ListClosed}, + wantQ: map[string]string{"state": "closed", "per_page": "30"}, + }, + { + name: "limit capped at 100", + filter: domain.ListFilter{Limit: 9999}, + wantQ: map[string]string{"state": "all", "per_page": "100"}, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + f := newFakeGH(t) + f.on("GET", "/repos/o/r/issues", func(w http.ResponseWriter, r *http.Request) { + got := r.URL.Query() + for k, want := range tc.wantQ { + if g := got.Get(k); g != want { + t.Errorf("query[%q] = %q, want %q", k, g, want) + } + } + _, _ = w.Write([]byte(`[]`)) + }) + tr := newTrackerForTest(t, f) + if _, err := tr.List(ctx(), domain.TrackerRepo{Provider: domain.TrackerProviderGitHub, Native: "o/r"}, tc.filter); err != nil { + t.Fatalf("List: %v", err) + } + }) + } +} + +func TestList_RejectsWrongProvider(t *testing.T) { + f := newFakeGH(t) + tr := newTrackerForTest(t, f) + _, err := tr.List(ctx(), domain.TrackerRepo{Provider: domain.TrackerProviderGitLab, Native: "g/p"}, domain.ListFilter{}) + if !errors.Is(err, ErrWrongProvider) { + t.Fatalf("err = %v, want ErrWrongProvider", err) + } + if calls := f.calls(); len(calls) != 0 { + t.Fatalf("unexpected HTTP calls: %#v", calls) + } +} + +func TestList_RejectsBadRepo(t *testing.T) { + cases := []string{"", "noseparator", "/repo", "owner/", "a/b/c"} + for _, native := range cases { + t.Run(native, func(t *testing.T) { + f := newFakeGH(t) + tr := newTrackerForTest(t, f) + _, err := tr.List(ctx(), domain.TrackerRepo{Provider: domain.TrackerProviderGitHub, Native: native}, domain.ListFilter{}) + if !errors.Is(err, ErrBadID) { + t.Fatalf("native=%q: err = %v, want ErrBadID", native, err) + } + }) + } +} diff --git a/backend/internal/domain/tracker.go b/backend/internal/domain/tracker.go index 202c6bb17..27137752f 100644 --- a/backend/internal/domain/tracker.go +++ b/backend/internal/domain/tracker.go @@ -47,3 +47,37 @@ type Issue struct { Labels []string `json:"labels,omitempty"` Assignees []string `json:"assignees,omitempty"` } + +// TrackerRepo identifies a repository (or its provider-equivalent) for +// cross-issue queries like Tracker.List. Native is the provider's canonical +// owner/project form: "owner/repo" for GitHub, "group/project" for GitLab. +// Linear has no native repo concept; adapters may use a team or workspace +// identifier in Native when this port reaches Linear. +type TrackerRepo struct { + Provider TrackerProvider `json:"provider"` + Native string `json:"native"` +} + +// ListStateFilter narrows Tracker.List results by the provider's coarse +// state (open vs closed). It is intentionally NOT the 5-value normalized +// enum — finer filtering (e.g. "only in-review issues") goes through the +// Labels field of ListFilter. +type ListStateFilter string + +const ( + // ListAll is the zero value and returns issues in any state. + ListAll ListStateFilter = "" + ListOpen ListStateFilter = "open" + ListClosed ListStateFilter = "closed" +) + +// ListFilter is the query the Session Manager passes to Tracker.List. +// Empty / zero values mean "no filter on this dimension". Limit is the +// requested page size; the adapter applies its own default when zero and +// caps at the provider's per-page maximum. +type ListFilter struct { + State ListStateFilter `json:"state,omitempty"` + Labels []string `json:"labels,omitempty"` + Assignee string `json:"assignee,omitempty"` + Limit int `json:"limit,omitempty"` +} diff --git a/backend/internal/ports/tracker.go b/backend/internal/ports/tracker.go index c4b0240ea..d9fac9104 100644 --- a/backend/internal/ports/tracker.go +++ b/backend/internal/ports/tracker.go @@ -7,10 +7,17 @@ import ( ) // Tracker is the outbound port for issue trackers (GitHub Issues, GitLab -// Issues, Linear). v1 is read-only: Get returns a normalized snapshot used -// by spawn-bootstrap to hydrate the agent prompt. Mirroring agent lifecycle -// back onto the tracker (Comment, Transition) is deferred to issue #40, and -// the observer/polling loop is deferred to issue #35. +// Issues, Linear). v1 is read-only: +// +// - Get returns a normalized snapshot of one issue, used by spawn-bootstrap +// to hydrate the agent prompt. +// - List returns a filtered slice of issues in a repo, used when the SM +// needs to enumerate work (e.g. backlog view, status sweeps). +// - Preflight verifies the configured credential is actually valid against +// the provider so daemons fail fast at startup, not at first request. +// +// Mirroring agent lifecycle back onto the tracker (Comment, Transition) is +// deferred to issue #40. The observer / polling loop is deferred to #35. // // All v1 providers share this interface. Provider differences (label vs // state machine vs close reason) are absorbed inside each adapter via @@ -19,4 +26,6 @@ import ( // separate port. type Tracker interface { Get(ctx context.Context, id domain.TrackerID) (domain.Issue, error) + List(ctx context.Context, repo domain.TrackerRepo, filter domain.ListFilter) ([]domain.Issue, error) + Preflight(ctx context.Context) error } From a0020e06be556c1173e1ce26ad93b170696ec2d2 Mon Sep 17 00:00:00 2001 From: harshitsinghbhandari <24b4506@iitb.ac.in> Date: Sat, 30 May 2026 22:32:45 +0530 Subject: [PATCH 4/4] refactor(tracker): address code-review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six fixes from the second code review pass — none load-bearing but all improve the contract honesty or prevent future churn. Tests pass with -race (49 in package, 299 across the backend). 1. Preflight: atomic.Bool fast-path before the mutex so cached-success calls don't contend on the lock. Double-checked locking on the mutex side so concurrent first-callers still serialize the single GET /user request. 2. Preflight godoc: tighten to say it verifies token validity, not repo authorization — Get/List against a specific repo may still return ErrAuthFailed after a green Preflight if the token lacks the scope or the repo isn't visible. 3. domain.ListFilter.Limit godoc: explicitly note that exceeding the provider per-page max is SILENTLY capped (no error, no truncation signal) and that auto-pagination is deferred to #35. 4. Extract issueFromGH helper. Get and List were duplicating the identical ghIssue -> domain.Issue projection; consolidating now prevents a 3-way merge when #40 (Comment/Transition) lands. 5. parseGitHubRepo: reject whitespace and # in both owner and repo segments. Leading dots stay legal (the "owner/.github" repo convention). New test cases cover the rejections plus a positive guard for the leading-dot case. 6. fakeGH test helper: lock the handlers map on both read and write, so future tests registering handlers from goroutines won't trip -race. Co-Authored-By: Claude Opus 4.7 --- .../adapters/tracker/github/tracker.go | 89 ++++++++++--------- .../adapters/tracker/github/tracker_test.go | 24 ++++- backend/internal/domain/tracker.go | 11 ++- 3 files changed, 77 insertions(+), 47 deletions(-) diff --git a/backend/internal/adapters/tracker/github/tracker.go b/backend/internal/adapters/tracker/github/tracker.go index 8176ebfdc..bf6ffcbfd 100644 --- a/backend/internal/adapters/tracker/github/tracker.go +++ b/backend/internal/adapters/tracker/github/tracker.go @@ -12,6 +12,7 @@ import ( "strconv" "strings" "sync" + "sync/atomic" "time" "github.com/aoagents/agent-orchestrator/backend/internal/domain" @@ -94,8 +95,12 @@ type Tracker struct { baseURL string userAgent string + // preflightOK is the fast-path: once a Preflight succeeds, every + // subsequent call short-circuits via atomic.Load without touching the + // mutex. preflightMu serializes the one-time network call so concurrent + // first-callers don't all fire GET /user against GitHub. + preflightOK atomic.Bool preflightMu sync.Mutex - preflightOK bool } // New returns a Tracker. It fails fast when no token can be obtained so @@ -173,6 +178,15 @@ func (t *Tracker) Get(ctx context.Context, id domain.TrackerID) (domain.Issue, e if err := json.Unmarshal(resp, &raw); err != nil { return domain.Issue{}, fmt.Errorf("github tracker: decode issue: %w", err) } + return issueFromGH(owner, repo, raw), nil +} + +// issueFromGH projects a raw GitHub issue payload into the normalized +// domain.Issue. owner and repo are passed in because the TrackerID.Native +// shape is "owner/repo#N" and we want the returned ID to round-trip +// through the same adapter even if the original caller used a zero +// Provider. +func issueFromGH(owner, repo string, raw ghIssue) domain.Issue { labels := make([]string, 0, len(raw.Labels)) for _, l := range raw.Labels { labels = append(labels, l.Name) @@ -182,9 +196,10 @@ func (t *Tracker) Get(ctx context.Context, id domain.TrackerID) (domain.Issue, e assignees = append(assignees, a.Login) } out := domain.Issue{ - // Canonicalize Provider so the returned Issue always re-routes back - // to this adapter, even if the caller built id with a zero Provider. - ID: domain.TrackerID{Provider: domain.TrackerProviderGitHub, Native: id.Native}, + ID: domain.TrackerID{ + Provider: domain.TrackerProviderGitHub, + Native: fmt.Sprintf("%s/%s#%d", owner, repo, raw.Number), + }, Title: raw.Title, Body: raw.Body, State: mapStateFromGitHub(raw.State, raw.StateReason, labels), @@ -198,7 +213,7 @@ func (t *Tracker) Get(ctx context.Context, id domain.TrackerID) (domain.Issue, e if len(out.Assignees) == 0 { out.Assignees = nil } - return out, nil + return out } // mapStateFromGitHub projects GitHub's open/closed + state_reason + labels @@ -286,33 +301,7 @@ func (t *Tracker) List(ctx context.Context, repo domain.TrackerRepo, filter doma if r.PullRequest != nil { continue } - labels := make([]string, 0, len(r.Labels)) - for _, l := range r.Labels { - labels = append(labels, l.Name) - } - assignees := make([]string, 0, len(r.Assignees)) - for _, a := range r.Assignees { - assignees = append(assignees, a.Login) - } - issue := domain.Issue{ - ID: domain.TrackerID{ - Provider: domain.TrackerProviderGitHub, - Native: fmt.Sprintf("%s/%s#%d", owner, repoName, r.Number), - }, - Title: r.Title, - Body: r.Body, - State: mapStateFromGitHub(r.State, r.StateReason, labels), - URL: r.HTMLURL, - Labels: labels, - Assignees: assignees, - } - if len(issue.Labels) == 0 { - issue.Labels = nil - } - if len(issue.Assignees) == 0 { - issue.Assignees = nil - } - out = append(out, issue) + out = append(out, issueFromGH(owner, repoName, r)) } return out, nil } @@ -321,20 +310,34 @@ func (t *Tracker) List(ctx context.Context, repo domain.TrackerRepo, filter doma // Preflight // --------------------------------------------------------------------------- -// Preflight verifies the configured token is accepted by GitHub by making a -// single GET /user request. A successful check is cached for the lifetime -// of the Tracker; failures are never cached so a transient network glitch -// at startup is recoverable on a subsequent call. +// Preflight verifies the configured token is currently accepted by GitHub +// (one GET /user). It does NOT prove the token has the repo scope or +// visibility needed for any specific Get/List call — those may still fail +// with ErrAuthFailed even after a successful Preflight. The guarantee is +// "token exists and is valid against GitHub's identity endpoint", not +// "token can do everything the SM will ask of it." Per-repo authorization +// is detected lazily at the first Get/List against that repo. +// +// Successful checks are cached for the lifetime of the Tracker via a +// double-checked atomic+mutex pattern: the hot path is one atomic.Load +// with no contention; concurrent first-callers serialize on the mutex so +// only one GET /user is in flight. Failures are intentionally NOT cached +// so a transient startup glitch is recoverable on a subsequent call. func (t *Tracker) Preflight(ctx context.Context) error { + if t.preflightOK.Load() { + return nil + } t.preflightMu.Lock() defer t.preflightMu.Unlock() - if t.preflightOK { + // Re-check after acquiring the lock — another goroutine may have raced + // us through the network call and stored success while we were waiting. + if t.preflightOK.Load() { return nil } if _, err := t.do(ctx, http.MethodGet, "/user", nil); err != nil { return err } - t.preflightOK = true + t.preflightOK.Store(true) return nil } @@ -485,8 +488,9 @@ func parseGitHubID(native string) (owner, repo string, number int, err error) { return owner, repo, n, nil } -// parseGitHubRepo accepts "owner/repo" and rejects anything containing -// additional slashes or a "#" segment. Used by List. +// parseGitHubRepo accepts "owner/repo" and rejects empty segments, +// embedded slashes, "#", and whitespace. Leading dots are kept legal — +// "owner/.github" is a real GitHub convention for repo-level config repos. func parseGitHubRepo(native string) (owner, repo string, err error) { if native == "" { return "", "", fmt.Errorf("%w: empty repo", ErrBadID) @@ -500,7 +504,10 @@ func parseGitHubRepo(native string) (owner, repo string, err error) { if owner == "" || repo == "" { return "", "", fmt.Errorf("%w: empty owner or repo segment", ErrBadID) } - if strings.ContainsAny(repo, "/#") { + if strings.ContainsAny(owner, "/# \t\n\r") { + return "", "", fmt.Errorf("%w: invalid owner segment %q", ErrBadID, owner) + } + if strings.ContainsAny(repo, "/# \t\n\r") { return "", "", fmt.Errorf("%w: invalid repo segment %q", ErrBadID, repo) } return owner, repo, nil diff --git a/backend/internal/adapters/tracker/github/tracker_test.go b/backend/internal/adapters/tracker/github/tracker_test.go index 1c3d7d545..a61a68999 100644 --- a/backend/internal/adapters/tracker/github/tracker_test.go +++ b/backend/internal/adapters/tracker/github/tracker_test.go @@ -45,16 +45,18 @@ func newFakeGH(t *testing.T) *fakeGH { } func (f *fakeGH) on(method, path string, h http.HandlerFunc) { + f.mu.Lock() + defer f.mu.Unlock() f.handlers[method+" "+path] = h } func (f *fakeGH) serve(w http.ResponseWriter, r *http.Request) { body, _ := io.ReadAll(r.Body) + key := r.Method + " " + r.URL.Path f.mu.Lock() f.requests = append(f.requests, recordedReq{Method: r.Method, Path: r.URL.Path, Body: string(body)}) - f.mu.Unlock() - key := r.Method + " " + r.URL.Path h, ok := f.handlers[key] + f.mu.Unlock() if !ok { f.t.Errorf("unexpected request: %s", key) http.Error(w, "no handler", http.StatusNotImplemented) @@ -526,7 +528,23 @@ func TestList_RejectsWrongProvider(t *testing.T) { } func TestList_RejectsBadRepo(t *testing.T) { - cases := []string{"", "noseparator", "/repo", "owner/", "a/b/c"} + cases := []string{ + "", // empty + "noseparator", // missing / + "/repo", // empty owner + "owner/", // empty repo + "a/b/c", // extra slash + " owner/repo", // leading whitespace in owner + "owner/repo ", // trailing whitespace in repo + "own er/repo", // embedded space in owner + "owner/re#po", // embedded # in repo + "\towner/repo", // tab in owner + "owner/repo\n", // newline in repo + } + // Sanity: a benign leading-dot repo (".github" convention) must pass. + if _, _, err := parseGitHubRepo("octocat/.github"); err != nil { + t.Fatalf("leading-dot repo rejected unexpectedly: %v", err) + } for _, native := range cases { t.Run(native, func(t *testing.T) { f := newFakeGH(t) diff --git a/backend/internal/domain/tracker.go b/backend/internal/domain/tracker.go index 27137752f..8fe0ed3b7 100644 --- a/backend/internal/domain/tracker.go +++ b/backend/internal/domain/tracker.go @@ -72,9 +72,14 @@ const ( ) // ListFilter is the query the Session Manager passes to Tracker.List. -// Empty / zero values mean "no filter on this dimension". Limit is the -// requested page size; the adapter applies its own default when zero and -// caps at the provider's per-page maximum. +// Empty / zero values mean "no filter on this dimension". +// +// Limit is the requested page size. The adapter applies its own default +// when zero and SILENTLY CAPS at the provider's per-page maximum — a +// caller asking for more than the cap gets exactly cap items back with no +// error and no indication of truncation. v1 has no auto-pagination; +// callers needing more results need to wait for the observer/polling work +// in issue #35. type ListFilter struct { State ListStateFilter `json:"state,omitempty"` Labels []string `json:"labels,omitempty"`