Skip to content

feat(cli): log token scope at start of fullsend run#1333

Merged
ralphbean merged 3 commits into
mainfrom
feat/token-scope-logging
May 27, 2026
Merged

feat(cli): log token scope at start of fullsend run#1333
ralphbean merged 3 commits into
mainfrom
feat/token-scope-logging

Conversation

@ralphbean
Copy link
Copy Markdown
Contributor

Summary

Refs: #1321

Test plan

  • TestFetchTokenScope_ReturnsRepoNames — happy path
  • TestFetchTokenScope_EmptyToken — skips gracefully
  • TestFetchTokenScope_APIError — returns error
  • TestFetchTokenScope_NonInstallationToken — 403 treated as error
  • Full cli test suite passes

🤖 Generated with Claude Code

At the start of every fullsend run, introspect the GH_TOKEN by calling
GET /installation/repositories and log which repos the token is scoped
to. This surfaces cross-org token scoping issues immediately rather
than requiring post-hoc debugging.

The check is non-fatal: if the token isn't an installation token or the
API call fails, a warning is logged and the run continues normally.

Refs: #1321

Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Ralph Bean <rbean@redhat.com>
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 21, 2026

Site preview

Preview: https://885d563a-site.fullsend-ai.workers.dev

Commit: 07cc6ae96465e65f78d07bad6f4f95351a3f021f

PATs and GITHUB_TOKENs return 401/403 on /installation/repositories.
Treat these as "not an installation token" and skip silently instead
of logging a warning on every run.

Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Ralph Bean <rbean@redhat.com>
@fullsend-ai-review
Copy link
Copy Markdown

Review

Reason: stale-head

The review agent reviewed commit adaa55cef0d13fe74b2e26407eac4b54f674c468 but the PR HEAD is now 3c1080db1ae0e6d3d17893e090ef6a81537207d4. This review was discarded to avoid approving unreviewed code.

Copy link
Copy Markdown
Contributor

@waynesun09 waynesun09 left a comment

Choose a reason for hiding this comment

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

Review Squad (10 agents) — 1 HIGH, 4 MEDIUM findings

Useful diagnostic feature with solid test coverage. The HIGH finding (pagination truncation at 100 repos) is the most actionable — large orgs will get silently incomplete output, defeating the debugging purpose. The hardcoded base URL, missing context.Context, empty repo list handling, and package-level client are standard code quality improvements.

return nil, nil
}

url := baseURL + "/installation/repositories?per_page=100"
Copy link
Copy Markdown
Contributor

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
Contributor 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.

Comment thread internal/cli/run.go Outdated
// 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(ghToken, "https://api.github.com")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

MEDIUM — Hardcoded api.github.com breaks GHES

GitHub Actions sets GITHUB_API_URL for GHES runners. If GHES support is ever added, this call will silently hit the wrong API.

apiURL := os.Getenv("GITHUB_API_URL")
if apiURL == "" {
    apiURL = "https://api.github.com"
}
repos, err := fetchTokenScope(ghToken, apiURL)

5/10 review agents flagged this

Copy link
Copy Markdown
Contributor 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 as-is. No GHES support exists anywhere in the codebase — GITHUB_API_URL isn't referenced in any Go file. Adding it here alone wouldn't actually enable GHES support, and I'd rather not introduce a one-off pattern that diverges from every other API call. Do you see a nearer-term need for this?

// GET /installation/repositories and returning the full_name of each
// accessible repo. Returns (nil, nil) if the token is empty.
func fetchTokenScope(token, baseURL string) ([]string, error) {
if token == "" {
Copy link
Copy Markdown
Contributor

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
Contributor 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.

Comment thread internal/cli/run.go
repos, err := fetchTokenScope(ghToken, "https://api.github.com")
if err != nil {
printer.StepWarn("Token scope check: " + err.Error())
} else if len(repos) > 0 {
Copy link
Copy Markdown
Contributor

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
Contributor 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.

)

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

Copy link
Copy Markdown
Contributor

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
Contributor 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?

- Add truncation indicator when total_count > returned repos (HIGH)
- Accept context.Context for cancellation support (MEDIUM)
- Warn when installation token has access to 0 repositories (MEDIUM)

Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Ralph Bean <rbean@redhat.com>
Copy link
Copy Markdown
Contributor

@waynesun09 waynesun09 left a comment

Choose a reason for hiding this comment

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

Thanks for the thorough responses. The three fixes (truncation indicator, context.Context, empty repo warning) all look correct and well-tested. The two pushbacks are reasonable — agreed on both: no point adding a one-off GHES pattern, and the package-level client is consistent with the existing httpClient in the same file.

@ralphbean ralphbean added this pull request to the merge queue May 27, 2026
Merged via the queue into main with commit 78b9071 May 27, 2026
17 of 20 checks passed
@ralphbean ralphbean deleted the feat/token-scope-logging branch May 27, 2026 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants