Feature: Add a file editor#779
Closed
gsxdsm wants to merge 13 commits intoAutoMaker-Org:v0.15.0rcfrom
Closed
Conversation
- Updated CLAUDE_PROVIDER_TEMPLATES to include GLM-5 model - Mapped GLM-5 to opus tier for maximum capability - Updated deprecated template for backward compatibility - GLM-5 is the latest Zhipu model with improved performance
gsxdsm
added
Enhancement
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughIntroduces recurring feature scheduling via cron expressions, new z.ai and Gemini provider integrations with usage tracking, a comprehensive file manager with editor and Git support, multiple API routes for filesystem and scheduling operations, and code editor configuration UI. Expands service layer, type definitions, and app store with supporting infrastructure. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
✨ 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 |
Contributor
Summary of ChangesHello @gsxdsm, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a significant upgrade to the application's core functionality by introducing a full-featured file editor and browser, complete with integrated Git capabilities. It also enhances automation by enabling recurring feature execution through a new scheduler service and expands AI provider support to include z.ai and Google Gemini. Furthermore, the server's stability is improved with dynamic port handling and a process watchdog. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a significant new feature: a full-featured file browser and editor. It also adds a task scheduling system, integrates with z.ai and Gemini for usage tracking, and improves server startup with automatic port conflict resolution. My review focuses on ensuring the new backend endpoints are secure and that existing functionality wasn't unintentionally removed. I've identified a critical security vulnerability related to command injection in the new git-related endpoints and a high-severity regression where duplicate feature title checks were removed. There are also some suggestions for improving type safety.
Comment on lines
+30
to
+35
| const diffCmd = filePath ? `git diff HEAD -- "${filePath}"` : 'git diff HEAD'; | ||
|
|
||
| const { stdout: diff } = await execAsync(diffCmd, { | ||
| cwd: repoPath, | ||
| maxBuffer: 10 * 1024 * 1024, | ||
| }); |
Contributor
There was a problem hiding this comment.
Using exec with user-provided input (filePath) can open the door to command injection vulnerabilities. Even with quoting, it's safer to use execFile, which does not spawn a shell and treats arguments as separate tokens. Please update this to use execFile for improved security.
You will also need to update the import from child_process and the promisified function.
Suggested change
| const diffCmd = filePath ? `git diff HEAD -- "${filePath}"` : 'git diff HEAD'; | |
| const { stdout: diff } = await execAsync(diffCmd, { | |
| cwd: repoPath, | |
| maxBuffer: 10 * 1024 * 1024, | |
| }); | |
| const args = ['diff', 'HEAD']; | |
| if (filePath) { | |
| args.push('--', filePath); | |
| } | |
| const { stdout: diff } = await execFileAsync('git', args, { | |
| cwd: repoPath, | |
| maxBuffer: 10 * 1024 * 1024, | |
| }); |
Comment on lines
+43
to
+46
| const cmd = | ||
| action === 'stage' ? `git add -- "${filePath}"` : `git reset HEAD -- "${filePath}"`; | ||
|
|
||
| await execAsync(cmd, { cwd: repoPath }); |
Contributor
There was a problem hiding this comment.
Similar to the git-diff endpoint, using exec with user-provided filePath is a security risk. To prevent potential command injection, please switch to using execFile. This is a safer alternative as it does not invoke a shell.
Remember to update the child_process import and the promisified function accordingly.
Suggested change
| const cmd = | |
| action === 'stage' ? `git add -- "${filePath}"` : `git reset HEAD -- "${filePath}"`; | |
| await execAsync(cmd, { cwd: repoPath }); | |
| const args = | |
| action === 'stage' ? ['add', '--', filePath] : ['reset', 'HEAD', '--', filePath]; | |
| await execFileAsync('git', args, { cwd: repoPath }); |
| success: false, | ||
| error: `A feature with title "${feature.title}" already exists`, | ||
| duplicateFeatureId: duplicate.id, | ||
| // Calculate nextRun and set status to 'scheduled' if schedule is provided and enabled |
Contributor
There was a problem hiding this comment.
The logic to check for and prevent duplicate feature titles appears to have been removed in this change. This could lead to multiple features having the same name, causing confusion. This check should be restored.
// Check for duplicate title if title is provided
if (feature.title && feature.title.trim()) {
const duplicate = await featureLoader.findDuplicateTitle(projectPath, feature.title);
if (duplicate) {
res.status(409).json({
success: false,
error: `A feature with title "${feature.title}" already exists`,
duplicateFeatureId: duplicate.id,
});
return;
}
}
// Calculate nextRun and set status to 'scheduled' if schedule is provided and enabled| } | ||
| } | ||
|
|
||
| // Get the current feature to detect status changes |
Contributor
There was a problem hiding this comment.
The logic to check for duplicate feature titles during an update seems to have been removed. This is important to prevent renaming a feature to a title that is already in use by another feature. Please restore this validation.
// Check for duplicate title if title is being updated
if (updates.title && updates.title.trim()) {
const duplicate = await featureLoader.findDuplicateTitle(
projectPath,
updates.title,
featureId // Exclude the current feature from duplicate check
);
if (duplicate) {
res.status(409).json({
success: false,
error: `A feature with title "${updates.title}" already exists`,
duplicateFeatureId: duplicate.id,
});
return;
}
}
// Get the current feature to detect status changes
Comment on lines
+41
to
+51
| (status as any).authMethod || | ||
| (status.authenticated ? (status.hasApiKey ? 'api_key' : 'cli_login') : 'none'); | ||
|
|
||
| res.json({ | ||
| success: true, | ||
| installed: status.installed, | ||
| version: status.version || null, | ||
| path: status.path || null, | ||
| authenticated: status.authenticated || false, | ||
| authMethod, | ||
| hasCredentialsFile: (status as any).hasCredentialsFile || false, |
Contributor
There was a problem hiding this comment.
The use of (status as any) to access properties like authMethod and hasCredentialsFile indicates that the type of the status object is not fully defined. This reduces type safety and makes the code harder to maintain.
Please consider defining a more specific type for the object returned by provider.detectInstallation() that includes these optional properties. This will provide better autocompletion and prevent potential runtime errors if the object structure changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Full featured file browser and editor
Summary by CodeRabbit