feat(#187): detect classic PAT rejection and show guidance

[fullsend-ai-coder]

Add a new ErrClassicPATForbidden sentinel to the forge package, detected when a GitHub org returns 403 with "forbids access via a personal access token" in the API response. The sentinel is wired into APIError.Unwrap() following the existing pattern used by ErrNotFound and ErrAlreadyExists.

At key CLI call sites (GetRepo, ListOrgRepos, ensureConfigRepoExists, loadRepoConfig) the error is intercepted and wrapped with actionable guidance: the token resolution order (GH_TOKEN > GITHUB_TOKEN > gh auth token), instructions to create a fine-grained PAT, and the required repository permissions.

The configuring-github.md docs are updated with a "Token resolution" section and a "Organizations that restrict classic PATs" section including the permissions table.

---

Closes #187

Post-script verification

• Branch is not main/master (agent/187-classic-pat-guidance)
• Secret scan passed (gitleaks — f96750babbed5ada406a9ae04e8068449701d9c7..HEAD)
• Pre-commit hooks passed (authoritative run on runner)
• Tests ran inside sandbox

[github-actions]

E2E tests did not run

E2E tests run automatically for org/repo members and collaborators on pull requests.

For other contributors, a maintainer must add the ok-to-test label after the latest push.

See E2E testing guide for details.

[fullsend-ai-review]

Review

Verdict: comment · non-blocking observations

Clean feature that follows the established sentinel-error pattern from ADR-0005. The forge-level ErrClassicPATForbidden sentinel, detection in APIError.Unwrap(), and CLI-level wrapClassicPATError helper are well-structured and correctly preserve the error chain (%w). Tests cover the detection logic thoroughly with positive and negative cases, and the existing TestAPIError_Unwrap modification is a legitimate extension, not test weakening.

Findings

1. Incomplete call-site coverage for wrapClassicPATError · medium · correctness

The PR wraps 5 call sites but the codebase has additional ListOrgRepos and GetRepo call sites that could also encounter the classic-PAT 403:

File	Line	Function	Context
internal/cli/admin.go	1164	runDryRun	Fallback ListOrgRepos when discoveredRepos is nil
internal/cli/admin.go	1493	runInstall	Fallback ListOrgRepos when discoveredRepos is nil
internal/cli/admin.go	1786	runAnalyze	Direct entry point — fullsend admin analyze <org>
internal/cli/admin.go	2193, 2212	enable subcommand	fullsend admin enable paths
internal/cli/github.go	697	runGitHubStatus	fullsend github status
internal/cli/github.go	843	runGitHubUninstall	fullsend github uninstall

Of these, runAnalyze (1786) and the enable paths (2193/2212) are direct CLI entry points where a user on a restricted org would see the raw API error without the guidance message. The runDryRun/runInstall fallback paths are unlikely to be hit from the install command (since line 434 pre-fetches), but could be triggered from other callers.

The error still propagates correctly at all these sites — the missing wrapping is a UX consistency gap, not a correctness bug.

2. isClassicPATForbiddenError deviates from sibling helper pattern · low · style

The existing helpers isBranchProtectionError and isAlreadyExistsError concatenate apiErr.Errors[] messages before checking:

// Existing pattern:
func isAlreadyExistsError(apiErr *APIError) bool {
    msg := strings.ToLower(apiErr.Message)
    for _, d := range apiErr.Errors {
        msg += " " + strings.ToLower(d.Message)
    }
    return strings.Contains(msg, "already exists")
}

The new helper only checks apiErr.Message. This is functionally correct — the classic-PAT 403 from GitHub puts its message in the top-level message field, not the errors array (which is a 422-era convention). But the inconsistency could confuse future contributors.

3. Test coverage gap for wired call sites · low · test-adequacy

Of the 5 call sites wired with wrapClassicPATError in the diff, only applyPerRepoScaffold has an integration-level test (TestApplyPerRepoScaffold_ClassicPATForbidden). The wrapClassicPATError unit tests provide good coverage of the wrapping logic in isolation, but ensureConfigRepoExists, loadRepoConfig, and the two ListOrgRepos sites lack tests confirming the wrapping is actually invoked from those code paths.

4. String-based error detection is fragile · low · robustness

The detection relies on strings.Contains(msg, "forbids access via a personal access token") which could silently stop matching if GitHub changes their error message wording. No fallback detection or structured field check (e.g., documentation_url) is used. The practical risk is low — the detection degrades gracefully to the pre-PR behavior (raw error, no guidance).

Summary

The PR correctly addresses both gaps identified in #187: CLI detection with actionable guidance and documentation updates. The sentinel/Unwrap/wrapper pattern is clean, tests are solid, and the error chain is preserved. The main observation is that the wrapping could be applied to more call sites for complete coverage across all CLI commands.

Previous run

Review

Verdict: Approve — clean implementation that follows established sentinel error patterns and addresses both gaps identified in #187.

What this PR does

Adds detection of GitHub's "classic PAT forbidden" 403 response (ErrClassicPATForbidden sentinel) and wraps it with actionable CLI guidance at five key call sites. Updates configuring-github.md with token resolution order and fine-grained PAT permissions table. Comprehensive tests cover the detection logic, unwrap behavior, and CLI integration.

Architecture

The implementation correctly follows the forge abstraction:

• Sentinel (ErrClassicPATForbidden) lives in forge.go — forge-agnostic, consistent with ErrNotFound, ErrAlreadyExists, ErrBranchProtected.
• Detection (isClassicPATForbiddenError) lives in github/github.go — GitHub-specific, wired into APIError.Unwrap().
• Guidance (wrapClassicPATError) lives in cli/admin.go — presentation layer, not forge layer. Static string, no user interpolation.

Error chain verified: APIError.Unwrap() → ErrClassicPATForbidden → fmt.Errorf("%w") wrapping preserves sentinel through errors.Is() — tests confirm this.

Observations

1. Detection function doesn't check apiErr.Errors[] (low)

isClassicPATForbiddenError only checks apiErr.Message, while the sibling functions isBranchProtectionError and isAlreadyExistsError both concatenate apiErr.Errors[].Message before matching:

// Existing pattern (isBranchProtectionError, isAlreadyExistsError):
msg := strings.ToLower(apiErr.Message)
for _, d := range apiErr.Errors {
    msg += " " + strings.ToLower(d.Message)
}
return strings.Contains(msg, "...")

// New function only checks Message:
return strings.Contains(strings.ToLower(apiErr.Message), "forbids access via a personal access token")

Practically this is fine — GitHub's classic PAT 403 puts the message in the top-level message field, and the Errors array is typically empty for 403s. But for pattern consistency, consider adopting the same structure.

2. Uncovered call sites (low)

The PR wraps 5 of ~15 GetRepo/ListOrgRepos call sites. The covered sites are the most likely entry points for a new user (admin install, github setup, loadRepoConfig, ensureConfigRepoExists, applyPerRepoScaffold). Uncovered sites like runAnalyze, runDryRun, runUninstall, and runGitHubStatus will still show the raw GitHub error message (which is itself somewhat informative), just without the fullsend-specific guidance. This is acceptable as a phased approach — the sentinel detection in Unwrap() is universal.

Security

No security concerns. The guidance constant is static text with a placeholder token example (github_pat_...). No user input interpolation. No secrets embedded. The permissions table accurately reflects the API operations in the codebase. No prompt injection or instruction injection patterns detected in the diff or PR body.

Tests

Adequate coverage:

• TestIsClassicPATForbiddenError — positive and negative cases (org forbids, generic 403, rate limits)
• TestAPIError_Unwrap — extended with classic PAT unwrap cases
• TestApplyPerRepoScaffold_ClassicPATForbidden — CLI integration test
• TestWrapClassicPATError_WrapsMatchingError / PassthroughNonMatching — helper unit tests

No existing tests weakened — the prior "403 does not unwrap" test is replaced with more specific cases that maintain the same invariant for non-classic-PAT 403s.

---

Labels: PR adds new CLI error detection feature and updates user-facing docs

[guyoron1]

/fs-review