Conversation
WalkthroughThis update removes Bun-specific build scripts and configuration files, replacing them with a new Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant tsup
participant TypeScript
participant dist
Developer->>tsup: Run build (via npm script)
tsup->>TypeScript: Compile code (tsconfig.build.json)
TypeScript->>dist: Output JS and .d.ts files
tsup->>dist: Clean directory, bundle, externalize dependencies
tsup-->>Developer: Build complete or error
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Bug: Issue Creation Action Enabled by Default
The enabled: false property was removed from the createIssueAction, enabling it by default. This action was previously disabled to prevent unintended repository state modifications, and its default enablement could now lead to unwanted issue creation.
src/actions/issues.ts#L383-L386
plugin-github/src/actions/issues.ts
Lines 383 to 386 in 98fee14
Bug: Branch Creation Action Enabled By Default
The createBranchAction is now enabled by default after enabled: false was removed. This action was previously disabled to prevent unintended repository modifications, as branch creation changes repository state. Its default enablement could allow agents to create branches without explicit configuration.
src/actions/branches.ts#L240-L245
plugin-github/src/actions/branches.ts
Lines 240 to 245 in 98fee14
Bug: Repository Creation Action Enabled By Default
The enabled: false property was removed from createRepositoryAction. This action was previously disabled by default because "repository creation is an infrastructure change". Its removal enables the action by default, potentially allowing unintended repository creation.
src/actions/repository.ts#L324-L330
plugin-github/src/actions/repository.ts
Lines 324 to 330 in 98fee14
Bug: Pull Request Actions Enabled By Default
The enabled: false property was removed from createPullRequestAction and mergePullRequestAction. These actions, which modify repository state, were previously disabled by default for safety. Their default enablement could lead to unintended repository modifications.
src/actions/pullRequests.ts#L375-L380
plugin-github/src/actions/pullRequests.ts
Lines 375 to 380 in 98fee14
Was this report helpful? Give feedback by reacting with 👍 or 👎
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (5)
src/index.ts (1)
248-256: Webhook signature verification usesJSON.stringify(payload)– this will never match GitHub’s signatureGitHub computes the HMAC over the raw request body (bytes) before JSON parsing. Re-serialising with
JSON.stringifyalters whitespace and key order → every legitimate webhook will be rejected.-const digest = `sha256=${hmac.update(JSON.stringify(payload)).digest("hex")}`; +// `rawBody` must be provided by the HTTP framework, e.g. Express `req.rawBody` +const digest = `sha256=${hmac.update(rawBody).digest("hex")}`;If the framework does not expose
rawBody, add a raw-body middleware.src/services/github.ts (1)
167-183: Replace delete operators with property nullification for better performance.The static analysis correctly identifies that the delete operator can impact performance by changing object shapes and preventing JavaScript engine optimizations.
Apply this diff to improve performance:
private sanitizeError(error: any): any { const sanitized = { ...error }; // Remove sensitive headers and response data if (sanitized.response) { - delete sanitized.response.headers?.authorization; - delete sanitized.response.headers?.["x-github-token"]; - delete sanitized.response.request?.headers?.authorization; + if (sanitized.response.headers) { + sanitized.response.headers.authorization = undefined; + sanitized.response.headers["x-github-token"] = undefined; + } + if (sanitized.response.request?.headers) { + sanitized.response.request.headers.authorization = undefined; + } } if (sanitized.request) { - delete sanitized.request.headers?.authorization; - delete sanitized.request.headers?.["x-github-token"]; + if (sanitized.request.headers) { + sanitized.request.headers.authorization = undefined; + sanitized.request.headers["x-github-token"] = undefined; + } } return sanitized; }src/types.ts (1)
258-284: Consolidate duplicated GitHub error classesThe following error classes are defined twice—once in
src/types.tsand again insrc/services/github.ts—with slightly different constructor signatures:
- GitHubAPIError
- GitHubAuthenticationError
- GitHubRateLimitError
To avoid drift and simplify maintenance:
• Choose a single source of truth (e.g. keep them in
src/types.ts).
• Insrc/services/github.ts, import the classes fromsrc/types.tsinstead of redefining them.
• Remove the duplicate definitions insrc/services/github.ts.src/actions/branches.ts (1)
73-85: Fix pagination logic -pageandperPagevariables are not being usedThe pagination variables are defined but never passed to the
listBranchesmethod. This will cause an infinite loop if a repository has more than 100 branches.- while (true) { - const branches = await githubService.listBranches(owner, repo); - - allBranches.push(...branches); - - if (branches.length < perPage) { - break; - } - page++; - } + while (true) { + const branches = await githubService.listBranches(owner, repo, { + page, + per_page: perPage + }); + + allBranches.push(...branches); + + if (branches.length < perPage) { + break; + } + page++; + }src/__tests__/test-helpers.ts (1)
196-292: Consolidate duplicate mock factory functionsThere are three different functions creating mock repositories:
createTestRepository,createMockRepository, andcreateMockRepo. Similarly, there are duplicates for issues (createTestIssuevscreateMockIssue) and pull requests. This creates confusion and maintenance overhead.Consider consolidating these into a single set of mock factories. For example:
- Keep only
createMockRepositoryand removecreateTestRepositoryandcreateMockRepo- Keep only
createMockIssueand removecreateTestIssue- Keep only
createMockPullRequestand removecreateTestPREach should have comprehensive default values and accept overrides.
Also applies to: 415-452, 749-867
🧹 Nitpick comments (17)
src/frontend/index.tsx (1)
30-42:queryKeyshould include dynamic parts to prevent cache collision
useQueryuses the static key["currentTime"]. If the component is ever mounted with differentapiBasevalues (e.g. multiple iframes or tests), React-Query’s cache will return stale data across back-ends.-const { data, isLoading, error, refetch } = useQuery<TimeResponse>({ - queryKey: ["currentTime"], +const { data, isLoading, error, refetch } = useQuery<TimeResponse>({ + queryKey: ["currentTime", apiBase],src/index.ts (1)
460-477:isTestEnvcomputation repeatedly scansprocess.argv– cache or simplifyEvery plugin init recomputes
isTestEnvwith multiplesome()passes overprocess.argv, which can be called hundreds of times in large test suites. Cache the result in a module-level constant or at least pre-compute theargvcheck once.src/__tests__/plugin.test.ts (2)
9-11: Prefer nulling overdeletein hot code pathsBiome flagged the
deleteoperator here. Re-assigning toundefinedis both faster and keeps the key enumerable state intact.- delete (global as any).GITHUB_CONFIG; + (global as any).GITHUB_CONFIG = undefined;
36-45: Use native assertion helpers instead ofexpect(true).toBe(true)Wrapping the call in a
try/catchand then assertingtrueisn’t necessary.
Bun/Jest/Vitest all support:await expect(githubPlugin.init!(config, mockRuntime)).resolves.not.toThrow();This both shortens the test and produces clearer failure output.
src/__tests__/e2e/intelligent-analysis.test.ts (1)
147-160: Same issue: auto-coding analysis test lacks assertionsThe block validates nothing except that no exception is thrown.
Add assertions on expected automation flags or complexity ratings to turn this into a meaningful E2E check.src/providers/github.ts (2)
62-73: Guard against undefinedupdated_atbefore date math
new Date(repo.updated_at).getTime()will yieldNaNifupdated_atis absent.
Add a fallback or filter falsy values prior to sorting to avoid ‘Invalid time value’ crashes on edge objects.
298-309: Same missing null-check for pull-requests sortingReplicate the defensive check when sorting
githubState.pullRequests.src/__tests__/autoCoder.test.ts (2)
101-106: Prefer optional chaining to silence null-checks & static-analysis warningsStatic analysis already flagged this block; a concise fix keeps intent while avoiding repetitive guards.
-if ( - params && - params.prompt && - params.prompt.includes("Analyze this GitHub mention") -) { +if (params?.prompt?.includes("Analyze this GitHub mention")) {
9-15: LeveragecreateTestRuntimeto reduce hand-rolled mocksOther new tests use the helper from
test-helpers.ts, but this file reconstructs a custom runtime/mock matrix manually.
Unifying on the shared helper:
- shortens the file by ±150 LOC,
- guarantees identical mock behaviour across suites,
- and prevents divergence if the helper’s API changes.
A light refactor now will save time later.
Also applies to: 45-63
src/__tests__/prAndBranch.test.ts (1)
5-37: Redundant double-mocking of@octokit/restadds complexity
mock.module("@octokit/rest" …)stubs the constructor, yet a second fully-featuredmockOctokitobject is later injected intogithubService(line 258).
Only one approach is required:
- Stub the module once and let
GitHubServiceuse it, or- Skip the module stub and overwrite
service.octokitmanually as you currently do.Keeping both paths increases maintenance cost and turns failures into detective work.
tsconfig.json (1)
10-18:rootDirexcludes top-level test helpers outsidesrcWith
"rootDir": "./src"the compiler refuses emitting declarations for files living in@typesor any future test utilities at project root.
Either:- "rootDir": "./src", + "rootDir": ".",or move every compile-time TypeScript file under
src/.
Failing to align paths surfaces as cryptic “file is outside of rootDir” errors.src/__tests__/runtime-scenarios.test.ts (1)
82-82: Fix performance issue: Replace delete operator with undefined assignment.The static analysis tool correctly identifies a performance concern with the delete operator.
- delete process.env.GITHUB_TOKEN; + process.env.GITHUB_TOKEN = undefined;src/actions/users.ts (1)
163-167: Consider standardizing error response structure.The error response structure here differs from other handlers in the same file (lines 387-390, 536-539) which use spread syntax. Consider using a consistent pattern across all error handlers.
- return { - success: false, - text: errorContent.text, - error: error instanceof Error ? error.message : String(error), - } as ActionResult; + return { + success: false, + ...errorContent, + } as ActionResult;src/actions/branches.ts (1)
315-321: Remove unused variable assignmentThe
_newBranchvariable is assigned but never used. If the return value is not needed, avoid the assignment.- const _newBranch = await githubService.createBranch( - owner, - repo, - branch, - refData.object.sha, - ); + await githubService.createBranch( + owner, + repo, + branch, + refData.object.sha, + );src/actions/issues.ts (2)
258-266:text.includes("all")can fire on words like “allocate”The state-detection heuristic checks substrings without word-boundaries, so a phrase like
“Show closed bugs allocated to me” flips the filter to"all".Consider a regex with word boundaries or a stricter tokenizer, e.g.:
- (text.includes("closed") - ? "closed" - : text.includes("all") ? "all" : "open") + (/(\ball\b)/i.test(text) + ? "all" + : /\bclosed\b/i.test(text) ? "closed" : "open")
69-71: Defaulting to string"0"hides parse failures
parseInt("0", 10)returns0, which is falsy and triggers the missing parameters branch – but it also masks the difference between “no match” and “the user literally typed#0”.Prefer
undefinedto signal not provided:-parseInt(issueMatch?.[3] || issueNumMatch?.[1] || "0", 10) +parseInt(issueMatch?.[3] ?? issueNumMatch?.[1] ?? "", 10)If neither regex matches,
parseInt("", 10)givesNaN, which is also falsy but distinguishes bad input from a genuine zero.src/__tests__/e2e/starter-plugin.ts (1)
120-131: Extract helper for locating the Starter serviceThe “try three names → fall back to
getAllServices” pattern is duplicated in two tests. A tiny helper keeps each test laser-focused on intent:function getStarterService(rt: any) { return ( rt.getService("starter") ?? rt.getService("_StarterService") ?? rt.getService("StarterService") ?? Object.entries(rt.getAllServices?.() ?? {}) .find(([k]) => k.toLowerCase().includes("starter"))?.[1] ); }Then in tests:
-let service = runtime.getService("starter") || … ; +const service = getStarterService(runtime);Less duplication, clearer failures.
Also applies to: 317-327
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (38)
.prettierrc.js(0 hunks)build.config.ts(0 hunks)build.ts(0 hunks)eslint.config.js(0 hunks)package.json(2 hunks)prettier.config.js(0 hunks)src/__tests__/action-chaining.test.ts(12 hunks)src/__tests__/autoCoder.test.ts(10 hunks)src/__tests__/e2e.test.ts(18 hunks)src/__tests__/e2e/intelligent-analysis.test.ts(8 hunks)src/__tests__/e2e/starter-plugin.ts(9 hunks)src/__tests__/integration.test.ts(5 hunks)src/__tests__/plugin.test.ts(6 hunks)src/__tests__/prAndBranch.test.ts(6 hunks)src/__tests__/runtime-integration.test.ts(6 hunks)src/__tests__/runtime-scenarios.test.ts(10 hunks)src/__tests__/test-helpers.ts(11 hunks)src/actions/activity.ts(24 hunks)src/actions/autoCoder.ts(27 hunks)src/actions/branches.ts(18 hunks)src/actions/issues.ts(22 hunks)src/actions/pullRequests.ts(20 hunks)src/actions/repository.ts(28 hunks)src/actions/search.ts(8 hunks)src/actions/stats.ts(15 hunks)src/actions/users.ts(20 hunks)src/actions/webhooks.ts(19 hunks)src/frontend/index.css(1 hunks)src/frontend/index.tsx(7 hunks)src/index.ts(21 hunks)src/providers/github.ts(20 hunks)src/services/github.ts(85 hunks)src/test-exports.ts(1 hunks)src/tests.ts(27 hunks)src/types.ts(8 hunks)test-results.json(0 hunks)tsconfig.json(1 hunks)tsup.config.ts(1 hunks)
💤 Files with no reviewable changes (6)
- test-results.json
- .prettierrc.js
- prettier.config.js
- build.config.ts
- build.ts
- eslint.config.js
🧰 Additional context used
🧬 Code Graph Analysis (9)
src/test-exports.ts (1)
src/tests.ts (3)
AllGitHubTestSuites(798-801)GitHubPluginTestSuite(794-794)GitHubIntelligentAnalysisTestSuite(795-795)
src/__tests__/plugin.test.ts (3)
src/__tests__/test-helpers.ts (1)
createTestRuntime(361-383)src/index.ts (1)
githubPlugin(428-887)src/types.ts (1)
githubConfigSchema(4-20)
src/services/github.ts (3)
src/types.ts (3)
GitHubAuthenticationError(269-274)GitHubAPIError(258-267)GitHubRateLimitError(276-284)src/index.ts (1)
GitHubService(890-890)test-server.js (3)
repo(56-56)repo(74-74)repo(92-92)
src/__tests__/autoCoder.test.ts (1)
src/actions/autoCoder.ts (2)
autoCodeIssueAction(94-525)respondToMentionAction(640-832)
src/actions/webhooks.ts (1)
src/services/github.ts (1)
GitHubService(52-2140)
src/__tests__/prAndBranch.test.ts (1)
src/__tests__/test-helpers.ts (1)
createMockPullRequest(524-549)
src/index.ts (2)
src/types.ts (3)
githubConfigSchemaFlexible(23-52)githubConfigSchema(4-20)GitHubConfig(54-54)src/services/github.ts (2)
GitHubConfig(4-8)GitHubService(52-2140)
src/types.ts (1)
src/services/github.ts (3)
GitHubAuthenticationError(33-38)GitHubAPIError(22-31)GitHubRateLimitError(40-48)
src/actions/users.ts (2)
src/index.ts (1)
GitHubService(890-890)src/services/github.ts (1)
GitHubService(52-2140)
🪛 Biome (1.9.4)
src/frontend/index.css
[error] 2-2: This @import is in the wrong position.
Any @import rules must precede all other valid at-rules and style rules in a stylesheet (ignoring @charset and @layer), or else the @import rule is invalid.
Consider moving import position.
(lint/correctness/noInvalidPositionAtImportRule)
src/__tests__/plugin.test.ts
[error] 10-10: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
src/services/github.ts
[error] 173-173: Avoid the delete operator which can impact performance.
(lint/performance/noDelete)
[error] 174-174: Avoid the delete operator which can impact performance.
(lint/performance/noDelete)
[error] 178-178: Avoid the delete operator which can impact performance.
(lint/performance/noDelete)
[error] 179-179: Avoid the delete operator which can impact performance.
(lint/performance/noDelete)
src/__tests__/autoCoder.test.ts
[error] 102-104: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/index.ts
[error] 479-479: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 554-554: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/__tests__/runtime-scenarios.test.ts
[error] 82-82: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
src/actions/activity.ts
[error] 240-240: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 366-366: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
src/tests.ts
[error] 277-278: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 Gitleaks (8.27.2)
src/__tests__/plugin.test.ts
15-15: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
15-15: Uncovered a GitHub Personal Access Token, potentially leading to unauthorized repository access and sensitive content exposure.
(github-pat)
130-130: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
163-163: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
130-130: Uncovered a GitHub Personal Access Token, potentially leading to unauthorized repository access and sensitive content exposure.
(github-pat)
163-163: Uncovered a GitHub Personal Access Token, potentially leading to unauthorized repository access and sensitive content exposure.
(github-pat)
src/__tests__/integration.test.ts
148-148: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor BugBot
🔇 Additional comments (43)
src/frontend/index.tsx (1)
90-93:document.documentElement.classList.add("dark")runs twice in React 18 Strict-ModeIn development builds React mounts/unmounts once to detect side-effects, so the
darkclass is added twice (no functional harm but unnecessary work). Guard with aneffectRanref or move the logic to CSS prefers-color-scheme if possible.src/test-exports.ts (1)
11-15: No issues – stylistic re-format is finesrc/__tests__/e2e/intelligent-analysis.test.ts (1)
53-82: Tests only log – no real assertionsInside the
forloop the test does nothing butconsole.log.
Without at least one assertion each iteration could silently pass even if the plugin misbehaves.
Consider asserting on a return value or mocked side-effect so the test suite adds real coverage.src/services/github.ts (2)
1-11: LGTM: Import statements and initial type definitions standardized.The conversion to double quotes improves consistency across the codebase.
2054-2089: LGTM: Excellent logging and error handling implementation.The logActivity method properly manages memory by limiting activity log size to 1000 entries and provides comprehensive logging for both success and failure cases.
src/actions/autoCoder.ts (3)
1-11: LGTM: Import statements standardized with double quotes.The quote style consistency improves readability and aligns with the codebase cleanup objectives.
94-525: LGTM: Comprehensive AI-powered auto-coding implementation.The intelligent issue analysis, structured LLM responses, and automated pull request creation logic is well-implemented. The quote style standardization enhances consistency without affecting the sophisticated functionality.
640-832: LGTM: Intelligent mention handling with proper fallback mechanisms.The mention analysis system with confidence scoring and intelligent response generation is well-designed. The fallback error handling ensures graceful degradation when AI analysis fails.
src/__tests__/action-chaining.test.ts (3)
1-5: LGTM: Test infrastructure modernized with createTestRuntime.The switch from
createMockRuntimetocreateTestRuntimealigns with the improved test helper utilities while maintaining the same test functionality.
14-161: LGTM: Comprehensive mock GitHub service with consistent quote style.The mock service provides thorough test data coverage for repositories, issues, pull requests, and API responses. The quote style standardization improves consistency.
163-472: LGTM: Well-structured action chaining test scenarios.The test suite effectively validates complex workflows including state propagation, conditional branching, and data accumulation across multiple GitHub actions. The quote style updates maintain test integrity.
src/types.ts (2)
1-52: LGTM: Configuration schemas with enhanced token validation.The flexible schema for testing environments with various token formats is well-designed. The quote style standardization improves consistency.
286-314: LGTM: Comprehensive validation schemas for GitHub operations.The Zod schemas provide robust input validation for repository, issue, and pull request creation with appropriate constraints and limits.
src/__tests__/autoCoder.test.ts (1)
1-1:bun:testrunner may no longer be available after the ts-up migrationAll tests are still tied to the Bun test runner (
import { … } from "bun:test").
The rest of the PR removes every Bun-specific build artifact and introducestsup, hinting at a Node-centric tool-chain. Unless the CI pipeline is still executed with Bun, these tests will not run.Consider moving the test suite to a runner that works in plain Node (Vitest / Jest / uvu, etc.) or keep Bun in the tool-chain explicitly.
Failing to align runner & build environment will break the pipeline silently.tsup.config.ts (1)
8-10: Bundle emits onlyesm; consumers on CommonJS may breakIf any downstream tooling or runtime still expects
require()-able output (e.g. older Node scripts or Jest without"type":"module"), limiting the build to ESM will be a breaking change.- format: ['esm'], + format: ['esm', 'cjs'],Adding CJS keeps compatibility at negligible cost.
Also applies to: 17-31
src/actions/webhooks.ts (2)
15-21: LGTM: Improved enum formatting for better readability.The multi-line array formatting for the webhook intent enum values enhances code readability and makes it easier to add/remove values in the future.
128-130: Good: Explicit default parameter handling.The explicit default parameter assignment with fallback objects improves code clarity and prevents potential undefined access issues.
src/tests.ts (2)
26-102: Excellent: Enhanced service registration with robust fallback mechanisms.The improved
ensureGitHubServicefunction now handles multiple service registration patterns and provides better error recovery. The fallback to overridinggetServicewhen no service map is found is a smart approach for testing scenarios.
127-129: Good: Improved logging with multi-line formatting.The multi-line logging format enhances readability for longer messages while maintaining the informational content.
src/actions/search.ts (3)
3-3: Good: Added ActionResult type for improved type safety.The explicit import and usage of ActionResult type enhances type safety and provides consistent return structures across action handlers.
151-168: Excellent: Structured success response with ActionResult.The explicit
success: trueflag and structured return object improve action chaining capabilities and provide clear success/failure indicators for downstream processing.
181-185: Good: Consistent error handling with ActionResult.The structured error response with
success: falseand error details maintains consistency with the success case and enables proper error propagation in action chains.src/__tests__/runtime-scenarios.test.ts (2)
1-11: Good: Improved import organization.Separating type-only imports from value imports enhances code clarity and aligns with TypeScript best practices.
139-153: Good: Enhanced mock service registration.The conditional mock setup and explicit service registration handling improve test reliability and better simulate real runtime scenarios.
src/actions/activity.ts (5)
3-3: Good: Added ActionResult type for improved type safety.The explicit import and usage of ActionResult type enhances type safety and provides consistent return structures across action handlers.
74-85: Excellent: Structured return with ActionResult.The explicit
success: trueflag and well-structured return object improve action chaining capabilities and provide clear success indicators.
131-152: Excellent: Comprehensive ActionResult implementation.The detailed return object with success flag, values, and data provides excellent support for action chaining and state management.
366-366: Fix type safety: Replace banned {} type with specific interface.Same issue as above - the
{}type should be more specific.- options: {} = {}, + options: Record<string, never> = {},Likely an incorrect or invalid review comment.
240-240: Fix type safety: Replace banned {} type with specific interface.The static analysis tool correctly identifies that
{}is too generic and should be replaced with a more specific type.- options: {} = {}, + options: Record<string, never> = {},Or better yet, define a specific options interface if options are expected in the future.
Likely an incorrect or invalid review comment.
src/actions/repository.ts (3)
1-11: Excellent type safety improvements.The addition of
ActionResulttype import and consistent double quote usage enhances code quality and type safety throughout the repository actions.
89-113: Good enhancement to return value structure.Adding explicit
success: trueflag and typedActionResultimproves consistency and makes the action results more predictable for consumers.
126-137: Improved error handling structure.The explicit
success: falseflag in error responses provides cleaner, more consistent error handling across all actions.src/actions/users.ts (2)
1-11: Consistent type safety improvements across user actions.The addition of
ActionResultimport and double quote usage maintains consistency with other action files.
285-323: Improved array operations formatting.The multiline formatting of filter and map operations enhances readability while maintaining the same functionality.
package.json (2)
75-78: Build tooling migration from Bun to tsup.The switch from
bun run build.tstotsupsimplifies the build process and aligns with modern TypeScript build practices. The removal of ESLint from the lint script suggests a consolidation toward Prettier-only formatting.
42-52: Dependencies Verified – No Issues DetectedAll of the bumped packages are published at the specified versions, no vulnerabilities were reported at the moderate audit level, and your caret ranges (
^) will allow the latest patch/minor releases (e.g. Tailwind 4.1.11, React Query 5.83.0) to be installed.• package.json (Lines 42–52):
– @elizaos/core@1.2.9
– @octokit/rest@22.0.0
– @tanstack/react-query@^5.80.7 → latest 5.83.0 (allowed by your range)
– react@19.1.0 / react-dom@19.1.0
– tailwindcss@^4.1.10 → latest 4.1.11 (allowed by your range)No compatibility conflicts were detected. You can proceed with your normal integration and testing workflow.
src/actions/stats.ts (3)
1-11: Consistent ActionResult type adoption.The addition of
ActionResulttype and consistent styling maintains the same high-quality improvements seen across other action files.
160-207: Well-structured return value with comprehensive data.The explicit
success: trueflag and structured data object provide excellent visibility into repository statistics while maintaining backward compatibility.
396-420: Good handling of permission-based API restrictions.The specific handling of 403 errors for traffic statistics with appropriate user feedback demonstrates thoughtful error handling for GitHub's permission model.
src/__tests__/e2e.test.ts (3)
1-12: Test infrastructure modernization.The switch to
createTestRuntimefrom the test helpers improves consistency and maintainability across the test suite.
142-150: Updated to use consistent GitHub API methods.The change from
getAuthenticatedUser()togetCurrentUser()andlistRepositories()with options object aligns with the updated service interface.
225-228: Improved async callback handling.The async callbacks now properly return empty arrays, ensuring consistent async behavior throughout the test chain.
src/__tests__/test-helpers.ts (1)
361-410: Well-structured test runtime creationThe new
createTestRuntime,createTestMessage, andcreateTestStatefunctions provide excellent abstractions for test setup. This standardization will make tests more maintainable and consistent.
| @config "../../tailwind.config.js"; | ||
| @import 'tailwindcss'; | ||
| @import "tailwindcss"; |
There was a problem hiding this comment.
Move @import above @config to satisfy CSS at-rule ordering
Biome correctly flags this: @import must precede all at-rules except @charset/@layer.
Today the file starts with
@config "../../tailwind.config.js";
@import "tailwindcss";Result: the @import rule is silently ignored by browsers, so Tailwind base layers are never loaded.
-@config "../../tailwind.config.js";
-@import "tailwindcss";
+@import "tailwindcss";
+@config "../../tailwind.config.js";🧰 Tools
🪛 Biome (1.9.4)
[error] 2-2: This @import is in the wrong position.
Any @import rules must precede all other valid at-rules and style rules in a stylesheet (ignoring @charset and @layer), or else the @import rule is invalid.
Consider moving import position.
(lint/correctness/noInvalidPositionAtImportRule)
🤖 Prompt for AI Agents
In src/frontend/index.css at lines 1 to 2, the @import rule is placed after the
@config rule, which violates CSS at-rule ordering and causes the @import to be
ignored by browsers. To fix this, move the @import "tailwindcss"; line above the
@config "../../tailwind.config.js"; line so that @import comes first, ensuring
Tailwind base layers load correctly.
| runtime?.getSetting("GITHUB_TOKEN") || | ||
| runtime?.getSetting("GITHUB_TOKEN") || | ||
| config.GITHUB_TOKEN || | ||
| config.GITHUB_TOKEN || | ||
| process.env.GITHUB_TOKEN || | ||
| process.env.GITHUB_TOKEN; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicate getSetting / env look-ups – dead code & maintenance risk
const token =
runtime?.getSetting("GITHUB_TOKEN") ||
runtime?.getSetting("GITHUB_TOKEN") || // duplicate
config.GITHUB_TOKEN ||
config.GITHUB_TOKEN || // duplicate
process.env.GITHUB_TOKEN ||
process.env.GITHUB_TOKEN; // duplicateRemove the redundant branches:
- runtime?.getSetting("GITHUB_TOKEN") ||
- runtime?.getSetting("GITHUB_TOKEN") ||
- config.GITHUB_TOKEN ||
- config.GITHUB_TOKEN ||
- process.env.GITHUB_TOKEN ||
- process.env.GITHUB_TOKEN;
+ runtime?.getSetting("GITHUB_TOKEN") ||
+ config.GITHUB_TOKEN ||
+ process.env.GITHUB_TOKEN;🤖 Prompt for AI Agents
In src/index.ts around lines 450 to 456, there are duplicate calls to
runtime.getSetting("GITHUB_TOKEN"), duplicate references to config.GITHUB_TOKEN,
and duplicate references to process.env.GITHUB_TOKEN. Remove the redundant
duplicate calls and references so that each source is checked only once in the
token assignment expression.
| import { describe, expect, it, mock, beforeEach } from "bun:test"; | ||
| import { githubPlugin, GitHubService } from "../index"; | ||
| import { IAgentRuntime, UUID } from "@elizaos/core"; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
This test still relies on Bun’s test runner while the repo migrated to Vite/Vitest
import { describe, expect, it, mock, beforeEach } from "bun:test";
The rest of the suite now uses Vitest (createTestRuntime, etc.). Keeping a Bun-specific test will:
- Fail CI on non-Bun environments.
- Prevent Jest/Vitest from collecting the file (unknown syntax).
Suggested quick fix:
-import { describe, expect, it, mock, beforeEach } from "bun:test";
+import { describe, expect, it, vi, beforeEach } from "vitest";Update mock() usages to vi.fn().
📝 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.
| import { describe, expect, it, mock, beforeEach } from "bun:test"; | |
| import { githubPlugin, GitHubService } from "../index"; | |
| import { IAgentRuntime, UUID } from "@elizaos/core"; | |
| -import { describe, expect, it, mock, beforeEach } from "bun:test"; | |
| +import { describe, expect, it, vi, beforeEach } from "vitest"; | |
| import { githubPlugin, GitHubService } from "../index"; | |
| import { IAgentRuntime, UUID } from "@elizaos/core"; |
🤖 Prompt for AI Agents
In src/__tests__/runtime-integration.test.ts at lines 1 to 4, the test imports
from Bun's test runner which is incompatible with the repo's migration to
Vite/Vitest. Replace the import statement to import testing utilities from
Vitest instead, and update all instances of Bun's mock() to Vitest's vi.fn() to
ensure compatibility and proper test collection in CI and other environments.
| import { createMockRuntime } from './test-utils'; | ||
|
|
||
| describe('GitHub Plugin Initialization', () => { | ||
| import { describe, it, expect, beforeEach, mock } from "bun:test"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid hard-coding the Bun test runner dependency
bun:test ties the tests to the Bun runtime even though the PR description says Bun-specific build tooling was removed.
If the project is no longer committed to Bun, switch to a runner-agnostic test API (e.g. vitest, jest, or Node’s built-in test module) to keep CI portable.
🤖 Prompt for AI Agents
In src/__tests__/plugin.test.ts at line 1, the test imports from the
Bun-specific test runner "bun:test", which conflicts with the PR's goal to
remove Bun-specific tooling. Replace the import statement to use a
runner-agnostic testing framework such as Vitest, Jest, or Node's built-in test
module. Update the import to match the chosen framework's API and adjust any
test syntax if necessary to ensure compatibility.
| GITHUB_TOKEN: "ghp_1234567890abcdef1234567890abcdef12345678", | ||
| GITHUB_OWNER: "test-owner", | ||
| GITHUB_WEBHOOK_SECRET: "test-webhook-secret", |
There was a problem hiding this comment.
Replace realistic-looking PATs with obvious placeholders
Static-analysis (Gitleaks) flags these strings as leaked credentials.
Shorten them so they fail the token regex but still satisfy test intent.
- GITHUB_TOKEN: "ghp_1234567890abcdef1234567890abcdef12345678",
+ GITHUB_TOKEN: "ghp_test_token",Do the same for the fine-grained token later in the file.
📝 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.
| GITHUB_TOKEN: "ghp_1234567890abcdef1234567890abcdef12345678", | |
| GITHUB_OWNER: "test-owner", | |
| GITHUB_WEBHOOK_SECRET: "test-webhook-secret", | |
| GITHUB_TOKEN: "ghp_test_token", | |
| GITHUB_OWNER: "test-owner", | |
| GITHUB_WEBHOOK_SECRET: "test-webhook-secret", |
🧰 Tools
🪛 Gitleaks (8.27.2)
15-15: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
15-15: Uncovered a GitHub Personal Access Token, potentially leading to unauthorized repository access and sensitive content exposure.
(github-pat)
🤖 Prompt for AI Agents
In src/__tests__/plugin.test.ts around lines 15 to 17, the GitHub token values
are realistic-looking personal access tokens which trigger static-analysis tools
like Gitleaks. Replace these tokens with obvious placeholder strings that do not
match token regex patterns but still fulfill the test requirements. Apply the
same replacement approach to any other similar tokens later in the file.
| { GITHUB_TOKEN: "ghp_test123456789012345678901234567890" }, | ||
| mockRuntime as unknown as IAgentRuntime, |
There was a problem hiding this comment.
Hard-coded string matches the GitHub PAT pattern – trips secret scanners
"ghp_test123456789012345678901234567890" looks like a Personal Access Token.
Even though obviously fake, most security scanners (Gitleaks already flagged it) will fail the pipeline.
Replace with a neutral placeholder:
- { GITHUB_TOKEN: "ghp_test123456789012345678901234567890" },
+ { GITHUB_TOKEN: "<TEST_GITHUB_TOKEN>" },📝 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.
| { GITHUB_TOKEN: "ghp_test123456789012345678901234567890" }, | |
| mockRuntime as unknown as IAgentRuntime, | |
| { GITHUB_TOKEN: "<TEST_GITHUB_TOKEN>" }, | |
| mockRuntime as unknown as IAgentRuntime, |
🧰 Tools
🪛 Gitleaks (8.27.2)
148-148: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
In src/__tests__/integration.test.ts around lines 148 to 149, the hard-coded
string "ghp_test123456789012345678901234567890" resembles a GitHub Personal
Access Token and triggers secret scanners. Replace this string with a neutral
placeholder value that does not match the PAT pattern to avoid false positives
in security scans.
| return { | ||
| success: false, | ||
| ...errorContent, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Maintain consistent error return structure
Same issue as above - the error return should match the pattern used in other actions.
- return {
- success: false,
- ...errorContent,
- };
+ return {
+ success: false,
+ text: errorContent.text,
+ error: error instanceof Error ? error.message : String(error),
+ } as ActionResult;📝 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.
| return { | |
| success: false, | |
| ...errorContent, | |
| }; | |
| } | |
| return { | |
| success: false, | |
| text: errorContent.text, | |
| error: error instanceof Error ? error.message : String(error), | |
| } as ActionResult; | |
| } |
🤖 Prompt for AI Agents
In src/actions/pullRequests.ts around lines 709 to 713, the error return object
structure is inconsistent with other actions. Modify the return statement to
follow the same error return pattern used elsewhere in the codebase, ensuring
the keys and structure match for consistency.
| return { | ||
| success: false, | ||
| ...errorContent, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Maintain consistent error return structure
The error return structure here differs from other actions in this file and branches.ts. Other actions explicitly set text and error fields.
- return {
- success: false,
- ...errorContent,
- };
+ return {
+ success: false,
+ text: errorContent.text,
+ error: error instanceof Error ? error.message : String(error),
+ } as ActionResult;📝 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.
| return { | |
| success: false, | |
| ...errorContent, | |
| }; | |
| } | |
| return { | |
| success: false, | |
| text: errorContent.text, | |
| error: error instanceof Error ? error.message : String(error), | |
| } as ActionResult; | |
| } |
🤖 Prompt for AI Agents
In src/actions/pullRequests.ts around lines 531 to 535, the error return object
spreads errorContent without explicitly setting the text and error fields,
causing inconsistency with other actions in this file and branches.ts. Modify
the return statement to explicitly include text and error properties with
appropriate values from errorContent or defaults, ensuring the error return
structure matches the consistent pattern used elsewhere.
| // Extract owner, repo, and issue number from message text or options | ||
| const text = message.content.text || ''; | ||
| const issueMatch = text.match(/(?:github\.com\/)?([^\/\s]+)\/([^\/\s]+)\/issues\/(\d+)/); | ||
| const ownerRepoMatch = text.match(/(?:github\.com\/)?([^\/\s]+)\/([^\/\s]+)/); | ||
| const text = message.content.text || ""; | ||
| const issueMatch = text.match( | ||
| /(?:github\.com\/)?([^\/\s]+)\/([^\/\s]+)\/issues\/(\d+)/, | ||
| ); | ||
| const ownerRepoMatch = text.match( | ||
| /(?:github\.com\/)?([^\/\s]+)\/([^\/\s]+)/, | ||
| ); | ||
| const issueNumMatch = text.match(/(?:issue\s*#?|#)(\d+)/i); | ||
|
|
||
| const owner = | ||
| options.owner || | ||
| issueMatch?.[1] || | ||
| ownerRepoMatch?.[1] || | ||
| state?.github?.lastRepository?.owner?.login || | ||
| runtime.getSetting('GITHUB_OWNER'); | ||
| runtime.getSetting("GITHUB_OWNER"); | ||
| const repo = | ||
| options.repo || | ||
| issueMatch?.[2] || | ||
| ownerRepoMatch?.[2] || | ||
| state?.github?.lastRepository?.name; | ||
| const issue_number = | ||
| options.issue_number || parseInt(issueMatch?.[3] || issueNumMatch?.[1] || '0', 10); | ||
| options.issue_number || | ||
| parseInt(issueMatch?.[3] || issueNumMatch?.[1] || "0", 10); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Factor out owner / repo parsing & fallback logic
The exact same block that (1) extracts owner / repo, (2) falls back to previous state, and (3) finally consults runtime.getSetting("GITHUB_OWNER") is repeated three times in this file.
Keeping it in‐line inflates the handlers, encourages drift, and makes it easy to forget to add a new fallback (e.g. there is no GITHUB_REPO fallback).
+// utils/github.ts
+export function resolveRepoCtx(
+ runtime: IAgentRuntime,
+ state: State | undefined,
+ text: string,
+ opts: { owner?: string; repo?: string }
+): { owner: string; repo: string } {
+ const ownerRepoMatch = text.match(/(?:github\.com\/)?([^\/\s]+)\/([^\/\s]+)/);
+ const owner =
+ opts.owner ??
+ ownerRepoMatch?.[1] ??
+ state?.github?.lastRepository?.owner?.login ??
+ runtime.getSetting("GITHUB_OWNER");
+ const repo =
+ opts.repo ??
+ ownerRepoMatch?.[2] ??
+ state?.github?.lastRepository?.name ??
+ runtime.getSetting("GITHUB_REPO"); // <- extra safety
+
+ if (!owner || !repo) {
+ throw new Error(
+ 'Repository owner and name are required (format "owner/repo" or pass in options)',
+ );
+ }
+ return { owner, repo };
+}Each handler then becomes:
-const text = message.content.text || "";
-const ownerRepoMatch = text.match(/(?:github\.com\/)?([^\/\s]+)\/([^\/\s]+)/);
-const owner = … // 5-line block
-const repo = … // 4-line block
+const { owner, repo } = resolveRepoCtx(
+ runtime,
+ state,
+ message.content.text ?? "",
+ options,
+);Single-sourced logic = easier audits & fewer subtle mismatches.
Also applies to: 236-251, 410-424
🤖 Prompt for AI Agents
In src/actions/issues.ts around lines 47 to 70, the code for extracting owner,
repo, and issue number from the message text or options is duplicated multiple
times with slight variations and inconsistent fallbacks. To fix this, refactor
by creating a single helper function that encapsulates the parsing and fallback
logic for owner, repo, and issue number, including all relevant fallbacks such
as state and runtime settings. Replace all repeated blocks in this file
(including lines 236-251 and 410-424) with calls to this new helper function to
ensure consistent and maintainable code.
1 participant
Plugin-GitHub: Complete TypeScript Cleanup & Test Framework Migration
Overview
This PR comprehensively fixes all TypeScript issues in the plugin-github package, migrates to the official test utilities, and ensures proper build enforcement. The package now has 0 TypeScript errors (down from 55) and uses standardized ElizaOS test infrastructure.
Changes Made
Phase 1: TypeScript Cleanup & Build System
1. Build System Overhaul
tsupfor consistency with other pluginsprebuildscript - builds now fail on TS errors2. Fixed All Action Handlers (11 files)
Every action handler was missing the required
successproperty in ActionResult:Files Fixed:
activity.ts- 3 handlersbranches.ts- 3 handlerspullRequests.ts- 2 handlersrepository.ts- 3 handlerssearch.ts- 1 handlerstats.ts- 2 handlersusers.ts- 3 handlersissues.ts- 4 handlerswebhooks.ts- 2 handlersautoCoder.ts- 2 handlersPattern Applied:
3. Removed Invalid Properties
enabledproperty from 3 actionsimport * as crypto from 'crypto'describeimport in test-helpers.ts4. Frontend/JSX Configuration
Phase 2: Test Framework Migration
1. Migrated to Official Test Utils
@elizaos/test-utilsv1.2.0 as devDependency2. Test Infrastructure Updates
3. Fixed Memory Object Structure
entityIdinstead ofuserId)Phase 3: Service Registration & Final Fixes
1. Service Registration Pattern
Fixed incorrect service registration across test files:
2. HandlerCallback Type Fixes
Fixed 9 callbacks to return
Promise<Memory[]>:3. Additional Fixes
listRepositoriesmethod signatureContent,UUID,logger)createMockRepohelper functionResults
Before
After
Verification
Breaking Changes
None - All changes are internal improvements. The public API remains unchanged.
Dependencies Added
Production
@types/react: ^18.3.3@types/react-dom: ^18.3.0react: ^18.3.1react-dom: ^18.3.1@tanstack/react-query: ^5.80.7Development
@elizaos/test-utils: ^1.2.0tsup: ^8.5.5Files Modified
Documentation
Created comprehensive documentation:
TYPESCRIPT_AND_SERVICE_FIXES.md- Detailed fix summaryTEST_UTILS_MIGRATION.md- Migration guideThis PR brings plugin-github up to ElizaOS standards for type safety, testing, and build processes.
Summary by CodeRabbit
Chores
tsupwith a new configuration for improved TypeScript support.Style
Refactor
ActionResulttypes.Documentation
Tests
No user-facing functionality was changed; all updates focus on internal structure, style, and developer tooling.