Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions internal/cli/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,20 @@ func runAgent(agentName, fullsendDir, outputBase, targetRepo, fullsendBinary str
}
printer.Blank()

// 1b. Log token scope for debugging cross-org issues (see #1321).
// Non-fatal: if the check fails (e.g., non-installation token), log a
// warning and continue.
if ghToken := os.Getenv("GH_TOKEN"); ghToken != "" {
repos, err := fetchTokenScope(context.Background(), ghToken, "https://api.github.com")
if err != nil {
printer.StepWarn("Token scope check: " + err.Error())
} else if len(repos) > 0 {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MEDIUM — Empty repo list silently ignored

When fetchTokenScope returns a non-nil empty slice (installation token with 0 repos), the len(repos) > 0 guard silently skips it. An installation token scoped to zero repos is a significant misconfiguration that will cause downstream agent failures.

} else if len(repos) > 0 {
    printer.KeyValue("Token scoped to", strings.Join(repos, ", "))
} else {
    printer.StepWarn("Token is an installation token but has access to 0 repositories")
}

2/10 review agents flagged this

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 07cc6ae. Distinguishing nil (non-installation token) from empty slice (0 repos) now — the latter gets a warning.

printer.KeyValue("Token scoped to", strings.Join(repos, ", "))
} else if repos != nil {
printer.StepWarn("Token is an installation token but has access to 0 repositories")
}
}

// 2. Check openshell availability.
openshellStart := time.Now()
printer.StepStart("Checking openshell availability")
Expand Down
69 changes: 69 additions & 0 deletions internal/cli/tokenscope.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
package cli

import (
"context"
"encoding/json"
"fmt"
"io"
"net/http"
"time"
)

var tokenScopeClient = &http.Client{Timeout: 10 * time.Second}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MEDIUM — Package-level mutable HTTP client

tokenScopeClient is a package-level var, making the function harder to test in isolation and inconsistent with the rest of the codebase where HTTP clients are struct fields (forge/github.LiveClient.http, mint.Server.httpClient). Consider passing *http.Client as a parameter instead.

3/10 review agents flagged this

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left this one too. There's already a package-level httpClient at run.go:1820, so this is consistent with the file's existing pattern. The tests work fine against httptest servers as-is. Does it feel worth the churn to you for a single-use internal function?

// fetchTokenScope introspects a GitHub installation token by calling
// GET /installation/repositories and returning the full_name of each
// accessible repo. Returns (nil, nil) if the token is empty.
func fetchTokenScope(ctx context.Context, token, baseURL string) ([]string, error) {
if token == "" {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MEDIUM — No context.Context parameter — request is uncancellable

Uses http.NewRequest instead of http.NewRequestWithContext. The 10s http.Client.Timeout is the only deadline mechanism — the request can't be cancelled if runAgent is interrupted (e.g., SIGTERM). The rest of the codebase uses context.Context throughout (e.g., forge/github.LiveClient.do()).

func fetchTokenScope(ctx context.Context, token, baseURL string) ([]string, error) {
    ctx, cancel := context.WithTimeout(ctx, 5*time.Second)
    defer cancel()
    req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil)
    ...
}

4/10 review agents flagged this

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 07cc6ae. Using http.NewRequestWithContext with a 5s timeout context now.

return nil, nil
}

ctx, cancel := context.WithTimeout(ctx, 5*time.Second)
defer cancel()

url := baseURL + "/installation/repositories?per_page=100"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HIGH — Installations with >100 repos silently truncated

per_page=100 with no pagination and no total_count check. Large orgs with >100 repos in the installation scope will silently show only the first 100, with no indication the list is incomplete. This is the exact scenario (large multi-org deployments) this debugging feature is meant to help with.

Suggest parsing total_count from the response and appending a suffix when truncated:

if result.TotalCount > len(repos) {
    repos = append(repos, fmt.Sprintf("... and %d more (%d total)",
        result.TotalCount-len(repos), result.TotalCount))
}

This avoids pagination complexity while ensuring operators know the list is incomplete.

9/10 review agents flagged this (near-unanimous)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 07cc6ae. Parsing total_count and appending a suffix when truncated, as you suggested.

req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil)
if err != nil {
return nil, fmt.Errorf("creating scope request: %w", err)
}
req.Header.Set("Authorization", "Bearer "+token)
req.Header.Set("Accept", "application/vnd.github+json")

resp, err := tokenScopeClient.Do(req)
if err != nil {
return nil, fmt.Errorf("fetching token scope: %w", err)
}
defer resp.Body.Close()

if resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusUnauthorized {
// PATs and GITHUB_TOKENs can't call /installation/repositories.
// Not an error — just means this isn't an installation token.
io.Copy(io.Discard, io.LimitReader(resp.Body, 4096))
return nil, nil
}
if resp.StatusCode != http.StatusOK {
io.Copy(io.Discard, io.LimitReader(resp.Body, 4096))
return nil, fmt.Errorf("token scope check returned status %d", resp.StatusCode)
}

var result struct {
TotalCount int `json:"total_count"`
Repositories []struct {
FullName string `json:"full_name"`
} `json:"repositories"`
}
if err := json.NewDecoder(resp.Body).Decode(&result); err != nil {
return nil, fmt.Errorf("decoding token scope: %w", err)
}

repos := make([]string, len(result.Repositories))
for i, r := range result.Repositories {
repos[i] = r.FullName
}
if result.TotalCount > len(result.Repositories) {
repos = append(repos, fmt.Sprintf("... and %d more (%d total)",
result.TotalCount-len(result.Repositories), result.TotalCount))
}
return repos, nil
}
113 changes: 113 additions & 0 deletions internal/cli/tokenscope_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
package cli

import (
"context"
"encoding/json"
"net/http"
"net/http/httptest"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func TestFetchTokenScope_ReturnsRepoNames(t *testing.T) {
github := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
assert.Equal(t, "/installation/repositories", r.URL.Path)
assert.Equal(t, "100", r.URL.Query().Get("per_page"))
assert.Equal(t, "Bearer ghs_test_token", r.Header.Get("Authorization"))

json.NewEncoder(w).Encode(map[string]interface{}{
"total_count": 2,
"repositories": []map[string]string{
{"full_name": "org-a/repo-one"},
{"full_name": "org-a/repo-two"},
},
})
}))
defer github.Close()

repos, err := fetchTokenScope(context.Background(), "ghs_test_token", github.URL)
require.NoError(t, err)
assert.Equal(t, []string{"org-a/repo-one", "org-a/repo-two"}, repos)
}

func TestFetchTokenScope_Truncated(t *testing.T) {
// When total_count exceeds the number of returned repos, append a
// summary entry so operators know the list is incomplete.
github := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
json.NewEncoder(w).Encode(map[string]interface{}{
"total_count": 150,
"repositories": []map[string]string{
{"full_name": "org/repo-1"},
{"full_name": "org/repo-2"},
},
})
}))
defer github.Close()

repos, err := fetchTokenScope(context.Background(), "ghs_token", github.URL)
require.NoError(t, err)
require.Len(t, repos, 3)
assert.Equal(t, "org/repo-1", repos[0])
assert.Equal(t, "org/repo-2", repos[1])
assert.Equal(t, "... and 148 more (150 total)", repos[2])
}

func TestFetchTokenScope_EmptyToken(t *testing.T) {
repos, err := fetchTokenScope(context.Background(), "", "https://unused")
require.NoError(t, err)
assert.Nil(t, repos)
}

func TestFetchTokenScope_NonInstallationToken_401(t *testing.T) {
// Non-installation tokens get 401. Treated as "not an installation
// token" — returns (nil, nil), not an error.
github := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusUnauthorized)
}))
defer github.Close()

repos, err := fetchTokenScope(context.Background(), "ghs_bad", github.URL)
assert.NoError(t, err)
assert.Nil(t, repos)
}

func TestFetchTokenScope_NonInstallationToken_403(t *testing.T) {
// PATs and GITHUB_TOKENs return 403 on /installation/repositories.
// Treated as "not an installation token" — silent, no error.
github := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusForbidden)
}))
defer github.Close()

repos, err := fetchTokenScope(context.Background(), "ghp_personal", github.URL)
assert.NoError(t, err)
assert.Nil(t, repos)
}

func TestFetchTokenScope_UnexpectedStatus(t *testing.T) {
github := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusInternalServerError)
}))
defer github.Close()

repos, err := fetchTokenScope(context.Background(), "ghs_token", github.URL)
assert.Error(t, err)
assert.Contains(t, err.Error(), "status 500")
assert.Nil(t, repos)
}

func TestFetchTokenScope_CancelledContext(t *testing.T) {
github := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
t.Fatal("request should not have been sent")
}))
defer github.Close()

ctx, cancel := context.WithCancel(context.Background())
cancel()

repos, err := fetchTokenScope(ctx, "ghs_token", github.URL)
assert.Error(t, err)
assert.Nil(t, repos)
}
Loading