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

[fullsend-ai-coder]

When a GitHub org blocks classic personal access tokens, API calls return a 403 with a specific message. The CLI now detects this at the forge layer (ErrClassicPATForbidden sentinel) and wraps it with actionable guidance: token resolution order, required fine-grained PAT permissions, and the export command.

Wrapped call sites: ListOrgRepos (admin + github), GetRepo (applyPerRepoScaffold, ensureConfigRepoExists, loadRepoConfig). Added docs section with token resolution order and permissions table.

Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com

---

Closes #245

Post-script verification

• Branch is not main/master (agent/245-classic-pat-guidance)
• Secret scan passed (gitleaks — c776c248ac9a91f10998ca7107e974a6c9d76514..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]

🤖 Finished Review · ❌ Failure · Started 12:59 PM UTC · Completed 1:00 PM UTC
Commit: c776c24 · View workflow run →

[guyoron1]

/fs-review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 3:57 PM UTC · Completed 4:11 PM UTC
Commit: d8e3df7 · View workflow run →

[fullsend-ai-review]

Review

Findings

Low

• [naming-convention] internal/cli/admin.go:108 — The function wrapClassicPATError could be named addClassicPATGuidance or withClassicPATGuidance to better reflect that it conditionally enriches an error with user-facing guidance rather than wrapping unconditionally. The current name is not wrong — wrap is idiomatic Go — but the add*/with* pattern for context enrichment is slightly more descriptive. Mild preference, not a convention violation.

• [error-handling-idiom] internal/cli/root.go:48 — The root-level wrapping in Execute() applies classic PAT guidance to all commands. This is the correct design since the 403 can occur on any GitHub API call from any subcommand. The comment at lines 45–47 already documents this intent. For future maintainers, consider briefly noting the deliberate contrast with ErrBranchProtected's targeted wrapping in commitFilesTo — that one is context-dependent (only meaningful during commit), while classic PAT rejection is context-independent.

Previous run

Review

Verdict: comment · non-blocking findings

Clean, well-structured PR that correctly addresses both gaps in issue #245. The forge-layer sentinel follows the established ErrNotFound/ErrAlreadyExists pattern, detection is properly scoped to internal/forge/github/, wrapping lives in the CLI layer, and tests cover the implemented paths. Documentation additions are accurate and match the code's resolveToken() implementation.

Two findings worth noting for follow-up — neither blocks this PR.

---

Findings

1. Incomplete call-site coverage — wrapClassicPATError applied to 5 of 14+ call sites · medium · correctness

File: internal/cli/admin.go, internal/cli/github.go

The PR wraps the github setup and install primary entry paths, which matches the issue's focus. However, the classic PAT restriction is org-wide — every API call to that org will 403 the same way. Users running other commands (analyze, enable --all, disable, status, uninstall, dry-run) will still see the opaque GitHub error with no guidance.

Unwrapped call sites:

• admin.go:1164 — runDryRun → ListOrgRepos
• admin.go:1493 — runInstall (non-discovery path) → ListOrgRepos
• admin.go:1786 — runAnalyze → ListOrgRepos
• admin.go:2193, 2212 — enableRepos → ListOrgRepos
• admin.go:2444 — disableRepos variable sync → ListOrgRepos (non-fatal warning path)
• github.go:697 — runGitHubStatus → GetRepo
• github.go:843 — runGitHubUninstall → GetRepo

Remediation: Consider centralizing the wrapping — either a decorator around the forge.Client that detects the sentinel at the transport level, or a single handleForgeError helper in the CLI's top-level error path. This would cover future call sites automatically and eliminate the per-site wrapping pattern.

2. isClassicPATForbiddenError only checks Message, not Errors[] · low · correctness

File: internal/forge/github/github.go
Line: ~802 (in diff)

The peer functions isBranchProtectionError and isAlreadyExistsError both concatenate apiErr.Message with apiErr.Errors[].Message before matching. The new function only checks the top-level Message field:

func isClassicPATForbiddenError(apiErr *APIError) bool {
	return strings.Contains(strings.ToLower(apiErr.Message),
		"forbids access via a personal access token")
}

For 403s, GitHub typically puts the message at the top level, so this works in practice. A code comment explaining the deviation from the peer pattern (or matching the pattern for consistency) would prevent future confusion.

---

What looks good

• Forge abstraction respected: Sentinel in forge.go, detection in github/github.go, wrapping in cli/ — textbook use of the existing architecture.
• Error chain preserved: fmt.Errorf("%s\n\n%w", ...) keeps ErrClassicPATForbidden reachable via errors.Is for callers further up the stack.
• Tests cover the mechanism: TestAPIError_Unwrap verifies detection, TestWrapClassicPATError verifies wrapping + pass-through, TestApplyPerRepoScaffold_ClassicPATError verifies end-to-end through a real call site.
• Docs match code: Token resolution order and permissions table in configuring-github.md accurately reflect resolveToken() and the permissions the CLI actually needs.
• No security concerns: No auth changes, no secret exposure, no injection vectors. The guidance URL is a legitimate first-party GitHub page.

---

Labels: PR adds error detection in forge layer and CLI guidance with documentation updates.

[guyoron1]

/fs-fix

[fullsend-ai-coder]

🤖 Finished Fix · ✅ Success · Started 5:38 PM UTC · Completed 5:51 PM UTC
Commit: d8e3df7 · View workflow run →

[fullsend-ai-coder]

🔧 Fix agent — iteration 1 (human-triggered)

Addressed both review findings. Centralized classic PAT error wrapping in Execute() to cover all commands (finding 1), and aligned isClassicPATForbiddenError with peer pattern by checking Errors[] (finding 2).

Fixed (2):

1. Incomplete call-site coverage — wrapClassicPATError applied to 5 of 14+ call sites: Moved wrapClassicPATError from 5 per-call-site invocations to the top-level Execute() function in root.go. This covers all current and future commands automatically, eliminating the per-site wrapping pattern. Removed per-call-site wrapping from admin.go (4 sites) and github.go (1 site). Updated TestApplyPerRepoScaffold_ClassicPATError to verify sentinel propagation rather than guidance text (guidance is now added centrally).
2. isClassicPATForbiddenError only checks Message, not Errors[]: Updated isClassicPATForbiddenError to concatenate apiErr.Message with apiErr.Errors[].Message before matching, consistent with the peer pattern used by isBranchProtectionError and isAlreadyExistsError.

Tests: passed

_{Updated by fullsend fix agent}

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 5:53 PM UTC · Completed 6:07 PM UTC
Commit: d8e3df7 · View workflow run →

[fullsend-ai-review]

[low] naming-convention

The function wrapClassicPATError could be named addClassicPATGuidance or withClassicPATGuidance to better reflect that it conditionally enriches an error with user-facing guidance rather than wrapping unconditionally. Mild preference, not a convention violation.

[fullsend-ai-review]

[low] error-handling-idiom

The root-level wrapping in Execute() applies classic PAT guidance to all commands, which is correct since the 403 can occur on any GitHub API call. The comment already documents this intent. Consider noting the deliberate contrast with ErrBranchProtected targeted wrapping in commitFilesTo for future maintainers.