Conversation
- GitHubService now supports both authenticated and unauthenticated modes - Added authMode property and isAuthenticated() method to service - Added requireAuth() method that throws GitHubAuthenticationError for write ops - Plugin init no longer throws when GITHUB_TOKEN is missing - Actions that require auth check isGitHubAuthenticated() in validate() - Added githubStatusProvider to give agent context about capabilities - Improved error messages for 403/404 to suggest token when in unauth mode - Rate limit errors now show reset time and mode-specific limits Unauthenticated mode (60 req/hr): view public repos/issues/PRs, search Authenticated mode (5000 req/hr): full access including private repos and writes
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughIntroduces optional GitHub authentication, persistent room-aware GitHub state, extensive provider refactor/additions, a PR split workflow with working-copy management, token-aware action/provider gating, new utilities (context management, shell exec, state persistence), banner output, and widespread branding/readme updates. Changes
Sequence DiagramsequenceDiagram
participant Agent as Agent Runtime
participant Action as Action Handler
participant GH as GitHub Service
participant State as State Persistence
participant Git as Local Git (working copy)
participant LLM as Language Model
Agent->>Action: invoke PR split or repo action
Action->>GH: isAuthenticated? / requireAuth()
GH-->>Action: auth status (authenticated/unauthenticated)
alt Authenticated
Action->>GH: fetch PR / repo data
GH-->>Action: PR/repo response
Action->>LLM: request analysis / split suggestions
LLM-->>Action: JSON plan/suggestions
Action->>State: mergeAndSaveGitHubState(plan, analysis)
State-->>Action: persisted
Action->>Git: clone/update working copy (gitExec)
Git-->>Action: local repo ready
Action->>Git: create branches / apply splits
Git-->>Action: result
Action->>GH: create PRs / push branches (if required)
GH-->>Action: result
Action-->>Agent: aggregated result
else Unauthenticated
Action-->>Agent: limited mode or auth-required error
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This pull request adds unauthenticated mode support to the GitHub plugin, allowing the plugin to operate without a GitHub token for public-only operations with reduced rate limits (60 requests/hour vs 5000 requests/hour when authenticated).
Changes:
- Added support for unauthenticated mode with automatic detection based on token presence
- Implemented state persistence utilities for daemon restarts and cross-action state sharing
- Added new context providers and actions for PR splitting, working copies, and enhanced feedback aggregation
Reviewed changes
Copilot reviewed 34 out of 35 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/services/github.ts | Added authMode property, isAuthenticated() and requireAuth() methods, improved error messages for unauthenticated mode |
| src/utils/state-persistence.ts | New utility for persisting GitHub state to cache across daemon restarts |
| src/utils/context-management.ts | New utility for managing context token budgets and preventing provider bloat |
| src/providers/*.ts | Split monolithic github.ts provider into separate files, added new providers for status, working copies, webhooks, etc. |
| src/actions/prSplit.ts | New actions for analyzing and splitting large PRs into smaller focused PRs |
| src/actions/repository.ts | Added clone, list, and delete working copy actions |
| src/index.ts | Added token filtering logic, banner integration, dependency updates |
| src/banner.ts | New startup banner showing GitHub configuration status |
| package.json | Changed dependencies from direct ngrok to @elizaos/plugin-ngrok and @elizaos/plugin-git |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/actions/prSplit.ts
Outdated
| const currentCommit = commitOut.trim(); | ||
|
|
||
| // Persist to state | ||
| await mergeAndSaveGitHubState(runtime, message.roomId, { |
There was a problem hiding this comment.
The function signature has parameters in the wrong order. The function is defined as mergeAndSaveGitHubState(runtime, newState, roomId?) but it's being called throughout the codebase with mergeAndSaveGitHubState(runtime, roomId, newState). This will cause runtime errors where roomId is treated as the state object and newState is treated as the roomId.
src/actions/prSplit.ts
Outdated
| await mergeAndSaveGitHubState(runtime, message.roomId, { | ||
| lastPrSplitPlan: confirmedPlan, | ||
| }); |
There was a problem hiding this comment.
Inconsistent parameter order for mergeAndSaveGitHubState calls. In this file, the function is called with (runtime, message.roomId, {...}) pattern (roomId before state), but in other files like issues.ts, repository.ts, and pullRequests.ts, it's called with (runtime, {...}, message.roomId) pattern (state before roomId). The function definition is mergeAndSaveGitHubState(runtime, newState, roomId?), so the calls in this file are incorrect - they pass roomId as the state parameter.
| await mergeAndSaveGitHubState(runtime, message.roomId, { | |
| lastPrSplitPlan: confirmedPlan, | |
| }); | |
| await mergeAndSaveGitHubState(runtime, { | |
| lastPrSplitPlan: confirmedPlan, | |
| }, message.roomId); |
| const outdatedThreads = reviewComments.filter((c: any) => | ||
| !c.in_reply_to_id && c.position === null | ||
| ).length; | ||
| const activeThreads = codeThreads - outdatedThreads; |
There was a problem hiding this comment.
Unused variable activeThreads.
| const activeThreads = codeThreads - outdatedThreads; |
| try { | ||
| const persistedState = await loadGitHubState(runtime, message.roomId); | ||
| const splitPlan = persistedState?.lastPrSplitPlan as PrSplitPlan | undefined; | ||
| const analysis = persistedState?.lastPrAnalysis as PrAnalysis | undefined; |
There was a problem hiding this comment.
Unused variable analysis.
| const analysis = persistedState?.lastPrAnalysis as PrAnalysis | undefined; |
| truncateToTokens, | ||
| summarizeComment, | ||
| formatCommentDetail, | ||
| formatListWithBudget, |
There was a problem hiding this comment.
Unused import formatListWithBudget.
| formatListWithBudget, |
| parseContextOptions, | ||
| getTokenBudget, | ||
| truncateToTokens, | ||
| formatListWithBudget, |
There was a problem hiding this comment.
Unused import formatListWithBudget.
| formatListWithBudget, |
| * Filter actions based on token availability. | ||
| */ | ||
| function filterActionsByToken(actions: Action[], hasToken: boolean): Action[] { | ||
| return actions.filter((action) => { | ||
| const requirement = actionTokenRequirements[action.name] || "enhanced"; | ||
| if (requirement === "required" && !hasToken) { | ||
| logger.debug(`[GitHub] Disabling action ${action.name} - requires token`); | ||
| return false; | ||
| } | ||
| return true; | ||
| }); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
Unused function filterActionsByToken.
| * Filter actions based on token availability. | |
| */ | |
| function filterActionsByToken(actions: Action[], hasToken: boolean): Action[] { | |
| return actions.filter((action) => { | |
| const requirement = actionTokenRequirements[action.name] || "enhanced"; | |
| if (requirement === "required" && !hasToken) { | |
| logger.debug(`[GitHub] Disabling action ${action.name} - requires token`); | |
| return false; | |
| } | |
| return true; | |
| }); | |
| } | |
| /** |
| * Filter providers based on token availability. | ||
| */ | ||
| function filterProvidersByToken(providers: Provider[], hasToken: boolean): Provider[] { | ||
| return providers.filter((provider) => { | ||
| const requirement = providerTokenRequirements[provider.name] || "none"; | ||
| if (requirement === "required" && !hasToken) { | ||
| logger.debug(`[GitHub] Disabling provider ${provider.name} - requires token`); | ||
| return false; | ||
| } | ||
| return true; | ||
| }); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
Unused function filterProvidersByToken.
| * Filter providers based on token availability. | |
| */ | |
| function filterProvidersByToken(providers: Provider[], hasToken: boolean): Provider[] { | |
| return providers.filter((provider) => { | |
| const requirement = providerTokenRequirements[provider.name] || "none"; | |
| if (requirement === "required" && !hasToken) { | |
| logger.debug(`[GitHub] Disabling provider ${provider.name} - requires token`); | |
| return false; | |
| } | |
| return true; | |
| }); | |
| } | |
| /** |
src/actions/repository.ts
Outdated
| shallow, | ||
| }; | ||
|
|
||
| const existingState = await loadGitHubState(runtime, message.roomId); |
There was a problem hiding this comment.
The value assigned to existingState here is unused.
| const existingState = await loadGitHubState(runtime, message.roomId); |
| localPath: repoDir, | ||
| branch: currentBranch, | ||
| commit: currentCommit, | ||
| clonedAt: operation === "clone" ? new Date().toISOString() : (existingState?.workingCopies?.[`${owner}/${repo}`]?.clonedAt || new Date().toISOString()), |
There was a problem hiding this comment.
Variable 'existingState' is used before its declaration.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| const outdatedThreads = reviewComments.filter((c: any) => | ||
| !c.in_reply_to_id && c.position === null | ||
| ).length; | ||
| const activeThreads = codeThreads - outdatedThreads; |
There was a problem hiding this comment.
Unused variable calculated but never referenced
Low Severity
The activeThreads variable is calculated as codeThreads - outdatedThreads but is never used anywhere in the function. The threadInfo string and return values use discussionThreads, codeThreads, and outdatedThreads, but activeThreads is completely unreferenced. This appears to be leftover code from incomplete development - the intent may have been to show active vs outdated threads but only the calculation was added.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
package.json (1)
86-104:GITHUB_TOKENmarked as required but PR supports unauthenticated mode.The
agentConfig.pluginParameters.GITHUB_TOKEN.requiredis set totrue, but the PR objectives state the plugin now supports unauthenticated mode for public API access. This metadata inconsistency could confuse users or tooling that relies on this configuration.Consider updating to
required: falsewith an updated description explaining the two modes.Suggested fix
"agentConfig": { "pluginType": "elizaos:plugin:1.0.0", "pluginParameters": { "GITHUB_TOKEN": { "type": "string", - "description": "GitHub Personal Access Token or Fine-grained token", - "required": true + "description": "GitHub Personal Access Token or Fine-grained token. Optional for read-only access to public repositories (60 requests/hour). Required for write operations and private repos (5,000 requests/hour).", + "required": false },src/actions/autoCoder.ts (1)
219-223: Inconsistent parameter naming:max_tokensvsmaxTokens.Lines 91 and 637 use
maxTokens, but this invocation and line 299 still usemax_tokens. This inconsistency could cause issues if the runtime expects a specific parameter name.Suggested fix
const analysisResponse = await runtime.useModel(ModelType.TEXT_LARGE, { prompt: analysisPrompt, temperature: 0.2, - max_tokens: 1000, + maxTokens: 1000, });And similarly at line 299:
const codeResponse = await runtime.useModel(ModelType.TEXT_LARGE, { prompt: codeGenPrompt, temperature: 0.1, - max_tokens: 3000, + maxTokens: 3000, });src/actions/webhooks.ts (1)
188-193: Inconsistent state access pattern.Line 190 checks
state.github?.lastRepositorybut lines 191-192 accessstate.data?.github?.lastRepository. This inconsistency appears in multiple handlers (lines 385-387, 516-518, 638-640) and could lead to missed fallback values.🐛 Proposed fix for createWebhookAction
if (!owner || !repo) { // Try to get from current state - if (state.github?.lastRepository) { - owner = owner || state.data?.github?.lastRepository?.owner?.login; - repo = repo || state.data?.github?.lastRepository?.name; + const lastRepo = state.data?.github?.lastRepository || state.github?.lastRepository; + if (lastRepo) { + owner = owner || lastRepo.owner?.login; + repo = repo || lastRepo.name; } else {src/services/github.ts (1)
154-163: Headers are at the response root level, not in thedataobject.
OnlyvalidateAuthentication()uses the broken pattern; all other octokit calls in the file correctly destructureheadersfrom the response root. This causes rate limits to silently fail to update here sincedata.headersis undefined and the fallback empty object is used instead.🔧 Suggested fix
- const { data } = await this.octokit.users.getAuthenticated(); - this.updateRateLimit((data as any)?.headers || {}); + const { data, headers } = await this.octokit.users.getAuthenticated(); + this.updateRateLimit(headers);
🤖 Fix all issues with AI agents
In `@README.md`:
- Around line 109-116: The fenced code block containing the example prompts
(lines starting with "Get information about the elizaOS/eliza repository",
"Create a new repository called my-awesome-project", etc.) is missing a language
specifier; update the opening fence from ``` to ```text (or remove the fences)
so the block is annotated as plain text to satisfy MD040—modify the block that
wraps those quoted example strings accordingly.
- Around line 9-14: Update the README table to mark GITHUB_TOKEN as optional
(change `GITHUB_TOKEN` Required from **Yes** to No) and add a brief
parenthetical note in the Description column that the plugin supports
unauthenticated mode for public API access with a 60 requests/hour rate limit;
reference the existing variable name `GITHUB_TOKEN` and include a short sentence
clarifying when a token is needed (e.g., for higher rate limits or private repo
access).
In `@src/actions/issues.ts`:
- Around line 39-46: The current validation in src/actions/issues.ts uses
hasIssueUrl to only match full GitHub URLs and will miss shorthand patterns;
update the regex used for hasIssueUrl (which checks the variable text) to a
combined pattern that accepts full URLs (github.com/owner/repo/issues/123),
owner/repo with an issue marker (owner/repo#123 or owner/repo issue `#123`), and
plain "issue `#123`" when accompanied by an owner/repo mention; modify the single
test(/.../) expression for hasIssueUrl accordingly so the handler's accepted
patterns (shorthand owner/repo notation and issue number extraction) pass
validation while still rejecting unrelated text.
In `@src/actions/prSplit.ts`:
- Around line 193-208: The call to mergeAndSaveGitHubState is using the wrong
argument order (passing roomId as the second argument), so newState is being set
incorrectly; update all call sites (e.g., the call near where runtime and
message.roomId are used) to call mergeAndSaveGitHubState(runtime, newState,
roomId) instead of mergeAndSaveGitHubState(runtime, roomId, newState) —
specifically fix the call in prSplit.ts (the block creating workingCopies with
owner/repo/branch/commit/prNumber/prTitle) and the other listed sites so runtime
remains first, the new state object (containing workingCopies and
lastPullRequest) is passed second, and message.roomId (or roomId) is passed
third.
- Around line 1073-1078: The validate function currently sets hasPlanContext to
always true because of the trailing "|| true"; remove "|| true" from
hasPlanContext in the validate method so it actually checks for plan-related
keywords, and tighten the regex to reliably detect planning context (e.g.,
/\b(split|plan|pr|changes|diff|proposal)\b/i) while keeping the confirm-intent
check (hasConfirmIntent) intact so the action only triggers when both confirm
intent and a true plan context are present; ensure you reference the validate
function, hasPlanContext variable, and message.content.text when making this
change.
- Around line 994-1026: Normalize and validate parsed.updatedSuggestions before
building updatedPlan and calling mergeAndSaveGitHubState: iterate
parsed.updatedSuggestions (used to create updatedPlan in the prSplit flow),
ensure each suggestion has required fields (name:string, description:string,
priority:number, files:array, dependencies:array) and coerce or set safe
defaults (e.g., [] for files/dependencies, 999 for missing priority, empty
string for name/description); replace parsed.updatedSuggestions with the
normalized array and use that normalized array when constructing updatedPlan and
when sorting/printing so downstream rendering (Files:
suggestion.files.slice(...), sorting by priority) never encounters undefined or
non-array values. Ensure normalization happens before creating updatedPlan and
before the for-loop that generates responseText.
- Around line 568-610: The parsed LLM output may omit or malform fields, so
before constructing PrSplitPlan and before using splitPlan.suggestions you must
normalize and validate the suggestions array: ensure parsed.suggestions is an
array (default to []), map each suggestion to a safe shape with defaults for
name (""), description (""), priority (Number.MAX_SAFE_INTEGER or incremental),
files (empty array), dependencies (empty array), and coerce types (e.g., files
and dependencies to arrays, priority to number); set splitPlan.rationale =
String(parsed.rationale || ""); replace direct .length and .sort usages with the
normalized suggestions (the variable used to build splitPlan.suggestions) and
then save via mergeAndSaveGitHubState and continue formatting using the
sanitized splitPlan.suggestions to avoid runtime errors in the loop and sorting.
- Around line 141-187: The code embeds the GitHub token in clone URLs and builds
shell-interpolated commands via execAsync, risking credential leakage and
command injection; replace usages of execAsync/cloneCmd (created from exec +
promisify) with child_process.execFile or spawn that accept argument arrays and
set git config http.extraheader for authentication (use a Base64 of
"x-access-token:<token>" as the Authorization header) when token present; update
the paths/branches operations (where alreadyCloned, headRepo, headBranch,
repoDir, getWorkingCopyPath are used) to pass branch and repo path as arguments
instead of interpolating into command strings and run commands with cwd set to
repoDir so no credentials end up in .git/config or shell history.
- Around line 762-779: The git commands currently build shell strings with
untrusted model output (split.files and split.description) in execAsync;
validate that split.files is a non-empty array and sanitize/escape or — better —
switch execAsync usage to an API that accepts argv (e.g., spawn/execFile) so you
pass file paths and commit message as separate arguments instead of
interpolating into a shell, and ensure you also pass analysis.headBranch and
branchName as argv rather than concatenating; also handle/normalize/validate
each filename (no path traversal, no shell metacharacters) before using it and
construct git add/checkout/commit arguments as arrays to avoid shell
interpretation.
In `@src/actions/repository.ts`:
- Around line 867-881: The code uses existingState inside the workingCopyData
object before existingState is declared, causing a TDZ/ReferenceError; fix by
moving the await loadGitHubState(runtime, message.roomId) call (the
existingState declaration) to before you build workingCopyData, then construct
workingCopyData (which references existingState) and finally call
mergeAndSaveGitHubState as before; ensure you keep the same variable names
(existingState, workingCopyData) and preserve the ternary that reads
existingState?.workingCopies?.[`${owner}/${repo}`]?.clonedAt.
- Around line 751-785: Validate and sanitize the untrusted owner and repo before
using them to build repoDir: enforce a strict regex (e.g., allow only
lowercase/uppercase letters, digits, hyphen, underscore, dot) for owner and repo
and throw if they contain disallowed characters or are empty; then construct the
target path by resolving via path.resolve(getReposDir(), owner, repo) (or
resolve(getReposDir(), ...owner.split(path.sep), repo) after stripping leading
slashes) and verify the resolved repoDir startsWith the resolved getReposDir()
(use path.sep normalization or compare path.resolve(getReposDir()) as prefix) to
prevent path traversal; if the check fails, throw an error and do not call
getWorkingCopyPath()/fs.rm/other filesystem ops. Reference symbols: owner, repo,
getReposDir, getWorkingCopyPath, repoDir, fs.rm, path.resolve, path.sep.
- Around line 771-848: The clone/pull logic currently uses execAsync
(promisified exec) with shell-interpolated command strings exposing shell
injection via user-controlled owner/repo/branch; replace the child_process
import and promisified exec with execFile (or promisify(execFile)) and call git
as an executable with argv arrays for commands used in this file (replace uses
in cloneCmd/fallbackCmd, git fetch/checkout/pull/reset) so user values are
passed as args not interpolated into a shell, and ensure you pass cwd in the
execFile options; also move the existingState declaration to before the creation
of workingCopyData so workingCopyData does not reference existingState before
it’s defined (look for symbols execAsync/cloneCmd/fallbackCmd and
workingCopyData/existingState).
In `@src/banner.ts`:
- Around line 73-96: The banner currently always marks GITHUB_TOKEN as required;
change the settings array construction (the settings constant that includes the
GITHUB_TOKEN entry) so the required flag for GITHUB_TOKEN is computed from the
app's auth mode instead of hard-coded—e.g., compute a boolean (like
githubTokenRequired) from your existing auth/public-only indicator (use your
existing isPublicOnly / isUnauthenticated flag or function) and set { name:
'GITHUB_TOKEN', value: token as string, isSecret: true, required:
githubTokenRequired }; leave other entries (GITHUB_OWNER, GITHUB_WEBHOOK_SECRET)
and the use of formatSettingValue untouched so the banner reflects the correct
required status.
In `@src/providers/activity.ts`:
- Around line 92-111: The rate limit usage calculation in the activity provider
can divide by zero (when computing usage = Math.round((rateLimit.used /
rateLimit.limit) * 100)); update the block that reads githubState.rateLimit to
guard against rateLimit.limit === 0 by computing usage only when limit > 0
(e.g., set usage = 0 or an appropriate sentinel when limit is 0), and ensure
values.rateLimitUsage and any contextText string interpolation use this safe
usage value; look for identifiers rateLimit, usage, githubState,
values.rateLimitUsage, contextText, and data.rateLimit to apply the change.
In `@src/providers/rateLimit.ts`:
- Around line 27-29: Guard against division-by-zero when computing usedPercent
from githubService.getRateLimit(): check rateLimit.limit (from the rateLimit
object) and if it's missing or <= 0, set usedPercent to 0 (or a safe fallback)
instead of performing the division; otherwise compute usedPercent as
Math.round((rateLimit.used / rateLimit.limit) * 100). Also ensure any downstream
consumer of usedPercent handles the fallback consistently. Reference: rateLimit,
usedPercent, resetTime, and githubService.getRateLimit().
In `@src/services/github.ts`:
- Around line 65-67: checkRateLimit and getRateLimitStatus assume a fixed 5000
limit and use a hardcoded "< 100" threshold, which breaks unauthenticated flows
(octokit with rateLimitRemaining default 60). Update both methods to read the
actual x-ratelimit-limit header from Octokit responses and store it (introduce
or update a rateLimitLimit property if needed), compute the threshold
dynamically (e.g., threshold = Math.min(100, Math.floor(rateLimitLimit * 0.02))
or similar) and compute used as rateLimitLimit - rateLimitRemaining; ensure
checkRateLimit uses rateLimitLimit and rateLimitRemaining/rateLimitReset values
to decide waiting and that getRateLimitStatus returns accurate limit, remaining,
used, and reset based on those tracked headers rather than hardcoded values
(modify functions checkRateLimit and getRateLimitStatus and any response-parsing
logic where headers are read).
In `@src/utils/context-management.ts`:
- Around line 56-63: The parser's regex maps the token "full" to "detail",
making the ContextMode value "full" unreachable; update the detection logic in
the block that sets options.mode (using variable lower and options.mode) so that
"full" is matched to options.mode = "full" (e.g., separate the full/all/verbose
pattern from the detail pattern) while leaving minimal and summary branches
intact; ensure the regexes for detail (e.g., "detailed", "details") do not
include "full" so ContextMode can be set to "full" where appropriate.
In `@src/utils/state-persistence.ts`:
- Around line 196-226: mergeAndSaveGitHubState currently deep-merges
repositories/issues/pullRequests but overwrites other map-like fields causing
workingCopies and prFeedbackCache to be lost; update mergedState inside
mergeAndSaveGitHubState to deep-merge workingCopies and prFeedbackCache (e.g., {
...existing.workingCopies, ...newState.workingCopies }) and ensure sensible
defaults (empty objects) so existing entries are preserved while new entries
override.
🧹 Nitpick comments (11)
src/providers/webhooks.ts (1)
58-60: Include error details in debug log.
Helps diagnose state-load failures without changing behavior.Proposed diff
- } catch (error) { - logger.debug("[GitHub] Could not load webhooks context"); + } catch (error) { + logger.debug({ error }, "[GitHub] Could not load webhooks context"); return { text: "", values: {}, data: {} }; }src/providers/branches.ts (1)
70-72: Include the caught error in the debug log.
It makes troubleshooting persistence failures easier.🛠️ Proposed tweak
- logger.debug("[GitHub] Could not load branch protection context"); + logger.debug({ error }, "[GitHub] Could not load branch protection context");src/providers/prSplitPlan.ts (1)
33-41: Prefer in-memory state before loading persistence.
This avoids stale context and unnecessary cache reads when runtime state already has the plan.♻️ Proposed update
- _state: State | undefined, + state: State | undefined, @@ - const persistedState = await loadGitHubState(runtime, message.roomId); - const splitPlan = persistedState?.lastPrSplitPlan; - const analysis = persistedState?.lastPrAnalysis; + let githubState = state?.github; + if (!githubState) { + githubState = await loadGitHubState(runtime, message.roomId); + } + const splitPlan = githubState?.lastPrSplitPlan; + const analysis = githubState?.lastPrAnalysis;src/providers/user.ts (1)
22-34: Short-circuit when unauthenticated to avoid noisy errors.
In unauthenticated mode this will throw every time; an early return keeps logs clean.🔧 Proposed guard
const githubService = runtime.getService<GitHubService>("github"); if (!githubService) { return { text: "", values: {}, data: {}, }; } + + if (!githubService.isAuthenticated()) { + return { + text: "GitHub user information unavailable (unauthenticated)", + values: {}, + data: {}, + }; + }src/providers/repository.ts (2)
47-68: Consider defensive checks for nested object properties.When accessing
repo.owner.loginon line 59, there's no null check forrepo.owner. IflastRepositoryexists but has a malformed structure (e.g., missingowner), this would throw a runtime error.🛡️ Suggested defensive access
values.currentRepository = repo.full_name; - values.repositoryOwner = repo.owner.login; + values.repositoryOwner = repo.owner?.login; values.repositoryName = repo.name;
74-92: Type assertion may hide runtime issues.The cast
as GitHubRepository[]on line 76 assumes all values ingithubState.repositoriesconform toGitHubRepository. If the persisted state contains malformed or partial data, this could cause runtime errors when accessing properties likeupdated_atorfull_name.Consider adding a filter or validation step to ensure data integrity.
src/actions/issues.ts (2)
19-25: Duplicated helper function - consider importing from shared location.The
isGitHubAuthenticatedfunction is defined locally here, but an identical version exists insrc/index.ts(lines 365-368) and is exported. This duplication creates maintenance burden if the logic needs to change.♻️ Proposed fix
import { GitHubService, type GitHubIssue, type CreateIssueOptions, + isGitHubAuthenticated, } from "../index"; import { mergeAndSaveGitHubState } from "../utils/state-persistence"; -/** - * Check if the GitHub service is available and authenticated. - */ -function isGitHubAuthenticated(runtime: IAgentRuntime): boolean { - const githubService = runtime.getService<GitHubService>("github"); - return githubService?.isAuthenticated() ?? false; -}
670-674: Minor formatting inconsistency.Line 672 has inconsistent indentation compared to surrounding code.
🧹 Proposed formatting fix
const repoName = issue.html_url ? issue.html_url.match(/github\.com\/([^\/]+\/[^\/]+)/)?.[1] || - "unknown" + "unknown" : "unknown";src/actions/webhooks.ts (1)
13-19: Duplicated helper function - same issue as in issues.ts.The
isGitHubAuthenticatedfunction is duplicated here and insrc/actions/issues.ts. Import fromsrc/index.tsinstead.♻️ Proposed fix
import { GitHubService } from "../services/github"; +import { isGitHubAuthenticated } from "../index"; import { z } from "zod"; -/** - * Check if the GitHub service is available and authenticated. - */ -function isGitHubAuthenticated(runtime: IAgentRuntime): boolean { - const githubService = runtime.getService<GitHubService>("github"); - return githubService?.isAuthenticated() ?? false; -}src/providers/workingCopies.ts (1)
92-111: Sequential async operations could be parallelized.The loop awaits
fs.statsequentially for each working copy. With many working copies, this could be slow. Consider usingPromise.allfor parallel execution.⚡ Proposed optimization
- for (const [key, data] of entries) { - let exists = false; - try { - const stat = await fs.stat(path.join(data.localPath, ".git")); - exists = stat.isDirectory(); - } catch { - exists = false; - } - - workingCopyList.push({ - key, - owner: data.owner, - repo: data.repo, - localPath: data.localPath, - branch: data.branch, - commit: data.commit, - exists, - clonedAt: data.clonedAt, - }); - } + const workingCopyList = await Promise.all( + entries.map(async ([key, data]) => { + let exists = false; + try { + const stat = await fs.stat(path.join(data.localPath, ".git")); + exists = stat.isDirectory(); + } catch { + exists = false; + } + return { + key, + owner: data.owner, + repo: data.repo, + localPath: data.localPath, + branch: data.branch, + commit: data.commit, + exists, + clonedAt: data.clonedAt, + }; + }) + );src/actions/pullRequests.ts (1)
1083-1122: Cap feedback payload before LLM summarization.
allFeedback.join()can get huge on busy PRs and exceed model context. Consider truncating using the new context utilities before building the prompt.🧩 Example using context-management utilities
+import { truncateToTokens, getTokenBudget } from "../utils/context-management"; ... - const summaryPrompt = `Analyze and summarize the feedback on this pull request: + const feedbackText = truncateToTokens( + allFeedback.join("\n\n"), + getTokenBudget({ mode: "detail", maxTokens: 8000 }) + ); + const summaryPrompt = `Analyze and summarize the feedback on this pull request: ... -**Feedback to analyze:** -${allFeedback.join("\n\n")} +**Feedback to analyze:** +${feedbackText}
**Breaking:** Ngrok is now an optional peer dependency instead of required **Performance Improvements:** - Remove 5+ redundant LLM calls per webhook command (95% reduction) - Replace LLM-based intent analysis with simple keyword matching - Validation now uses pattern matching instead of TEXT_LARGE model calls **Webhook URL Detection (Progressive Enhancement):** 1. GITHUB_WEBHOOK_URL - Explicit override (highest priority) 2. PUBLIC_URL - Auto-constructs webhook endpoint 3. SERVER_HOST + SERVER_PORT - Hostname-based URL construction 4. Ngrok tunnel - Fallback for local development (lowest priority) **Configuration:** - Added GITHUB_WEBHOOK_URL, PUBLIC_URL, SERVER_HOST, SERVER_PORT, SERVER_PROTOCOL settings - Ngrok now auto-detected when available, not required - Works on any public hostname/IP without tunnel services **Impact:** - Production-ready for cloud deployments (Railway, Render, Vercel, K8s, etc.) - Faster webhook operations (no LLM roundtrips) - Lower API costs (~95% reduction for webhook commands) - Better developer experience with clear configuration priority **Files Changed:** - src/actions/webhooks.ts: Refactored all 4 webhook actions - package.json: Moved ngrok to optional peerDependency - README.md: Added comprehensive webhook configuration docs Co-authored-by: Cursor <cursoragent@cursor.com>
**Documentation:** - Mark GITHUB_TOKEN as optional with unauthenticated mode note (60 req/hr) - Add language specifier to markdown code block (fixes MD040 lint) **Issue Action:** - Update validation regex to accept shorthand patterns (owner/repo#123, issue #123) - Supports full URLs, shorthand notation, and plain issue numbers with context **PR Split Actions:** - Fix mergeAndSaveGitHubState argument order (newState, roomId) across all calls - Remove || true from hasPlanContext validation to properly check planning keywords - Add normalizeSuggestions helper to sanitize LLM responses - Normalize parsed.suggestions and parsed.updatedSuggestions before use - Prevent undefined/non-array values in files, dependencies, priority fields - Set safe defaults: [] for arrays, incremental priority, empty strings for text **Impact:** - Prevents runtime errors from malformed LLM responses - Fixes state persistence bugs (workingCopies, plans now saved correctly) - Improves action validation accuracy - Better support for natural language issue references Co-authored-by: Cursor <cursoragent@cursor.com>
**New Utility: shell-exec.ts**
- Created centralized secure shell execution module
- Uses execFile (no shell interpolation) to prevent command injection
- Provides gitExec wrapper for common git operations
- Input sanitization functions:
- sanitizeRepoIdentifier: Validates owner/repo names
- sanitizeBranchName: Validates git branch names
- sanitizeFilePaths: Validates file path arrays
- validatePath: Prevents path traversal attacks
**Security Improvements in prSplit.ts:**
- Replaced all execAsync (shell interpolation) with gitExec
- Sanitize all user inputs (owner, repo, branch, files)
- Use git config http.extraheader for authentication (no tokens in URLs)
- Validate paths against base directory to prevent traversal
- Pass arguments as arrays instead of shell strings
**Examples:**
Before: execAsync(`git clone ${url} "${dir}"`) // Shell injection risk
After: gitExec(['clone', url, dir]) // Safe argument passing
**Impact:**
- Eliminates command injection vulnerabilities
- Prevents credential leakage in git URLs/history
- Blocks path traversal attacks
- Maintains functionality while improving security posture
Co-authored-by: Cursor <cursoragent@cursor.com>
**Bug Fixes:** - Fix TDZ error: Load existingState BEFORE using it in workingCopyData - Move loadGitHubState call before workingCopyData construction **Security Improvements:** - Replace all execAsync with gitExec (no shell interpolation) - Sanitize owner/repo inputs with sanitizeRepoIdentifier - Sanitize branch names with sanitizeBranchName - Validate paths with validatePath to prevent traversal - Use git config http.extraheader for auth (no tokens in URLs) **Changes Applied:** - cloneGitHubRepositoryAction: Secure clone/pull operations - listWorkingCopiesAction: Secure git status checks - deleteWorkingCopyAction: Secure safety checks **Impact:** - Fixes runtime error from using existingState before declaration - Eliminates command injection in all repository operations - Prevents path traversal attacks - Keeps tokens out of git history and shell history Co-authored-by: Cursor <cursoragent@cursor.com>
**Banner (banner.ts):** - Make GITHUB_TOKEN required flag dynamic based on auth status - Shows as required only when not authenticated **Rate Limit Handling (github.ts):** - Add rateLimitLimit property to track actual API limit - Initialize correctly for auth (5000) vs unauth (60) modes - Read x-ratelimit-limit header from API responses - Use dynamic threshold (2% of limit, min 10) instead of hardcoded 100 - Fix getRateLimitStatus to use actual limit instead of hardcoded 5000 **Division by Zero Guards:** - activity.ts: Guard usage calculation when limit is 0 - rateLimit.ts: Guard usedPercent calculation when limit is 0 - Prevents NaN/Infinity in rate limit display **Context Mode Detection (context-management.ts):** - Separate "full" pattern from "detail" pattern - Fix regex order so "full" maps to ContextMode.full correctly - Pattern priority: minimal > summary > full > detail **State Persistence (state-persistence.ts):** - Deep merge workingCopies to preserve existing clones - Deep merge prFeedbackCache to preserve cached PR data - Prevents data loss when updating state with new entries **Impact:** - Accurate rate limit tracking for both auth modes - No more NaN in rate limit displays - Working "full" context mode - State updates preserve existing data correctly - Dynamic banner based on actual auth status Co-authored-by: Cursor <cursoragent@cursor.com>
**package.json:**
- Mark GITHUB_TOKEN as optional (required: false)
- Update description to explain unauthenticated mode support
- Clarifies 60 req/hr public access vs 5000 req/hr with token
**autoCoder.ts:**
- Standardize parameter naming: max_tokens → maxTokens
- Consistent with useModel API across all calls (lines 222, 299)
**webhooks.ts:**
- Fix inconsistent state access pattern
- Use state?.data?.github consistently across all handlers
- Add proper null checking before accessing lastRepository
- Fixes createWebhook, listWebhooks, deleteWebhook, pingWebhook
**github.ts:**
- Fix validateAuthentication headers destructuring bug
- Headers are at response root, not in data object
- Change: const { data } → const { data, headers }
- Ensures rate limits update correctly during auth validation
**Impact:**
- Correct metadata for unauthenticated mode support
- Prevents runtime errors from malformed state access
- Fixes rate limit tracking during authentication
- Consistent API parameter naming
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In `@src/actions/issues.ts`:
- Around line 415-422: The validate function currently returns
isGitHubAuthenticated(runtime) unconditionally; change it to require both
authentication and explicit user intent to create an issue by adding an intent
check (e.g., call an existing intent helper like isCreateIssueIntent(message) or
detect the intent from the Memory/message content) and return true only when
isGitHubAuthenticated(runtime) && isCreateIssueIntent(message). Update the
validate signature in src/actions/issues.ts (the validate method for this
action) to run both checks and ensure any helper used
(isCreateIssueIntent/detectIntent) is imported or implemented and returns a
boolean.
In `@src/actions/prSplit.ts`:
- Around line 830-831: The commit currently uses split.description directly in
the gitExec(['commit', '-m', split.description]) call which will fail when
split.description is an empty string; update the commit step in
src/actions/prSplit.ts to compute a safe message (e.g., const message =
split.description && split.description.trim().length ? split.description :
'chore: apply split changes') and pass that message to gitExec(['commit', '-m',
message], { cwd: repoDir }) so commits never receive an empty -m value; ensure
you reference the split variable and the gitExec(['commit'...]) invocation when
making the change.
In `@src/actions/repository.ts`:
- Around line 361-368: The current validate function on the repository action
returns true for any authenticated user (validate in src/actions/repository.ts
uses isGitHubAuthenticated(runtime)), which is too permissive for a write
operation; update validate to require both authentication and explicit intent by
inspecting the incoming Memory (message) content/text for intent keywords (e.g.,
"create", "new", "init", "create repository", "create repo", "init repo", "make
repo", "repository") before returning true, so only when
isGitHubAuthenticated(runtime) is true AND the message contains one of those
intent tokens should validate return true; implement the keyword check safely
(case-insensitive, trim) and keep State and IAgentRuntime parameters unchanged.
- Around line 212-219: The validate function currently returns true for any
authenticated user; tighten it by adding a simple intent check on the incoming
Memory message before allowing the action. In the validate implementation in
repository.ts (the validate async function that calls
isGitHubAuthenticated(runtime)), add a small intent-matching step that inspects
the message text/content (e.g., message.text or message.content) with a
case-insensitive regex for keywords like "list", "show", "repos", or
"repositories" (for example
/(list|show).*(repo|repository|repos)|\b(repo|repository|repos)\b/i) and return
isGitHubAuthenticated(runtime) && intentMatches; keep the check lightweight and
inline (or add a small helper like matchesListReposIntent) so only authenticated
requests that match the list-repos intent pass validation.
- Around line 821-902: Summary: git config writes persist tokens to disk; use
per-command -c to avoid storing credentials. Replace all gitExec calls that run
'config' to set http.https://github.com/.extraheader (both local and global)
with adding a per-command git config flag: prepend '-c',
`http.https://github.com/.extraheader=Authorization: Basic
${Buffer.from(\`x-access-token:${token}\`).toString('base64')}` to the gitExec
argument arrays when hasToken is true (e.g., for fetch/checkout/pull/reset in
the pull branch path and for clone/fallbackArgs in the clone branch path),
remove the corresponding gitExec(['config', ...]) and any --unset calls, and
ensure cwd and other options are preserved when you call gitExec with the
modified argument arrays (refer to gitExec, hasToken, token, cloneArgs,
fallbackArgs, repoDir, branch).
In `@src/actions/webhooks.ts`:
- Around line 38-42: The current regex in src/actions/webhooks.ts (variable
webhookMatch) is too broad because it falls back to matching any "#123" (which
can be issues/PRs); change the pattern to only match webhooks by replacing
/webhook\s*#?(\d+)|#(\d+)/i with a single more specific pattern like
/webhook\s*#?(\d+)/i, then update the parsing to use the single capture group
(use parseInt(webhookMatch[1]) and assign to result.webhookId) so only explicit
"webhook" references produce a webhookId.
- Around line 32-36: The current repo extraction using
text.match(/\b([a-zA-Z0-9_-]+)\/([a-zA-Z0-9_-]+)\b/) (producing repoMatch) fails
for identifiers with dots and because \b breaks on dots; update the regex used
in the text.match call to allow dots in owner/repo (e.g. use
/([A-Za-z0-9._-]+)\/([A-Za-z0-9._-]+)/) or a non-word-boundary aware pattern (or
add appropriate lookarounds) so repoMatch[1]/repoMatch[2] correctly capture
names like user.name/repo.js.
In `@src/utils/shell-exec.ts`:
- Around line 198-207: The branch-name validation currently uses the
character-class invalidChars which erroneously treats '.' as forbidden
(rejecting single dots) because '..' cannot be expressed inside []; update the
validation in the branch name check (the invalidChars / branchName test in src
utils shell-exec.ts) to allow single dots but still disallow the two-character
sequence '..' and the other forbidden tokens; implement this by making
invalidChars only forbid whitespace and the single-character tokens (e.g., use a
character-class that includes @ { \\ ^ ~ : ? * [ but not .) and add a separate
test to explicitly reject the literal sequence '..' (or use a combined regex
with an alternation like \.\. to detect two dots); keep the thrown Error message
the same but ensure '.' is permitted in branch names.
- Around line 86-105: The catch block may set exitCode to a string (Node system
error codes like 'ENOENT'), breaking the expected number | null type; change the
exitCode computation to coerce/normalize to a numeric or null value by using a
numeric-first check: if error.code is a number use it, else if error.exitCode is
a number use that, otherwise set exitCode to null (or a numeric fallback if
desired). Update the local variable (exitCode) used in logger.warn and the
returned object so it always matches number | null; adjust references to
exitCode, stdout, stderr, logger.warn, and the returned object in the catch
block accordingly.
🧹 Nitpick comments (8)
src/actions/webhooks.ts (4)
8-11: Unused imports detected.
ModelTypeandz(zod) are imported but never used in this file. These appear to be leftovers from the previous LLM-based validation flow.🧹 Proposed cleanup
import { type Action, type IAgentRuntime, type Memory, type State, type HandlerCallback, logger, - ModelType, } from "@elizaos/core"; import { GitHubService } from "../services/github"; -import { z } from "zod";
13-19: Duplicate helper function.This
isGitHubAuthenticatedfunction duplicates the exported version insrc/index.ts. Consider importing it from there to maintain a single source of truth.♻️ Proposed refactor
-import { GitHubService } from "../services/github"; +import { GitHubService } from "../services/github"; +import { isGitHubAuthenticated } from "../index";Then remove lines 13-19 (the local function definition).
80-86: Type safety lost withas anycast.Casting
tunnelServicetoanyloses type safety. Consider defining and using aTunnelServiceinterface for better type checking.💡 Suggestion
interface TunnelService { isActive(): boolean; getUrl(): Promise<string>; } const tunnelService = runtime.getService("tunnel") as TunnelService | undefined;
589-591: Consider verifying webhook exists before pinging.Unlike
deleteWebhookAction(lines 467-479), this action doesn't verify the webhook exists before attempting to ping it. This could result in cryptic API errors. Consider adding a verification step for consistency and better error messages.💡 Suggestion
+ // Verify webhook exists first + const webhooks = await githubService.listWebhooks(owner, repo); + const webhook = webhooks.find((w) => w.id === webhookId); + + if (!webhook) { + await callback({ + text: `❌ Webhook #${webhookId} not found in ${owner}/${repo}.\n\nAvailable webhooks: ${webhooks.map((w) => `#${w.id}`).join(", ") || "None"}`, + thought: `Webhook ${webhookId} not found`, + }); + return; + } + // Ping the webhook await githubService.pingWebhook(owner, repo, webhookId);src/utils/shell-exec.ts (2)
138-140: Movepathimport to module top level.Inline
requireinside a function is unconventional and incurs repeated module resolution overhead. Import at the top of the file with other imports.♻️ Proposed refactor
Add to imports at top of file:
import path from "path";Then update the function:
export function validatePath(basePath: string, targetPath: string): string { - const path = require('path'); - // Resolve to absolute paths
236-239: Consider cross-platform absolute path detection.The check
file.startsWith('/')only catches Unix-style absolute paths. Windows absolute paths (e.g.,C:\) would pass through. If cross-platform support is needed, consider usingpath.isAbsolute().♻️ Proposed refactor for cross-platform support
+import path from "path"; + // Check for path traversal attempts -if (file.includes('..') || file.startsWith('/')) { +if (file.includes('..') || path.isAbsolute(file)) { throw new Error(`Path traversal detected in file: ${file}`); }src/actions/autoCoder.ts (1)
13-19: Consider reusing the sharedisGitHubAuthenticatedhelper.There’s already an exported helper in src/index.ts; centralizing avoids drift. If import cycles are a concern, move it to a small shared util module.
src/services/github.ts (1)
195-214: Track rate limits separately per resource (search, core) usingx-ratelimit-resourceheader.GitHub's rate limits are resource-scoped:
search(30/hour),core(5000/hour for authenticated),code_search(10/minute), andgraphqlare separate buckets with independent limits and reset times. The current implementation blindly updates globalrateLimitRemaining/Limit/Resetfrom any response, mixing these buckets.Your service calls both
search.issuesAndPullRequests()andsearch.repos()along with core endpoints. When a search request leaves only 29 requests remaining (out of 30), the global counter becomes 29, then unnecessarily throttles subsequent core API calls despite having the full 5000/hour core quota. Each bucket also has its own reset time, making the singlerateLimitResetincorrect.Update
updateRateLimit()to read thex-ratelimit-resourceheader and maintain separate counters per resource. RoutecheckRateLimit()checks by resource type (or call it with the appropriate resource before making search vs. core calls).
| validate: async ( | ||
| runtime: IAgentRuntime, | ||
| message: Memory, | ||
| state: State | undefined, | ||
| ): Promise<boolean> => { | ||
| const githubService = runtime.getService<GitHubService>("github"); | ||
| return !!githubService; | ||
| // This action requires authentication since it creates an issue | ||
| return isGitHubAuthenticated(runtime); | ||
| }, |
There was a problem hiding this comment.
Validate should still check user intent.
Returning true for any message when authenticated can trigger unintended issue creation. Add an intent check alongside auth.
✅ Add intent gating
- // This action requires authentication since it creates an issue
- return isGitHubAuthenticated(runtime);
+ if (!isGitHubAuthenticated(runtime)) return false;
+ const text = message.content?.text?.toLowerCase() || "";
+ return /create|submit|file|report|open/.test(text) && /issue|bug/.test(text);🤖 Prompt for AI Agents
In `@src/actions/issues.ts` around lines 415 - 422, The validate function
currently returns isGitHubAuthenticated(runtime) unconditionally; change it to
require both authentication and explicit user intent to create an issue by
adding an intent check (e.g., call an existing intent helper like
isCreateIssueIntent(message) or detect the intent from the Memory/message
content) and return true only when isGitHubAuthenticated(runtime) &&
isCreateIssueIntent(message). Update the validate signature in
src/actions/issues.ts (the validate method for this action) to run both checks
and ensure any helper used (isCreateIssueIntent/detectIntent) is imported or
implemented and returns a boolean.
| await gitExec(['add', '.'], { cwd: repoDir }); | ||
| await gitExec(['commit', '-m', split.description], { cwd: repoDir }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and read the context around lines 830-831
wc -l src/actions/prSplit.tsRepository: elizaos-plugins/plugin-github
Length of output: 100
🏁 Script executed:
# Read lines around 830-831 to verify the code
sed -n '820,840p' src/actions/prSplit.tsRepository: elizaos-plugins/plugin-github
Length of output: 1238
🏁 Script executed:
# Search for the Split type definition to understand the description field
rg -n "interface Split|type Split" src/Repository: elizaos-plugins/plugin-github
Length of output: 126
🏁 Script executed:
# Read the SplitSuggestion interface definition
sed -n '88,120p' src/actions/prSplit.tsRepository: elizaos-plugins/plugin-github
Length of output: 1224
🏁 Script executed:
# Search for where split.description is assigned or validated
rg -n "description\s*[=:]|split\.description" src/actions/prSplit.ts | head -20Repository: elizaos-plugins/plugin-github
Length of output: 927
🏁 Script executed:
# Search for where split.description might be set or come from
rg -n "split\.description\s*=" src/actions/prSplit.tsRepository: elizaos-plugins/plugin-github
Length of output: 55
🏁 Script executed:
# Look at the context around line 46 to understand the description assignment
sed -n '40,55p' src/actions/prSplit.tsRepository: elizaos-plugins/plugin-github
Length of output: 605
🏁 Script executed:
# Search for where suggestions are created and populated
rg -n "suggestions\s*=" src/actions/prSplit.ts | head -10Repository: elizaos-plugins/plugin-github
Length of output: 55
🏁 Script executed:
# Check the broader context around the commit to understand error handling
sed -n '800,835p' src/actions/prSplit.tsRepository: elizaos-plugins/plugin-github
Length of output: 1947
🏁 Script executed:
# Search for how suggestions are created from AI output
rg -n "SplitSuggestion\|suggestions\s*:" src/actions/prSplit.ts | grep -A 5 "suggestions"Repository: elizaos-plugins/plugin-github
Length of output: 55
🏁 Script executed:
# Look for the actual AI parsing that creates suggestions
sed -n '500,600p' src/actions/prSplit.tsRepository: elizaos-plugins/plugin-github
Length of output: 3983
🏁 Script executed:
# Continue reading the AI response parsing to see how descriptions are handled
sed -n '580,630p' src/actions/prSplit.tsRepository: elizaos-plugins/plugin-github
Length of output: 2186
🏁 Script executed:
# Check if there's any validation of split.description before the commit
sed -n '750,835p' src/actions/prSplit.tsRepository: elizaos-plugins/plugin-github
Length of output: 4149
🏁 Script executed:
# Check if there's any validation of split.description between normalization and commit
sed -n '40,100p' src/actions/prSplit.tsRepository: elizaos-plugins/plugin-github
Length of output: 1651
🏁 Script executed:
# Verify there's no validation between normalizeSuggestions and the commit line
rg -n "split\.description" src/actions/prSplit.tsRepository: elizaos-plugins/plugin-github
Length of output: 166
Add a safe commit-message fallback for empty descriptions.
The commit at line 831 uses split.description directly, which can be an empty string if the LLM-generated suggestion lacks a description. Git commit with an empty -m flag will fail. Provide a fallback message:
Suggested fix
- await gitExec(['commit', '-m', split.description], { cwd: repoDir });
+ const commitMsg = split.description?.trim() || `Split ${split.name}`;
+ await gitExec(['commit', '-m', commitMsg], { cwd: repoDir });🤖 Prompt for AI Agents
In `@src/actions/prSplit.ts` around lines 830 - 831, The commit currently uses
split.description directly in the gitExec(['commit', '-m', split.description])
call which will fail when split.description is an empty string; update the
commit step in src/actions/prSplit.ts to compute a safe message (e.g., const
message = split.description && split.description.trim().length ?
split.description : 'chore: apply split changes') and pass that message to
gitExec(['commit', '-m', message], { cwd: repoDir }) so commits never receive an
empty -m value; ensure you reference the split variable and the
gitExec(['commit'...]) invocation when making the change.
| validate: async ( | ||
| runtime: IAgentRuntime, | ||
| message: Memory, | ||
| state: State | undefined, | ||
| ): Promise<boolean> => { | ||
| const githubService = runtime.getService<GitHubService>("github"); | ||
| return !!githubService; | ||
| // This action requires authentication since it lists the authenticated user's repos | ||
| return isGitHubAuthenticated(runtime); | ||
| }, |
There was a problem hiding this comment.
List validation should still check intent.
Right now any message becomes eligible when authenticated. Add a simple intent check.
✅ Add intent gating
- // This action requires authentication since it lists the authenticated user's repos
- return isGitHubAuthenticated(runtime);
+ if (!isGitHubAuthenticated(runtime)) return false;
+ const text = message.content?.text?.toLowerCase() || "";
+ return /list|show|my|repositories|repos/.test(text);🤖 Prompt for AI Agents
In `@src/actions/repository.ts` around lines 212 - 219, The validate function
currently returns true for any authenticated user; tighten it by adding a simple
intent check on the incoming Memory message before allowing the action. In the
validate implementation in repository.ts (the validate async function that calls
isGitHubAuthenticated(runtime)), add a small intent-matching step that inspects
the message text/content (e.g., message.text or message.content) with a
case-insensitive regex for keywords like "list", "show", "repos", or
"repositories" (for example
/(list|show).*(repo|repository|repos)|\b(repo|repository|repos)\b/i) and return
isGitHubAuthenticated(runtime) && intentMatches; keep the check lightweight and
inline (or add a small helper like matchesListReposIntent) so only authenticated
requests that match the list-repos intent pass validation.
| validate: async ( | ||
| runtime: IAgentRuntime, | ||
| message: Memory, | ||
| state: State | undefined, | ||
| ): Promise<boolean> => { | ||
| const githubService = runtime.getService<GitHubService>("github"); | ||
| return !!githubService; | ||
| // This action requires authentication since it creates a repository | ||
| return isGitHubAuthenticated(runtime); | ||
| }, |
There was a problem hiding this comment.
Create validation is too permissive for a write action.
When authenticated, this action is always eligible. Require intent keywords.
✅ Add intent gating
- // This action requires authentication since it creates a repository
- return isGitHubAuthenticated(runtime);
+ if (!isGitHubAuthenticated(runtime)) return false;
+ const text = message.content?.text?.toLowerCase() || "";
+ return /create|make|new/.test(text) && /repo|repository/.test(text);🤖 Prompt for AI Agents
In `@src/actions/repository.ts` around lines 361 - 368, The current validate
function on the repository action returns true for any authenticated user
(validate in src/actions/repository.ts uses isGitHubAuthenticated(runtime)),
which is too permissive for a write operation; update validate to require both
authentication and explicit intent by inspecting the incoming Memory (message)
content/text for intent keywords (e.g., "create", "new", "init", "create
repository", "create repo", "init repo", "make repo", "repository") before
returning true, so only when isGitHubAuthenticated(runtime) is true AND the
message contains one of those intent tokens should validate return true;
implement the keyword check safely (case-insensitive, trim) and keep State and
IAgentRuntime parameters unchanged.
| try { | ||
| // Configure auth if token available | ||
| if (hasToken) { | ||
| await gitExec([ | ||
| 'config', | ||
| 'http.https://github.com/.extraheader', | ||
| `Authorization: Basic ${Buffer.from(`x-access-token:${token}`).toString('base64')}` | ||
| ], { cwd: repoDir }); | ||
| } | ||
|
|
||
| // Fetch and checkout the branch | ||
| await gitExec(['fetch', 'origin'], { cwd: repoDir }); | ||
| await gitExec(['checkout', branch], { cwd: repoDir }); | ||
| await gitExec(['pull', 'origin', branch], { cwd: repoDir }); | ||
| resultMessage = `Updated existing clone of ${owner}/${repo} (branch: ${branch})`; | ||
| } catch (pullError) { | ||
| logger.warn({ error: pullError }, "Pull failed, trying reset"); | ||
| await gitExec(['fetch', 'origin'], { cwd: repoDir }); | ||
| await gitExec(['reset', '--hard', `origin/${branch}`], { cwd: repoDir }); | ||
| resultMessage = `Reset ${owner}/${repo} to origin/${branch}`; | ||
| } | ||
| } else { | ||
| // Fresh clone | ||
| operation = "clone"; | ||
| const cloneArgs = ['clone']; | ||
| if (shallow) { | ||
| cloneArgs.push('--depth', '1'); | ||
| } | ||
| cloneArgs.push('--branch', branch, cloneUrl, repoDir); | ||
|
|
||
| logger.debug(`Running: git clone ${shallow ? '--depth 1 ' : ''}--branch ${branch} [url] "${repoDir}"`); | ||
|
|
||
| try { | ||
| // Configure auth globally before clone if token available | ||
| if (hasToken) { | ||
| await gitExec([ | ||
| 'config', | ||
| '--global', | ||
| 'http.https://github.com/.extraheader', | ||
| `Authorization: Basic ${Buffer.from(`x-access-token:${token}`).toString('base64')}` | ||
| ]); | ||
| } | ||
|
|
||
| await gitExec(cloneArgs); | ||
| resultMessage = `Cloned ${owner}/${repo} to ${repoDir} (branch: ${branch}${shallow ? ", shallow" : ""})`; | ||
|
|
||
| // Clear global auth config after clone | ||
| if (hasToken) { | ||
| await gitExec(['config', '--global', '--unset', 'http.https://github.com/.extraheader']); | ||
| } | ||
| } catch (cloneError: any) { | ||
| // Clear auth on error too | ||
| if (hasToken) { | ||
| await gitExec(['config', '--global', '--unset', 'http.https://github.com/.extraheader']); | ||
| } | ||
|
|
||
| // If branch doesn't exist, try without branch specification | ||
| if (cloneError.message?.includes("Remote branch") || cloneError.message?.includes("not found")) { | ||
| logger.warn(`Branch ${branch} not found, cloning default branch`); | ||
| const fallbackArgs = ['clone']; | ||
| if (shallow) { | ||
| fallbackArgs.push('--depth', '1'); | ||
| } | ||
| fallbackArgs.push(cloneUrl, repoDir); | ||
|
|
||
| // Configure auth again for fallback | ||
| if (hasToken) { | ||
| await gitExec([ | ||
| 'config', | ||
| '--global', | ||
| 'http.https://github.com/.extraheader', | ||
| `Authorization: Basic ${Buffer.from(`x-access-token:${token}`).toString('base64')}` | ||
| ]); | ||
| } | ||
|
|
||
| await gitExec(fallbackArgs); | ||
| resultMessage = `Cloned ${owner}/${repo} to ${repoDir} (default branch${shallow ? ", shallow" : ""})`; | ||
|
|
||
| // Clear auth after fallback | ||
| if (hasToken) { | ||
| await gitExec(['config', '--global', '--unset', 'http.https://github.com/.extraheader']); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/actions/repository.ts | sed -n '821,902p'Repository: elizaos-plugins/plugin-github
Length of output: 4071
🏁 Script executed:
# Let's check the gitExec function to understand how it works
rg -A 10 "const gitExec|function gitExec" src/actions/repository.tsRepository: elizaos-plugins/plugin-github
Length of output: 55
🏁 Script executed:
# Search for gitExec definition in the file
rg "gitExec" src/actions/repository.ts -B 5 -A 5 | head -50Repository: elizaos-plugins/plugin-github
Length of output: 2050
🏁 Script executed:
# Find and examine the gitExec function definition
cat -n src/utils/shell-exec.ts | head -100Repository: elizaos-plugins/plugin-github
Length of output: 3806
🏁 Script executed:
# Continue reading the gitExec function
cat -n src/utils/shell-exec.ts | sed -n '100,150p'Repository: elizaos-plugins/plugin-github
Length of output: 2020
🏁 Script executed:
# Let's verify the flow and understand the context better
# Check if there are any other places where gitExec or auth is handled
rg "config.*extraheader|gitExec" src/actions/repository.ts | wc -lRepository: elizaos-plugins/plugin-github
Length of output: 75
🏁 Script executed:
# Let me check git configuration best practices and verify the local config persistence issue
# First, let's see the full context of the pull operation
cat -n src/actions/repository.ts | sed -n '810,842p'Repository: elizaos-plugins/plugin-github
Length of output: 1679
🏁 Script executed:
# Let's verify the actual security implications by checking git documentation
# and understanding how local vs global config works
# Also check if there's any cleanup mechanism we're missing
cat -n src/actions/repository.ts | sed -n '842,910p'Repository: elizaos-plugins/plugin-github
Length of output: 3131
🏁 Script executed:
# Let's search for any cleanup of local config in the pull operation
rg "unset.*extraheader" src/actions/repository.tsRepository: elizaos-plugins/plugin-github
Length of output: 380
🌐 Web query:
git -c option authentication extraheader http config security best practices
💡 Result:
What git -c … http.extraHeader=… is doing (and why it’s risky)
http.extraHeadertells Git to add an additional HTTP header to requests; if you configure it multiple times, all of them are sent. Setting it to an empty value resets the list. [1]- Any
http.*option (includinghttp.extraHeader) can be scoped to a specific URL viahttp.<url>.*, with matching rules for scheme/host/port (and optional wildcards for subdomains). [2]
This makes http.extraHeader convenient for passing Authorization: …, but it also creates sharp edges: secrets can be persisted in config, hard to override cleanly, and accidentally applied more broadly than intended.
Best-practice recommendations
1) Prefer Git’s credential system over extraHeader for auth
Use credential helpers / askpass flows rather than injecting auth headers:
- Git supports credential helpers (OS keychain, cache, etc.) and other mechanisms to avoid repeatedly typing secrets. [3]
This usually avoids persisting an Authorization header in Git config at all.
2) If you must use extraHeader, scope it as narrowly as possible
- Prefer
http.https://example.com/.extraHeader=…(URL-scoped) instead of globalhttp.extraHeader, so the header only applies to that host. [2]
3) Don’t assume -c http.…extraHeader=… “overrides” an existing header
- Git will add multiple
extraHeaderentries, meaning you can end up sending two Authorization headers. This behavior has caused real-world breakage (notably with Git LFS) when attempting to “override” in CI. [4] - This is a known pain point with
actions/checkoutpersisting auth viahttp.https://github.com/.extraheader, making later overrides difficult. [5]
4) Avoid persisting long-lived tokens; rotate and minimize privileges
- If you’re in CI (e.g., GitHub Actions), ensure tokens are least-privilege (minimize
GITHUB_TOKENpermissions / PAT scopes). [6]
5) If you do persist headers, explicitly clear them after the operation
- Because an empty value resets extra headers, you can remove them deterministically when done (especially important in shared runners/workspaces). [1]
Sources
[1] Git config: http.extraHeader behavior (multiple headers; empty resets) (git-scm.com)
[2] Git config: URL-scoped http.<url>.* matching rules (git-scm.com)
[3] Git credentials & helpers (git-scm.com)
[4] Git LFS issue: duplicate Authorization headers when trying to override extraheader (github.com)
[5] actions/checkout issue: persisting auth in extraheader makes override difficult; credential-helper suggested (github.com)
[6] GitHub Docs: least-privilege guidance for GITHUB_TOKEN (archive.ph)
Citations:
- 1: https://git-scm.com/docs/git-config.html
- 2: https://git-scm.com/docs/git-config.html
- 3: https://git-scm.com/docs/gitcredentials.html?utm_source=openai
- 4: Authorization header added twice when git options override local config git-lfs/git-lfs#4031
- 5: Auth in extraheader makes it difficult to override actions/checkout#162
- 6: https://archive.ph/2025.12.24-113148/https%3A/docs.github.com/en/actions/tutorials/authenticate-with-github_token?utm_source=openai
Replace git config writes with per-command -c flag to prevent token persistence.
The code sets http.https://github.com/.extraheader in both local (pull, line 824) and global (clone, line 856) git config. This persists tokens in plaintext to .git/config or ~/.gitconfig, creating a security risk. Tokens can outlive their intended scope and be exposed if config files are logged or shared.
The pull scenario is particularly vulnerable: local config is written but never cleaned up, leaving credentials in the repository's .git/config indefinitely.
Use git's -c flag to pass auth per-command instead. This keeps secrets out of persistent config and is the recommended approach for CI/CD contexts.
🔒 Safer per-command auth (no persistent config)
- if (hasToken) {
- await gitExec([
- 'config',
- 'http.https://github.com/.extraheader',
- `Authorization: Basic ${Buffer.from(`x-access-token:${token}`).toString('base64')}`
- ], { cwd: repoDir });
- }
+ const authArgs = hasToken
+ ? ['-c', `http.https://github.com/.extraheader=Authorization: Basic ${Buffer.from(`x-access-token:${token}`).toString('base64')}`]
+ : [];
...
- await gitExec(['fetch', 'origin'], { cwd: repoDir });
+ await gitExec([...authArgs, 'fetch', 'origin'], { cwd: repoDir });
...
- await gitExec(['pull', 'origin', branch], { cwd: repoDir });
+ await gitExec([...authArgs, 'pull', 'origin', branch], { cwd: repoDir });
...
- if (hasToken) {
- await gitExec([
- 'config',
- '--global',
- 'http.https://github.com/.extraheader',
- `Authorization: Basic ${Buffer.from(`x-access-token:${token}`).toString('base64')}`
- ]);
- }
-
- await gitExec(cloneArgs);
-
- // Clear global auth config after clone
- if (hasToken) {
- await gitExec(['config', '--global', '--unset', 'http.https://github.com/.extraheader']);
- }
+ await gitExec([...authArgs, ...cloneArgs]);🤖 Prompt for AI Agents
In `@src/actions/repository.ts` around lines 821 - 902, Summary: git config writes
persist tokens to disk; use per-command -c to avoid storing credentials. Replace
all gitExec calls that run 'config' to set http.https://github.com/.extraheader
(both local and global) with adding a per-command git config flag: prepend '-c',
`http.https://github.com/.extraheader=Authorization: Basic
${Buffer.from(\`x-access-token:${token}\`).toString('base64')}` to the gitExec
argument arrays when hasToken is true (e.g., for fetch/checkout/pull/reset in
the pull branch path and for clone/fallbackArgs in the clone branch path),
remove the corresponding gitExec(['config', ...]) and any --unset calls, and
ensure cwd and other options are preserved when you call gitExec with the
modified argument arrays (refer to gitExec, hasToken, token, cloneArgs,
fallbackArgs, repoDir, branch).
| const repoMatch = text.match(/\b([a-zA-Z0-9_-]+)\/([a-zA-Z0-9_-]+)\b/); | ||
| if (repoMatch) { | ||
| result.owner = repoMatch[1]; | ||
| result.repo = repoMatch[2]; | ||
| } |
There was a problem hiding this comment.
Repository name regex doesn't match all valid GitHub identifiers.
GitHub usernames and repository names can contain dots (.), e.g., user.name/repo.js. The current pattern will fail to match these.
🔧 Proposed fix
- const repoMatch = text.match(/\b([a-zA-Z0-9_-]+)\/([a-zA-Z0-9_-]+)\b/);
+ const repoMatch = text.match(/\b([a-zA-Z0-9_.-]+)\/([a-zA-Z0-9_.-]+)\b/);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const repoMatch = text.match(/\b([a-zA-Z0-9_-]+)\/([a-zA-Z0-9_-]+)\b/); | |
| if (repoMatch) { | |
| result.owner = repoMatch[1]; | |
| result.repo = repoMatch[2]; | |
| } | |
| const repoMatch = text.match(/\b([a-zA-Z0-9_.-]+)\/([a-zA-Z0-9_.-]+)\b/); | |
| if (repoMatch) { | |
| result.owner = repoMatch[1]; | |
| result.repo = repoMatch[2]; | |
| } |
🤖 Prompt for AI Agents
In `@src/actions/webhooks.ts` around lines 32 - 36, The current repo extraction
using text.match(/\b([a-zA-Z0-9_-]+)\/([a-zA-Z0-9_-]+)\b/) (producing repoMatch)
fails for identifiers with dots and because \b breaks on dots; update the regex
used in the text.match call to allow dots in owner/repo (e.g. use
/([A-Za-z0-9._-]+)\/([A-Za-z0-9._-]+)/) or a non-word-boundary aware pattern (or
add appropriate lookarounds) so repoMatch[1]/repoMatch[2] correctly capture
names like user.name/repo.js.
| // Match webhook ID | ||
| const webhookMatch = text.match(/webhook\s*#?(\d+)|#(\d+)/i); | ||
| if (webhookMatch) { | ||
| result.webhookId = parseInt(webhookMatch[1] || webhookMatch[2]); | ||
| } |
There was a problem hiding this comment.
Webhook ID regex may produce false positives.
The pattern #(\d+) is overly broad and will match any #number reference (e.g., issue #123, PR #456). This could cause unintended behavior in messages that reference both webhooks and issues/PRs.
Consider making the pattern more specific or only matching #number when it appears in webhook-related context.
🔧 Proposed fix - more specific pattern
- const webhookMatch = text.match(/webhook\s*#?(\d+)|#(\d+)/i);
+ // Only match webhook ID when explicitly mentioned with "webhook"
+ const webhookMatch = text.match(/webhook\s*(?:id\s*)?#?(\d+)/i);
if (webhookMatch) {
- result.webhookId = parseInt(webhookMatch[1] || webhookMatch[2]);
+ result.webhookId = parseInt(webhookMatch[1]);
}🤖 Prompt for AI Agents
In `@src/actions/webhooks.ts` around lines 38 - 42, The current regex in
src/actions/webhooks.ts (variable webhookMatch) is too broad because it falls
back to matching any "#123" (which can be issues/PRs); change the pattern to
only match webhooks by replacing /webhook\s*#?(\d+)|#(\d+)/i with a single more
specific pattern like /webhook\s*#?(\d+)/i, then update the parsing to use the
single capture group (use parseInt(webhookMatch[1]) and assign to
result.webhookId) so only explicit "webhook" references produce a webhookId.
| } catch (error: any) { | ||
| // execFile throws on non-zero exit codes | ||
| const exitCode = error.code || error.exitCode || 1; | ||
| const stdout = error.stdout?.toString().trim() || ""; | ||
| const stderr = error.stderr?.toString().trim() || error.message || ""; | ||
|
|
||
| logger.warn({ | ||
| command, | ||
| args, | ||
| exitCode, | ||
| stderr: stderr.slice(0, 200), // Truncate for logging | ||
| }, "Shell command failed"); | ||
|
|
||
| // For non-zero exits, return the result instead of throwing | ||
| // This allows callers to handle expected failures (e.g., git branch -D on non-existent branch) | ||
| return { | ||
| stdout, | ||
| stderr, | ||
| exitCode, | ||
| }; |
There was a problem hiding this comment.
exitCode may be a string for system errors, violating the interface.
Node.js sets error.code to a string (e.g., 'ENOENT', 'ETIMEDOUT') for system errors, but to a number for process exit codes. The current logic could return a string exitCode, violating the number | null type contract.
🐛 Proposed fix to ensure numeric exitCode
} catch (error: any) {
// execFile throws on non-zero exit codes
- const exitCode = error.code || error.exitCode || 1;
+ // error.code can be a string for system errors (e.g., 'ENOENT') or number for exit codes
+ const exitCode = typeof error.code === 'number'
+ ? error.code
+ : (error.exitCode ?? 1);
const stdout = error.stdout?.toString().trim() || "";
const stderr = error.stderr?.toString().trim() || error.message || "";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (error: any) { | |
| // execFile throws on non-zero exit codes | |
| const exitCode = error.code || error.exitCode || 1; | |
| const stdout = error.stdout?.toString().trim() || ""; | |
| const stderr = error.stderr?.toString().trim() || error.message || ""; | |
| logger.warn({ | |
| command, | |
| args, | |
| exitCode, | |
| stderr: stderr.slice(0, 200), // Truncate for logging | |
| }, "Shell command failed"); | |
| // For non-zero exits, return the result instead of throwing | |
| // This allows callers to handle expected failures (e.g., git branch -D on non-existent branch) | |
| return { | |
| stdout, | |
| stderr, | |
| exitCode, | |
| }; | |
| } catch (error: any) { | |
| // execFile throws on non-zero exit codes | |
| // error.code can be a string for system errors (e.g., 'ENOENT') or number for exit codes | |
| const exitCode = typeof error.code === 'number' | |
| ? error.code | |
| : (error.exitCode ?? 1); | |
| const stdout = error.stdout?.toString().trim() || ""; | |
| const stderr = error.stderr?.toString().trim() || error.message || ""; | |
| logger.warn({ | |
| command, | |
| args, | |
| exitCode, | |
| stderr: stderr.slice(0, 200), // Truncate for logging | |
| }, "Shell command failed"); | |
| // For non-zero exits, return the result instead of throwing | |
| // This allows callers to handle expected failures (e.g., git branch -D on non-existent branch) | |
| return { | |
| stdout, | |
| stderr, | |
| exitCode, | |
| }; |
🤖 Prompt for AI Agents
In `@src/utils/shell-exec.ts` around lines 86 - 105, The catch block may set
exitCode to a string (Node system error codes like 'ENOENT'), breaking the
expected number | null type; change the exitCode computation to coerce/normalize
to a numeric or null value by using a numeric-first check: if error.code is a
number use it, else if error.exitCode is a number use that, otherwise set
exitCode to null (or a numeric fallback if desired). Update the local variable
(exitCode) used in logger.warn and the returned object so it always matches
number | null; adjust references to exitCode, stdout, stderr, logger.warn, and
the returned object in the catch block accordingly.
| // Git branch names: no spaces, .., @{, \, ^, ~, :, ?, *, [ | ||
| // Allow: letters, numbers, /, -, _, . | ||
| const invalidChars = /[\s@{\\^~:?*\[..]/; | ||
|
|
||
| if (invalidChars.test(branchName)) { | ||
| throw new Error( | ||
| `Invalid branch name: "${branchName}". ` + | ||
| `Branch names cannot contain spaces, .., @{, \\, ^, ~, :, ?, *, or [` | ||
| ); | ||
| } |
There was a problem hiding this comment.
Regex incorrectly rejects single dots; .. sequence detection is broken.
Inside a character class [], .. simply adds . to the set of matched characters—it doesn't match the literal two-character sequence ... This causes valid branch names containing single dots (e.g., release/1.0.0, feature.name) to be rejected.
Git allows single dots; only the .. sequence is prohibited.
🐛 Proposed fix to correctly detect invalid patterns
// Git branch names: no spaces, .., @{, \, ^, ~, :, ?, *, [
// Allow: letters, numbers, /, -, _, .
- const invalidChars = /[\s@{\\^~:?*\[..]/;
+ const invalidChars = /[\s\\^~:?*\[]/;
+ const invalidPatterns = /\.\.|@\{/;
- if (invalidChars.test(branchName)) {
+ if (invalidChars.test(branchName) || invalidPatterns.test(branchName)) {
throw new Error(
`Invalid branch name: "${branchName}". ` +
`Branch names cannot contain spaces, .., @{, \\, ^, ~, :, ?, *, or [`
);
}🤖 Prompt for AI Agents
In `@src/utils/shell-exec.ts` around lines 198 - 207, The branch-name validation
currently uses the character-class invalidChars which erroneously treats '.' as
forbidden (rejecting single dots) because '..' cannot be expressed inside [];
update the validation in the branch name check (the invalidChars / branchName
test in src utils shell-exec.ts) to allow single dots but still disallow the
two-character sequence '..' and the other forbidden tokens; implement this by
making invalidChars only forbid whitespace and the single-character tokens
(e.g., use a character-class that includes @ { \\ ^ ~ : ? * [ but not .) and add
a separate test to explicitly reject the literal sequence '..' (or use a
combined regex with an alternation like \.\. to detect two dots); keep the
thrown Error message the same but ensure '.' is permitted in branch names.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| // Clear global auth config after clone | ||
| if (hasToken) { | ||
| await gitExec(['config', '--global', '--unset', 'http.https://github.com/.extraheader']); | ||
| } |
There was a problem hiding this comment.
Global git config modification creates race condition and token leak
High Severity
The code sets authentication headers in --global git config before cloning. This affects ALL git operations system-wide and creates two problems: (1) concurrent git processes will see/use this token, and (2) if the process crashes between setting and unsetting the config (lines 222-229), the token persists in global config indefinitely. The repo-local config approach used when the repo already exists (lines 198-202) is safer, but unavailable before .git exists. Consider using GIT_ASKPASS or credential helpers instead.
|
|
||
| // Commit the changes | ||
| await gitExec(['add', '.'], { cwd: repoDir }); | ||
| await gitExec(['commit', '-m', split.description], { cwd: repoDir }); |
There was a problem hiding this comment.
Git command failures silently ignored due to unchecked exit codes
Medium Severity
The gitExec function returns results instead of throwing on failure (per shell-exec.ts lines 99-105), but this code never checks exitCode from the returned results. If checkout, pull, or file operations fail, execution continues silently on potentially wrong state. For example, a failed checkout safeBaseBranch means subsequent operations run on whatever branch was previously checked out, corrupting the split results.
| source: message.content.source, | ||
| }); | ||
|
|
||
| const branchName = sanitizeBranchName(`split/${analysis.prNumber}/${split.name}`); |
There was a problem hiding this comment.
Branch name validation outside try-catch breaks error isolation
Medium Severity
The sanitizeBranchName call at line 812 is outside the per-split try-catch block that starts at line 814. If split.name is empty (which normalizeSuggestions allows by converting missing names to ""), the branch name split/123/ ends with /, causing sanitizeBranchName to throw. This exception bypasses the per-split error handling and propagates to the outer handler, failing the entire operation even if other splits would have succeeded.
1 participant
Unauthenticated mode (60 req/hr): view public repos/issues/PRs, search
Authenticated mode (5000 req/hr): full access including private repos and writes
Note
Medium Risk
Adds substantial new GitHub automation (PR comment aggregation/summarization and local git-based PR splitting) plus new auth gating; mistakes could expose tokens via git config or cause unintended git/file operations despite added sanitization.
Overview
Adds token-optional operation:
GitHubServicenow supports unauthenticated public-only mode (60 req/hr) with clearer 403/404/rate-limit messaging, while write operations enforcerequireAuth()and actions that create/merge/comment are gated viaisAuthenticated().Expands PR tooling:
GET_GITHUB_PULL_REQUESTnow fetches and returns issue comments, review comments, and reviews (with bot-friendly formatting) and persists these artifacts; new actionsSUMMARIZE_PR_FEEDBACKandAGGREGATE_PR_CONTEXTgenerate LLM summaries or a copy/paste “mega prompt” of unaddressed review items with caching.Introduces PR split workflow: new
prSplitactions clone a PR branch to a local working copy, analyze commits/files, suggest/refine/confirm split plans, and execute splits by creating local branches with selected files, persisting plans/working-copy metadata for daemon restarts.Docs and packaging were updated to reflect new env vars/webhook URL strategies, unauthenticated mode behavior, and optional
@elizaos/plugin-ngrok/added@elizaos/plugin-gitdependency.Written by Cursor Bugbot for commit b6ad821. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.