From c2cf8b1dbcfb8a988d58d93c229b72553c1dfd52 Mon Sep 17 00:00:00 2001 From: harshitsinghbhandari <24b4506@iitb.ac.in> Date: Sun, 31 May 2026 00:07:58 +0530 Subject: [PATCH 1/2] feat(tracker): zero-config GitHub auth via gh auth token fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The merged adapter required an env var (AO_GITHUB_TOKEN or GITHUB_TOKEN) even when the user already had `gh auth login` set up. That's friction for the common local-dev case. EnvTokenSource now falls through to `gh auth token` after the configured env vars and GITHUB_TOKEN. If gh isn't installed or isn't logged in, the exec error is swallowed and ErrNoToken is returned — callers see one uniform "configure a token" signal regardless of why no token was found. The gh shell-out is injectable via a new GH func field for testing; the default real-exec is used when nil. The New() construction-time probe is also bounded by a 5s deadline so a hung gh fallback (broken PATH, etc.) cannot stall daemon startup. Adds 5 table-driven tests covering: configured env wins, GITHUB_TOKEN fallback, gh fallback path, gh error swallowed to ErrNoToken, and gh empty stdout falls through. go test -race + go vet + gofmt clean. Full backend suite green (305 pass). Same pattern is being added to the GitLab adapter (aa-28 in flight). No equivalent CLI for Linear (aa-29) — that one stays env-var only. Co-Authored-By: Claude Opus 4.7 --- .../internal/adapters/tracker/github/auth.go | 45 +++++++-- .../internal/adapters/tracker/github/doc.go | 9 ++ .../adapters/tracker/github/tracker.go | 8 +- .../adapters/tracker/github/tracker_test.go | 97 +++++++++++++++++++ 4 files changed, 151 insertions(+), 8 deletions(-) diff --git a/backend/internal/adapters/tracker/github/auth.go b/backend/internal/adapters/tracker/github/auth.go index 9aa810df..b39b2b62 100644 --- a/backend/internal/adapters/tracker/github/auth.go +++ b/backend/internal/adapters/tracker/github/auth.go @@ -4,6 +4,7 @@ import ( "context" "errors" "os" + "os/exec" "strings" ) @@ -30,16 +31,27 @@ func (s StaticTokenSource) Token(context.Context) (string, error) { 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. +// EnvTokenSource resolves a token from the user's environment with zero +// configuration on a stock developer machine. Lookup order: +// +// 1. Each name in EnvVars (project-configured first, e.g. AO_GITHUB_TOKEN). +// 2. The well-known GITHUB_TOKEN env var. +// 3. The `gh` CLI's auth state, via `gh auth token`. If the user has +// already run `gh auth login`, this just works — no env var required. +// +// If step 3 errors (gh not installed, not authenticated, exec failure) the +// error is swallowed and we fall through to ErrNoToken so the caller sees +// the same "configure a token" signal regardless of why no token was found. +// +// GH is the function invoked in step 3. Production code leaves it nil and +// the default `gh auth token` exec is used. Tests inject a fake to avoid +// shelling out to a real gh binary. type EnvTokenSource struct { EnvVars []string + GH func(ctx context.Context) (string, error) } -func (s EnvTokenSource) Token(context.Context) (string, error) { +func (s EnvTokenSource) Token(ctx context.Context) (string, error) { for _, name := range s.EnvVars { if v := strings.TrimSpace(os.Getenv(name)); v != "" { return v, nil @@ -48,5 +60,26 @@ func (s EnvTokenSource) Token(context.Context) (string, error) { if v := strings.TrimSpace(os.Getenv("GITHUB_TOKEN")); v != "" { return v, nil } + gh := s.GH + if gh == nil { + gh = ghAuthToken + } + if v, err := gh(ctx); err == nil { + if v = strings.TrimSpace(v); v != "" { + return v, nil + } + } return "", ErrNoToken } + +// ghAuthToken shells out to the `gh` CLI and asks it for the user's +// currently logged-in token. Returns an error if gh is not installed or +// the user is not authenticated; the EnvTokenSource fallback chain +// swallows that error and reports ErrNoToken instead. +func ghAuthToken(ctx context.Context) (string, error) { + out, err := exec.CommandContext(ctx, "gh", "auth", "token").Output() + if err != nil { + return "", err + } + return strings.TrimSpace(string(out)), nil +} diff --git a/backend/internal/adapters/tracker/github/doc.go b/backend/internal/adapters/tracker/github/doc.go index f37c4c90..1e060c53 100644 --- a/backend/internal/adapters/tracker/github/doc.go +++ b/backend/internal/adapters/tracker/github/doc.go @@ -13,6 +13,15 @@ // Writing back to the tracker (Comment, Transition) is deferred to issue // #40. The observer/polling loop is deferred to issue #35. // +// # Zero-config auth +// +// EnvTokenSource looks for a token in this order: configured env vars, +// GITHUB_TOKEN, then `gh auth token`. If the user has already run +// `gh auth login`, the adapter just works — no env var setup required. +// gh-CLI errors (binary missing, not authenticated) are intentionally +// swallowed and reported as ErrNoToken so the caller sees one uniform +// "configure a token" signal. +// // # Reverse state mapping // // GitHub Issues only have two native states (open, closed) plus a diff --git a/backend/internal/adapters/tracker/github/tracker.go b/backend/internal/adapters/tracker/github/tracker.go index bf6ffcbf..5ebf0be5 100644 --- a/backend/internal/adapters/tracker/github/tracker.go +++ b/backend/internal/adapters/tracker/github/tracker.go @@ -104,13 +104,17 @@ type Tracker struct { } // New returns a Tracker. It fails fast when no token can be obtained so -// daemons crash at startup rather than at first issue lookup. +// daemons crash at startup rather than at first issue lookup. The token +// probe is bounded by a short deadline so a hung `gh auth token` fallback +// (broken shell, unhealthy PATH) cannot stall daemon startup. func New(opts Options) (*Tracker, error) { src := opts.Token if src == nil { return nil, ErrNoToken } - if _, err := src.Token(context.Background()); err != nil { + bootCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + if _, err := src.Token(bootCtx); err != nil { return nil, err } t := &Tracker{ diff --git a/backend/internal/adapters/tracker/github/tracker_test.go b/backend/internal/adapters/tracker/github/tracker_test.go index a61a6899..d779e2b5 100644 --- a/backend/internal/adapters/tracker/github/tracker_test.go +++ b/backend/internal/adapters/tracker/github/tracker_test.go @@ -7,6 +7,7 @@ import ( "io" "net/http" "net/http/httptest" + "os" "reflect" "strconv" "strings" @@ -101,6 +102,102 @@ func TestNewRejectsMissingToken(t *testing.T) { } } +// TestEnvTokenSource covers the auth lookup chain that gives us zero-config +// behavior on a stock dev machine: configured env vars beat GITHUB_TOKEN +// beats `gh auth token`. All three paths must work, AND a failing gh +// fallback must surface as ErrNoToken — never as an exec error — so the +// caller sees one uniform "configure a token" signal. +func TestEnvTokenSource(t *testing.T) { + // Save and restore env vars the chain reads. + saved := map[string]string{} + for _, k := range []string{"GITHUB_TOKEN", "AO_GITHUB_TOKEN"} { + saved[k] = os.Getenv(k) + _ = os.Unsetenv(k) + } + t.Cleanup(func() { + for k, v := range saved { + if v == "" { + _ = os.Unsetenv(k) + } else { + _ = os.Setenv(k, v) + } + } + }) + + t.Run("configured env var wins over everything", func(t *testing.T) { + t.Setenv("AO_GITHUB_TOKEN", "from-configured") + t.Setenv("GITHUB_TOKEN", "from-github-token") + ghCalled := false + src := EnvTokenSource{ + EnvVars: []string{"AO_GITHUB_TOKEN"}, + GH: func(context.Context) (string, error) { ghCalled = true; return "from-gh", nil }, + } + got, err := src.Token(context.Background()) + if err != nil || got != "from-configured" { + t.Fatalf("got=%q err=%v", got, err) + } + if ghCalled { + t.Fatal("GH fallback called despite env var being set") + } + }) + + t.Run("GITHUB_TOKEN used when configured env empty", func(t *testing.T) { + _ = os.Unsetenv("AO_GITHUB_TOKEN") + t.Setenv("GITHUB_TOKEN", "from-github-token") + ghCalled := false + src := EnvTokenSource{ + EnvVars: []string{"AO_GITHUB_TOKEN"}, + GH: func(context.Context) (string, error) { ghCalled = true; return "from-gh", nil }, + } + got, err := src.Token(context.Background()) + if err != nil || got != "from-github-token" { + t.Fatalf("got=%q err=%v", got, err) + } + if ghCalled { + t.Fatal("GH fallback called despite GITHUB_TOKEN being set") + } + }) + + t.Run("gh fallback used when env empty", func(t *testing.T) { + _ = os.Unsetenv("AO_GITHUB_TOKEN") + _ = os.Unsetenv("GITHUB_TOKEN") + src := EnvTokenSource{ + EnvVars: []string{"AO_GITHUB_TOKEN"}, + GH: func(context.Context) (string, error) { return " from-gh \n", nil }, // trimmed + } + got, err := src.Token(context.Background()) + if err != nil || got != "from-gh" { + t.Fatalf("got=%q err=%v", got, err) + } + }) + + t.Run("gh fallback error swallowed to ErrNoToken", func(t *testing.T) { + _ = os.Unsetenv("AO_GITHUB_TOKEN") + _ = os.Unsetenv("GITHUB_TOKEN") + src := EnvTokenSource{ + EnvVars: []string{"AO_GITHUB_TOKEN"}, + GH: func(context.Context) (string, error) { return "", errors.New("gh: command not found") }, + } + _, err := src.Token(context.Background()) + if !errors.Is(err, ErrNoToken) { + t.Fatalf("err = %v, want ErrNoToken (gh error must not propagate)", err) + } + }) + + t.Run("gh fallback empty stdout falls through", func(t *testing.T) { + _ = os.Unsetenv("AO_GITHUB_TOKEN") + _ = os.Unsetenv("GITHUB_TOKEN") + src := EnvTokenSource{ + EnvVars: []string{"AO_GITHUB_TOKEN"}, + GH: func(context.Context) (string, error) { return " \n\t ", nil }, + } + _, err := src.Token(context.Background()) + if !errors.Is(err, ErrNoToken) { + t.Fatalf("err = %v, want ErrNoToken", err) + } + }) +} + func TestParseID(t *testing.T) { cases := []struct { name string From 78d938c4577cc2749a930d401dcf0844365edc9a Mon Sep 17 00:00:00 2001 From: harshitsinghbhandari <24b4506@iitb.ac.in> Date: Sun, 31 May 2026 01:39:48 +0530 Subject: [PATCH 2/2] refactor(tracker): address code-review on zero-config auth MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five fixes from the second review pass on PR #44. 1. Wrap gh runtime errors under ErrNoToken instead of swallowing them. gh's stderr ("not logged in to any GitHub hosts. Try `gh auth login`") now flows into the surfaced error message so daemon operators can see WHY zero-config auth fell through, not just THAT it did. errors.Is(err, ErrNoToken) still holds. 2. Exclude exec.ErrNotFound from the wrap — "user doesn't have gh installed" is uninteresting noise; bare ErrNoToken is the right shape. 3. Capture gh's stderr via cmd.Stderr = & builder before .Output() so the ExitError carries the human-readable cause that we then wrap into the ErrNoToken result. 4. Name the 5s probe deadline as bootTokenProbeTimeout — magic numbers get a story. 5. Add TestNewBoundsTokenProbe pinning the deadline. Stub TokenSource blocks until ctx is cancelled; New() must return within 4-8s. CI-safe margin. Cheap insurance for the one defensive property in this PR. Also: tightened the env-restore comment in TestEnvTokenSource to explain why both t.Setenv and manual save/restore are present (subtests call os.Unsetenv which t.Setenv can't restore). Tests: 57 in package with -race, 307 full suite green. vet + gofmt clean. Co-Authored-By: Claude Opus 4.7 --- .../internal/adapters/tracker/github/auth.go | 32 +++++++-- .../adapters/tracker/github/tracker.go | 8 ++- .../adapters/tracker/github/tracker_test.go | 69 +++++++++++++++++-- 3 files changed, 96 insertions(+), 13 deletions(-) diff --git a/backend/internal/adapters/tracker/github/auth.go b/backend/internal/adapters/tracker/github/auth.go index b39b2b62..76ddd8fc 100644 --- a/backend/internal/adapters/tracker/github/auth.go +++ b/backend/internal/adapters/tracker/github/auth.go @@ -3,6 +3,7 @@ package github import ( "context" "errors" + "fmt" "os" "os/exec" "strings" @@ -64,21 +65,40 @@ func (s EnvTokenSource) Token(ctx context.Context) (string, error) { if gh == nil { gh = ghAuthToken } - if v, err := gh(ctx); err == nil { + v, err := gh(ctx) + if err == nil { if v = strings.TrimSpace(v); v != "" { return v, nil } + // gh succeeded with empty stdout — same outcome as no token, but + // no underlying error to carry forward. Fall through to plain ErrNoToken. + return "", ErrNoToken + } + // gh failed. "Binary missing" is the uninteresting case — the user + // simply doesn't have gh installed, and we'd be noise to surface it. + // Anything else (not logged in, network blip, hung subprocess) carries + // operator-useful explanation we should preserve so a daemon log shows + // WHY zero-config auth fell through, not just THAT it did. + if errors.Is(err, exec.ErrNotFound) { + return "", ErrNoToken } - return "", ErrNoToken + return "", fmt.Errorf("%w (gh fallback: %v)", ErrNoToken, err) } // ghAuthToken shells out to the `gh` CLI and asks it for the user's -// currently logged-in token. Returns an error if gh is not installed or -// the user is not authenticated; the EnvTokenSource fallback chain -// swallows that error and reports ErrNoToken instead. +// currently logged-in token. On non-zero exit, gh writes its explanation +// ("not logged in to any GitHub hosts. Try `gh auth login`") to stderr; +// we capture it and fold it into the returned error so callers can surface +// the cause. func ghAuthToken(ctx context.Context) (string, error) { - out, err := exec.CommandContext(ctx, "gh", "auth", "token").Output() + cmd := exec.CommandContext(ctx, "gh", "auth", "token") + var stderr strings.Builder + cmd.Stderr = &stderr + out, err := cmd.Output() if err != nil { + if msg := strings.TrimSpace(stderr.String()); msg != "" { + return "", fmt.Errorf("%w: %s", err, msg) + } return "", err } return strings.TrimSpace(string(out)), nil diff --git a/backend/internal/adapters/tracker/github/tracker.go b/backend/internal/adapters/tracker/github/tracker.go index 5ebf0be5..cebdaafe 100644 --- a/backend/internal/adapters/tracker/github/tracker.go +++ b/backend/internal/adapters/tracker/github/tracker.go @@ -37,6 +37,12 @@ const ( // (matching the legacy gh CLI default) when the caller passes 0. defaultListLimit = 30 maxListLimit = 100 + + // bootTokenProbeTimeout bounds the construction-time token probe so a + // hung `gh auth token` fallback (broken shell, unhealthy PATH) cannot + // stall daemon startup. gh is sub-100ms on a healthy box; 5s gives + // ~50x headroom without making daemon-start feel slow. + bootTokenProbeTimeout = 5 * time.Second ) // Sentinel errors. Adapter-level callers should match on these via @@ -112,7 +118,7 @@ func New(opts Options) (*Tracker, error) { if src == nil { return nil, ErrNoToken } - bootCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + bootCtx, cancel := context.WithTimeout(context.Background(), bootTokenProbeTimeout) defer cancel() if _, err := src.Token(bootCtx); err != nil { return nil, err diff --git a/backend/internal/adapters/tracker/github/tracker_test.go b/backend/internal/adapters/tracker/github/tracker_test.go index d779e2b5..1a39d73a 100644 --- a/backend/internal/adapters/tracker/github/tracker_test.go +++ b/backend/internal/adapters/tracker/github/tracker_test.go @@ -8,6 +8,7 @@ import ( "net/http" "net/http/httptest" "os" + "os/exec" "reflect" "strconv" "strings" @@ -102,13 +103,43 @@ func TestNewRejectsMissingToken(t *testing.T) { } } +// TestNewBoundsTokenProbe pins the construction-time deadline so a future +// refactor can't silently drop it. The TokenSource stub blocks until ctx +// is cancelled; New() must return within the configured timeout + slack. +func TestNewBoundsTokenProbe(t *testing.T) { + blocking := EnvTokenSource{ + GH: func(ctx context.Context) (string, error) { + <-ctx.Done() + return "", ctx.Err() + }, + } + t.Setenv("GITHUB_TOKEN", "") // force chain to reach the gh stub + started := time.Now() + _, err := New(Options{Token: blocking}) + elapsed := time.Since(started) + + if !errors.Is(err, ErrNoToken) { + t.Fatalf("err = %v, want ErrNoToken", err) + } + // Generous CI margin: deadline is 5s, accept 4-8s. + if elapsed < 4*time.Second { + t.Fatalf("New() returned in %v — deadline not enforced", elapsed) + } + if elapsed > 8*time.Second { + t.Fatalf("New() took %v — deadline ineffective", elapsed) + } +} + // TestEnvTokenSource covers the auth lookup chain that gives us zero-config // behavior on a stock dev machine: configured env vars beat GITHUB_TOKEN // beats `gh auth token`. All three paths must work, AND a failing gh -// fallback must surface as ErrNoToken — never as an exec error — so the -// caller sees one uniform "configure a token" signal. +// fallback must surface as ErrNoToken (errors.Is) — the exec error is +// wrapped under it for debuggability but never propagated as its own type. func TestEnvTokenSource(t *testing.T) { - // Save and restore env vars the chain reads. + // t.Setenv handles its own restore, but several subtests call + // os.Unsetenv directly (Go has no t.Unsetenv). Save-and-restore at + // the parent level so an explicit unset in one subtest doesn't leak + // into the next or into other tests in the package. saved := map[string]string{} for _, k := range []string{"GITHUB_TOKEN", "AO_GITHUB_TOKEN"} { saved[k] = os.Getenv(k) @@ -171,16 +202,42 @@ func TestEnvTokenSource(t *testing.T) { } }) - t.Run("gh fallback error swallowed to ErrNoToken", func(t *testing.T) { + t.Run("gh exec.ErrNotFound surfaces as bare ErrNoToken", func(t *testing.T) { + // Binary missing is the uninteresting case — operator hasn't + // installed gh. No useful explanation to surface; plain ErrNoToken. _ = os.Unsetenv("AO_GITHUB_TOKEN") _ = os.Unsetenv("GITHUB_TOKEN") src := EnvTokenSource{ EnvVars: []string{"AO_GITHUB_TOKEN"}, - GH: func(context.Context) (string, error) { return "", errors.New("gh: command not found") }, + GH: func(context.Context) (string, error) { return "", &exec.Error{Name: "gh", Err: exec.ErrNotFound} }, } _, err := src.Token(context.Background()) if !errors.Is(err, ErrNoToken) { - t.Fatalf("err = %v, want ErrNoToken (gh error must not propagate)", err) + t.Fatalf("err = %v, want ErrNoToken", err) + } + // Bare ErrNoToken: no wrapping suffix. + if got := err.Error(); strings.Contains(got, "gh fallback") { + t.Fatalf("err = %q, expected no gh-fallback annotation for missing-binary case", got) + } + }) + + t.Run("gh runtime error wrapped under ErrNoToken with cause", func(t *testing.T) { + // gh exists but failed (not logged in, etc). errors.Is contract still + // holds, AND the operator-useful explanation is in the message. + _ = os.Unsetenv("AO_GITHUB_TOKEN") + _ = os.Unsetenv("GITHUB_TOKEN") + src := EnvTokenSource{ + EnvVars: []string{"AO_GITHUB_TOKEN"}, + GH: func(context.Context) (string, error) { + return "", errors.New("not logged in to any GitHub hosts. Try `gh auth login`") + }, + } + _, err := src.Token(context.Background()) + if !errors.Is(err, ErrNoToken) { + t.Fatalf("err = %v, want errors.Is(err, ErrNoToken)", err) + } + if got := err.Error(); !strings.Contains(got, "not logged in") { + t.Fatalf("err = %q, expected gh's explanation in the wrapped message", got) } })