-
Notifications
You must be signed in to change notification settings - Fork 64
fix: improve fine-grained PAT guidance in preflight and setup #3383
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
248dd22
bf0b3c4
998e9a5
aa5d6f8
45ff7fe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -995,6 +995,9 @@ func runPerRepoInstall(ctx context.Context, c perRepoInstallConfig) error { | |
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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. |
||
| return handlePATForbidden(printer, owner, repo, repoErr) | ||
| } | ||
| return fmt.Errorf("getting repo info: %w", repoErr) | ||
| } | ||
| commitMsg := fmt.Sprintf("chore: initialize fullsend-%s per-repo installation", version) | ||
|
|
@@ -1166,6 +1169,9 @@ func applyPerRepoScaffold(ctx context.Context, client forge.Client, printer *ui. | |
|
|
||
| targetRepo, err := client.GetRepo(ctx, owner, repo) | ||
| if err != nil { | ||
| if gh.IsPATForbiddenError(err) { | ||
| return handlePATForbidden(printer, owner, repo, err) | ||
| } | ||
| return fmt.Errorf("getting repo info: %w", err) | ||
| } | ||
| commitMsg := fmt.Sprintf("chore: initialize fullsend-%s per-repo installation", version) | ||
|
|
@@ -2068,7 +2074,15 @@ func runPreflight(ctx context.Context, stack *layers.Stack, op layers.Operation, | |
| } | ||
|
|
||
| if result.Skipped { | ||
| printer.StepWarn("Preflight skipped: fine-grained token detected (scopes cannot be verified)") | ||
| switch result.SkippedReason { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] edge-case The switch on result.SkippedReason uses a default case for fine-grained token guidance. If a new SkipReason variant is added, it will silently fall into the default branch and print fine-grained PAT guidance, which may be incorrect. Consider explicitly matching SkipFineGrained with a generic default fallback. Suggested fix: Change the switch to explicitly handle SkipFineGrained and add a default case that prints a generic 'preflight skipped' message without fine-grained guidance. |
||
| case layers.SkipInstallationToken: | ||
| printer.StepWarn("Preflight skipped: installation token (OAuth scopes do not apply)") | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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. |
||
| printer.StepWarn(fmt.Sprintf("Preflight skipped: %s", result.SkippedReason)) | ||
| } | ||
|
qodo-code-review[bot] marked this conversation as resolved.
|
||
| } else { | ||
| printer.StepDone("Token permissions verified") | ||
| } | ||
|
|
@@ -2879,6 +2893,7 @@ func checkTokenScopes(ctx context.Context, client forge.Client, printer *ui.Prin | |
|
|
||
| if granted == nil { | ||
| printer.StepWarn("Preflight skipped: fine-grained token detected (scopes cannot be verified)") | ||
| printSkipGuidance(printer, &layers.PreflightResult{Required: required, Skipped: true, SkippedReason: layers.SkipFineGrained}) | ||
| return nil | ||
| } | ||
|
|
||
|
|
@@ -2910,6 +2925,47 @@ func checkTokenScopes(ctx context.Context, client forge.Client, printer *ui.Prin | |
| return nil | ||
| } | ||
|
|
||
| // printSkipGuidance prints fine-grained permission guidance when | ||
| // preflight is skipped due to a fine-grained token. | ||
| func printSkipGuidance(printer *ui.Printer, result *layers.PreflightResult) { | ||
| if guidance := result.SkipGuidance(); guidance != "" { | ||
| for _, line := range strings.Split(guidance, "\n") { | ||
| if line != "" { | ||
| printer.StepInfo(line) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] naming-convention patForbiddenGuidance returns a string but lacks a verb prefix (e.g., buildPATForbiddenGuidance) to distinguish it from side-effect functions like printSkipGuidance. |
||
| func handlePATForbidden(printer *ui.Printer, owner, repo string, err error) error { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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'. |
||
| printer.StepFail("Classic PAT rejected by organization") | ||
| printer.Blank() | ||
| printer.ErrorBox("Token not accepted", patForbiddenGuidance(owner, repo)) | ||
| return fmt.Errorf("organization forbids classic PATs: %w", err) | ||
| } | ||
|
|
||
| func patForbiddenGuidance(owner, repo string) string { | ||
| var b strings.Builder | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] naming-convention patForbiddenGuidance returns a string but lacks a verb prefix (e.g., buildPATForbiddenGuidance) to distinguish it from side-effect functions like printSkipGuidance. |
||
| fmt.Fprintf(&b, "Organization %q forbids classic personal access tokens.\n\n", owner) | ||
| b.WriteString("The CLI resolves tokens in this order:\n") | ||
| b.WriteString(" 1. GH_TOKEN environment variable\n") | ||
| b.WriteString(" 2. GITHUB_TOKEN environment variable\n") | ||
| b.WriteString(" 3. gh auth token (GitHub CLI)\n\n") | ||
| b.WriteString("The token that resolved was rejected. To fix this, create a\n") | ||
| b.WriteString("fine-grained PAT at https://github.com/settings/personal-access-tokens/new\n") | ||
| fmt.Fprintf(&b, "scoped to %s/%s with these permissions:\n\n", owner, repo) | ||
| b.WriteString(" • Contents: read/write\n") | ||
| b.WriteString(" • Workflows: read/write\n") | ||
| b.WriteString(" • Secrets: read/write\n") | ||
| b.WriteString(" • Variables: read/write\n") | ||
| b.WriteString(" • Pull requests: read/write (without --direct)\n") | ||
| b.WriteString(" • Metadata: read-only (required by GitHub)\n\n") | ||
| b.WriteString("Then export it before running setup:\n") | ||
| fmt.Fprintf(&b, " export GH_TOKEN=github_pat_...\n") | ||
| fmt.Fprintf(&b, " fullsend github setup %s/%s", owner, repo) | ||
| return b.String() | ||
| } | ||
|
|
||
| // Helper functions. | ||
|
|
||
| func repoNameList(repos []forge.Repository) []string { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[low] code-duplication
Classic PAT rejection handling appears identically at two call sites (~998 and ~1174). Both check gh.IsPATForbiddenError, print the same UI messages (StepFail, Blank, ErrorBox with patForbiddenGuidance), and return the same error. Consider extracting a shared helper.
Suggested fix: Extract the PAT rejection handling into a helper function like handlePATForbiddenError(printer, owner, repo, err) that can be called from both locations.