[ARCHIVED] Benchmark issue 467

[guyoron1]

Closes #466

LSP experiment v2: clean first-review with gopls-lsp enabled.
Code identical to PR #427 (issue #364).
Branch created from commit 4081de8.

[guyoron1]

/fs-review

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 10:15 AM UTC · Completed 10:25 AM UTC
Commit: 758c511 · View workflow run →

[fullsend-ai-review]

Review

Verdict: Approve ✅

Clean, well-structured enhancement that adds actionable remediation guidance when a GitHub organization rejects classic PATs. The implementation follows established forge abstraction patterns precisely.

What was reviewed

Dimension	Result
Correctness	✅ No issues — all ListOrgRepos/GetRepo call sites across admin.go and github.go are covered by classicPATHint() wrapping. Error chain preserved through fmt.Errorf("%w", ...) correctly.
Security	✅ No issues — no secrets exposed, no injection vectors, no permission changes. Error messages reference env var names and public URLs, not credential values.
Intent & coherence	✅ Matches issue #466 scope exactly. Forge abstraction respected: sentinel error in forge.go, detection in github/github.go, CLI guidance in cli/.
Style & conventions	✅ New sentinel ErrClassicPATForbidden + IsClassicPATForbidden() follows the exact pattern of ErrNotFound/ErrAlreadyExists/ErrBranchProtected. Detection function follows isAlreadyExistsError pattern with justified status-code guard.
Documentation	✅ Token resolution order in docs matches resolveToken() implementation. Permissions table matches CLI operations. Error message and docs are consistent.
Cross-repo contracts	✅ No concerns — all changes are in internal/ packages. Purely additive (new sentinel + helper), no interface changes.

Observations (low severity, non-blocking)

1. Test coverage (internal/cli/admin_test.go): Unit tests for classicPATHint() and isClassicPATForbiddenError() are solid (table-driven, good edge cases). No integration test exercises the end-to-end path from a 403 API response through to CLI remediation output — acceptable given the unit coverage and that this is a UX enhancement.

2. String matching robustness (internal/forge/github/github.go): Detection relies on strings.Contains(msg, "forbids access via a personal access token"). If GitHub changes this wording, the detection stops working silently (false negative, not false positive). Degrades gracefully — the CLI returns the original error without the hint. The test suite pins this string, so a deliberate update would catch the dependency.

Design notes

• The runEnableRepos and loadRepoConfig paths use an explicit forge.IsClassicPATForbidden(err) check before classicPATHint() to differentiate the printer.StepFail message. This is intentional UX differentiation, not redundancy.
• runDisableRepos correctly treats the classic PAT error as non-fatal (StepWarn + skip variable sync) since disable is a cleanup operation.
• Downstream GetFileContent calls are not wrapped because they are always preceded by a covered GetRepo/ListOrgRepos call — org-level PAT rejection blocks all API calls, so the first call in each path catches it.

Previous run

Review

Verdict: approve

Clean, well-structured change that adds detection and actionable remediation guidance for GitHub organizations that block classic personal access tokens. The implementation follows the existing sentinel error pattern (ErrNotFound / ErrAlreadyExists / ErrBranchProtected) exactly, with a new ErrClassicPATForbidden sentinel in the forge package, GitHub-specific detection in isClassicPATForbiddenError, and a CLI wrapper (classicPATHint) applied comprehensively across error paths.

Dimensions reviewed

Dimension	Result
Correctness	✅ No issues. Error chain preservation via %w is correct — errors.Is traversal works through classicPATHint → fmt.Errorf → APIError.Unwrap → ErrClassicPATForbidden. Detection function correctly gates on HTTP 403 + specific message substring, avoiding false positives on rate limits and other 403 types. All ListOrgRepos and GetRepo call sites in admin.go and github.go are wrapped.
Security	✅ No issues. No credential leakage — error output contains only static remediation text and placeholder token patterns. No changes to authentication, authorization, or RBAC logic. Detection mechanism influences only error message formatting, not access control decisions.
Intent & coherence	✅ Matches issue #466's two stated gaps (CLI error guidance + missing docs). Follows the forge abstraction — GitHub-specific logic stays in internal/forge/github/, CLI code uses forge-agnostic forge.IsClassicPATForbidden().
Style & conventions	✅ Naming follows established patterns: ErrClassicPATForbidden / IsClassicPATForbidden() / isClassicPATForbiddenError() match existing sentinels. The two application patterns (simple classicPATHint() wrapper vs. inline forge.IsClassicPATForbidden check with printer feedback) serve different purposes — the inline pattern is used where additional UI output (StepFail/StepWarn) is needed before the error return.
Documentation	✅ New sections in configuring-github.md accurately document the resolveToken() order (GH_TOKEN → GITHUB_TOKEN → gh auth token) and the required fine-grained PAT permissions. Permission table matches the CLI's actual forge.Client usage.
Cross-repo contracts	⏭ Skipped — changes are purely additive (new sentinel + helper), no breaking API changes.

Test coverage

Adequate. Detection logic has 6 test cases covering the key scenarios (org-name variations, false positives on other 403s, Errors-array-only matching, wrong status codes). APIError.Unwrap has a new case confirming the sentinel mapping. classicPATHint has 2 cases (matching + passthrough).

Notes

• The ListOrgInstallations call sites in runUninstall and detectSharedApps are not wrapped with classicPATHint. This is acceptable — those paths are downstream of already-wrapped ListOrgRepos calls that would fail first with the hint, and some handle errors gracefully (swallowing for race conditions, warning and proceeding).
• Issue [ARCHIVED] Benchmark issue 466 #466 is closed as duplicate (of [ARCHIVED] Benchmark issue 364 #364). The PR body states this is an LSP experiment re-submission of PR fix(#364): detect classic PAT rejection and guide user #427 with identical code. The changes themselves are substantive and address a real user-facing gap regardless of the experiment context.

---

Labels: PR modifies CLI error handling (internal/cli/) and forge error abstraction (internal/forge/), and adds user documentation

Previous run (2)

Review — PR #467

Verdict: ✅ Approve

Summary

This PR addresses both gaps identified in #466: it detects GitHub's "classic PAT forbidden" 403 error at the forge layer, surfaces actionable remediation in the CLI, and documents token resolution order and fine-grained PAT requirements.

The implementation follows existing codebase patterns precisely — sentinel error in forge.go, detection logic in github.go, and a CLI-layer hint wrapper — consistent with how ErrNotFound, ErrAlreadyExists, and ErrBranchProtected are structured.

Dimension results

Dimension	Result
Correctness	✅ No issues. Error chain preserves errors.Is traversal. Detection logic correctly gates on HTTP 403 + specific message substring. Tests cover positive, negative, and edge cases without weakening existing assertions.
Security	✅ No issues. No secrets exposed, no injection vectors. Recommended PAT scopes match the CLI's actual API calls.
Intent & coherence	✅ Scope matches issue #466. Sentinel → detection → hint wrapper design aligns with the forge abstraction architecture documented in AGENTS.md.
Style & conventions	✅ All new symbols follow established naming patterns (Err* / Is*() / is*Error()). Error wrapping uses %w correctly. Test patterns match existing table-driven style.
Documentation currency	✅ Token resolution order in docs matches resolveToken() code. Permissions table matches CLI operations. No stale references found.
Cross-repo contracts	✅ Not applicable — internal/forge/ cannot be imported externally per Go's internal package rules. All changes are additive. forge.Client interface unchanged.

Low-severity notes

[coverage-gap] A few secondary forge call sites (ListOrgInstallations at admin.go:1231, admin.go:1295, admin.go:1699; github.go:903) are not wrapped with classicPATHint. In practice, these are all downstream of a ListOrgRepos call that IS wrapped, so the first API call in any user flow will catch the classic PAT rejection and surface the hint before reaching these. Reasonable scope boundary for this PR.

---

Labels: PR fixes error UX gap (type/bug) and includes documentation updates

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:28 AM UTC · Completed 10:38 AM UTC
Commit: fa6bdf7 · View workflow run →

[guyoron1]

/fs-review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:29 PM UTC · Completed 12:38 PM UTC
Commit: fa6bdf7 · View workflow run →