fix: improve fine-grained PAT guidance in preflight and setup#3383
Conversation
When a fine-grained token is detected during preflight, the CLI now lists the specific scopes that could not be verified along with their equivalent fine-grained permissions, instead of a generic skip message. When an org blocks classic PATs and the GetRepo call returns a 403, the CLI now detects the specific error and prints actionable guidance: token resolution order, required fine-grained permissions, and the export command to use a fine-grained PAT. Adds token resolution and fine-grained permission documentation to docs/guides/getting-started/configuring-github.md. Closes #3368 Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
PR Summary by QodoImprove fine-grained PAT guidance in preflight and setup
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
|
🤖 Review · |
Site previewPreview: https://95e39e11-site.fullsend-ai.workers.dev Commit: |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Code Review by Qodo
1. Unnumbered procedural token step
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| Export it before running setup so it takes priority: | ||
|
|
||
| ```bash | ||
| export GH_TOKEN=github_pat_... | ||
| fullsend github setup <org>/<repo> ... | ||
| ``` |
There was a problem hiding this comment.
2. Unnumbered procedural token step 📜 Skill insight ✧ Quality
The added procedural instruction Export it before running setup... is not expressed as a numbered step, despite being a procedure. This violates the requirement that procedures be written as numbered steps instead of prose paragraphs.
Agent Prompt
## Issue description
Procedural guidance is introduced in prose (`Export it before running setup...`) without using a numbered list.
## Issue Context
Documentation procedures must be written as numbered steps.
## Fix Focus Areas
- docs/guides/getting-started/configuring-github.md[42-47]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
- Add SkippedReason to PreflightResult to distinguish installation tokens from fine-grained PATs; runPreflight now shows the correct message for each token type instead of always saying "fine-grained" - Extract printSkipGuidance helper to deduplicate the guidance-printing loop that appeared in both runPreflight and checkTokenScopes - Add Pull requests (read/write) to fineGrainedEquivalents so SkipGuidance output is consistent with patForbiddenGuidance and docs - Add test for patForbiddenGuidance verifying owner/repo, all permission bullets, token resolution order, and export command Assisted-by: Claude (review, fix), Gemini (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 4:47 PM UTC · Completed 5:01 PM UTC |
ReviewFindingsLow
Previous runLooks good to me Previous run (2)ReviewFindingsLow
Labels: PR modifies CLI install/setup flow and getting-started documentation |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
ready-for-merge
The token resolution details are not a prerequisite for most users. Collapse the section into a compact note so it does not dominate the prerequisites list. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
|
🤖 Review · |
- Explicitly match SkipFineGrained in switch instead of using default, preventing future SkipReason variants from getting wrong guidance - Guard SkipGuidance() to only generate output for fine-grained tokens, making it safe to call unconditionally - Extract handlePATForbidden helper to deduplicate PAT rejection handling at two GetRepo call sites - Set SkippedReason in checkTokenScopes when constructing PreflightResult - Add test for SkipGuidance returning empty for installation tokens Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 5:16 PM UTC · Completed 5:31 PM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
ready-for-merge
Fine-grained PATs are scoped to a single GitHub org and cannot create cross-org forks. When the CLI detects a fine-grained token (nil scopes from GetTokenScopes), it now hides the [f] fork option and presents only the upstream [u] path with a confirmation prompt explaining that a PR with scaffolding files will be created. - Add isFineGrainedToken() to detect token type via scope introspection - Add promptUpstreamOnly() for fine-grained-specific confirmation UX - Update commitScaffoldViaPR to route fine-grained tokens to upstream - Add fork limitation note to configuring-github.md - Add tests for fine-grained interactive/non-interactive/decline paths Relates-to: #3368 Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 9:00 PM UTC · Completed 9:17 PM UTC |
| case layers.SkipFineGrained: | ||
| printer.StepWarn("Preflight skipped: fine-grained token detected (scopes cannot be verified)") | ||
| printSkipGuidance(printer, result) | ||
| default: |
There was a problem hiding this comment.
[low] edge-case
The switch on result.SkippedReason uses a default case that prints a generic message without calling printSkipGuidance. If a new SkipReason variant is added and is semantically similar to fine-grained, the guidance would not be printed. Consider adding a code comment noting that new skip reasons should get explicit case handling.
| scaffoldCommitFn := func(ctx context.Context, owner, repo string, files []forge.TreeFile, direct bool) error { | ||
| targetRepo, repoErr := client.GetRepo(ctx, owner, repo) | ||
| if repoErr != nil { | ||
| if gh.IsPATForbiddenError(repoErr) { |
There was a problem hiding this comment.
[low] code-duplication
Classic PAT rejection handling appears identically at two call sites (~998 and ~1172). Both check gh.IsPATForbiddenError, call handlePATForbidden, and return the result. Consider extracting a shared helper that encapsulates the check-and-handle pattern.
| } | ||
| } | ||
|
|
||
| func handlePATForbidden(printer *ui.Printer, owner, repo string, err error) error { |
There was a problem hiding this comment.
[low] error-wrapping
The error message 'organization forbids classic PATs' in handlePATForbidden is somewhat redundant with the underlying error's own message from IsPATForbiddenError which contains 'forbids access via a personal access token'.
| } | ||
|
|
||
| func patForbiddenGuidance(owner, repo string) string { | ||
| var b strings.Builder |
There was a problem hiding this comment.
[low] naming-convention
patForbiddenGuidance returns a string but lacks a verb prefix (e.g., buildPATForbiddenGuidance) to distinguish it from side-effect functions like printSkipGuidance.
fullsend-ai-review
Bot
added
ready-for-merge
|
🤖 Finished Retro · ✅ Success · Started 5:17 PM UTC · Completed 5:27 PM UTC |
|
PR #3383 improved fine-grained PAT guidance in the CLI preflight and setup flow, closing issue #3368. The workflow involved 5 commits over ~4.5 hours by a human author (waynesun09, assisted by Claude), 5 review agent dispatches (2 cancelled, 3 completed), Qodo code review, and a human reviewer who approved with no comments. The multi-agent review system worked well: Qodo caught a real bug (installation token misclassification) within minutes, the author fixed it before the fullsend review agent's first successful run, and the fullsend agent provided valuable code quality feedback that the author addressed in targeted follow-up commits. The main waste was: (1) 2/5 review runs cancelled due to rapid pushes (existing issue #1355), and (2) all 4 inline findings in the final review run were repeats of Run 2 findings (existing issue #1013). The human reviewer approved with no comments ~17 hours after the last push, adding latency but no substantive feedback. Three proposals filed: evidence for #1013 (repeated findings), evidence for #1355 (cancelled reviews from rapid pushes), and a new proposal about tracking out-of-diff documentation findings that are lost after approval. Proposals filed
|
2 participants
Summary
When a fine-grained token is used, the CLI preflight now lists the specific scopes
that could not be verified and their equivalent fine-grained permissions, instead of
a generic "preflight skipped" message. When an org blocks classic PATs, the CLI
detects the specific 403 and prints actionable token resolution guidance.
Related Issue
Closes #3368
Changes
SkipGuidance()method toPreflightResultthat maps OAuth scopes tofine-grained equivalents and renders a guidance message
runPreflightandcheckTokenScopes) to printscope names and fine-grained equivalents when a fine-grained token is detected
IsPATForbiddenError()classifier in the GitHub forge to detectthe specific 403 when an org blocks classic PATs
patForbiddenGuidance()helper that prints token resolution order,required fine-grained permissions, and the export command
GetRepocall sites in per-repo setup to detect the PAT-forbiddenerror and show actionable guidance
docs/guides/getting-started/configuring-github.mdTesting
SkipGuidance()(3 subtests: per-repo scopes, install scopes, empty)IsPATForbiddenError()(5 subtests: match, wrapped, generic 403, non-API, nil)Checklist
!for breaking changes)