Skip to content

feat(cli): add optional GitHub token support and actionable rate-limit guidance#186

Open
cauyxy wants to merge 2 commits intonextlevelbuilder:mainfrom
cauyxy:main
Open

feat(cli): add optional GitHub token support and actionable rate-limit guidance#186
cauyxy wants to merge 2 commits intonextlevelbuilder:mainfrom
cauyxy:main

Conversation

@cauyxy
Copy link
Copy Markdown

@cauyxy cauyxy commented Mar 16, 2026

Motivation

GitHub API rate limits can sometimes cause failures when listing or downloading releases.
This PR adds optional authentication support and clearer guidance to help users resolve rate-limit issues.

Changes

  • Added optional --token <token> to:
    • uipro init
    • uipro versions
    • uipro update
  • Pass token through command handlers and GitHub request helpers to set the Authorization header.
  • Improved rate-limit messaging:
    • Centralized guidance in getGitHubTokenGuidance()
    • Included guidance in 403 / 429 responses
    • Added init fallback warning before template fallback
  • Updated README with:
    • --token usage examples
    • UI_PRO_MAX_GITHUB_TOKEN environment variable
    • Instructions for creating a GitHub PAT and suggested minimum scope

Behavior

  • Token usage is optional; existing workflows continue to work without it.
  • Default install remains template-based.
  • ZIP download mode is available via --legacy.

Validation

  • CLI builds successfully (bun run build)
  • Verified command wiring and documentation updates through source inspection

cauyxy and others added 2 commits March 16, 2026 18:59
Copy link
Copy Markdown

@accesspath27-lgtm accesspath27-lgtm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: feat(cli): add optional GitHub token support and rate-limit guidance

Solid PR — clean implementation with good factoring (resolveGitHubToken(), createGitHubHeaders(), getGitHubTokenGuidance()). Backward compatible, everything optional.

Minor notes:

  1. --token on init only matters with --legacy — worth documenting this so users aren't confused when uipro init --token <t> does nothing in default template mode
  2. README alignment is slightly ragged (tabs vs spaces in examples)
  3. Control flow in error handlingGitHubRateLimitError catch works because process.exit terminates, but an else if would be clearer

None are blockers. Good work.

@mrgoonie
Copy link
Copy Markdown
Contributor

@claude review

@claude
Copy link
Copy Markdown

claude bot commented Mar 24, 2026

Claude finished @mrgoonie's task in 2m 26s —— View job


PR Review: feat(cli): add optional GitHub token support and actionable rate-limit guidance

  • Gather context and understand the request
  • Read changed files
  • Analyze code quality, security, and correctness
  • Post review feedback

Overall: This is a well-motivated, backward-compatible addition. The centralized helpers (resolveGitHubToken(), createGitHubHeaders(), getGitHubTokenGuidance()) show good factoring. That said, there are a few issues worth addressing before merge — one security concern and a few UX/correctness gaps.


Security: --token exposes secrets in shell history and process list

Accepting a PAT via CLI flag (--token <token>) is a common pattern but comes with real risks:

  • The token appears in ~/.bash_history / ~/.zsh_history
  • It's visible in ps aux while the process runs
  • CI logs can accidentally capture it

The UI_PRO_MAX_GITHUB_TOKEN env var is the right approach for secrets. The --token flag should carry a clear warning in README docs — something like:

⚠️ Avoid --token in CI; prefer the UI_PRO_MAX_GITHUB_TOKEN environment variable to prevent the token from appearing in logs or shell history.

Fix this →


--token is a no-op on init without --legacy

The default init command uses template generation (templateInstall) — no GitHub API calls are made. The token is only consumed inside tryGitHubInstall(), which is only called when options.legacy is true.

So uipro init --token <t> silently ignores the token. The README and --help output should note this explicitly:

--token <token>   GitHub PAT for API auth (only used with --legacy)

This was flagged by the previous reviewer and is the most confusing UX issue in the PR.

Fix this →


Empty string token is not guarded

If a user runs uipro init --token "", resolveGitHubToken() would return an empty string and createGitHubHeaders() would produce Authorization: token — an invalid header that could cause unexpected 401 responses. A simple guard would fix this:

// in resolveGitHubToken()
const token = cliToken ?? process.env.UI_PRO_MAX_GITHUB_TOKEN;
return token && token.trim().length > 0 ? token.trim() : undefined;

Fix this →


versions command wiring needs verification

Currently index.ts:46 wires versions as:

.action(versionsCommand);

After the PR, versionsCommand would accept an options object. Commander passes the Command object as the last argument when using direct .action(fn) — this could cause options.token to be undefined even when --token is passed, or cause the function signature to misinterpret Commander's injected argument. The wiring should be:

.action(async (options) => {
  await versionsCommand(options);
});

Fix this →


Consider falling back to GITHUB_TOKEN

GITHUB_TOKEN is automatically set in GitHub Actions environments. Supporting it alongside UI_PRO_MAX_GITHUB_TOKEN would let CI users get authenticated API calls for free without extra config:

const token = cliToken 
  ?? process.env.UI_PRO_MAX_GITHUB_TOKEN 
  ?? process.env.GITHUB_TOKEN;

This is a minor enhancement, not a blocker.


403 error messaging — distinguish auth failure from rate limit

The current checkRateLimit() correctly gates on remaining === '0' for 403 — so it won't misattribute auth failures as rate limits. But the PR adds getGitHubTokenGuidance() to 403 responses. If a 403 happens due to a wrong token scope (not rate limiting), showing rate-limit guidance could mislead users. Worth adding a note in the error message distinguishing the two cases.


Minor: control flow (else if)

As noted by the previous reviewer — in the GitHubRateLimitError catch block, using else if instead of relying on process.exit for control flow makes intent clearer. Low priority.


Summary

Issue Severity
--token exposes PAT in shell history Medium
--token is a no-op on init without --legacy Medium (UX)
Empty string token creates invalid auth header Low
versions command wiring may be incorrect Medium (correctness)
Fallback to GITHUB_TOKEN env var Low (enhancement)
403 messaging conflates rate limit with auth failure Low
else if control flow Nit

The core approach is sound — this is close to merge-ready. The wiring issue and the --token/--legacy UX gap are the most important items to address.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants