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] 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"`