-
Notifications
You must be signed in to change notification settings - Fork 2.8k
S2scopilot 1 #10946
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
S2scopilot 1 #10946
Conversation
Reviewing your PR now. I'll share feedback in a few minutes! |
|
Generated with ❤️ by ellipsis.dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Review: Request Changes
This PR cannot be merged in its current state due to several critical issues:
1. Inappropriate Rebranding (Blocker)
This PR includes extensive rebranding from "Roo Code" to "s2sCopilot" which is not appropriate for an upstream contribution. The following files contain rebranding changes that must be removed:
.roomodes- Replaces "Roo" with "s2sCopilot" in all custom mode role definitionsPRIVACY.md- Renames "Roo Code Privacy Policy" to "s2sCopilot Privacy Policy"README.md- Project documentation rebranding- All 18 localization files (
src/i18n/locales/*/common.json) - All 19 package NLS files (
src/package.nls.*.json)
2. Large Files Should Not Be Committed (Blocker)
The following files are inappropriate for the repository:
ca-bundle.crt(~6500+ lines) - A massive CA certificate bundle should NOT be committed to the repository. Enterprise CA certificates should be configured via system trust stores or environment variables, not bundled in source code.rename.png- Screenshot file that should not be in the repos2sCopilot.jpg- Branding image that should not be committed
3. Icon/Asset Replacements (Blocker)
The PR replaces Roo Code branding assets with s2sCopilot branding:
src/assets/icons/icon-nightly.pngsrc/assets/icons/icon.pngsrc/assets/icons/icon.svgsrc/assets/icons/panel_dark.pngsrc/assets/icons/panel_light.pngsrc/assets/images/roo-logo.svgsrc/assets/images/roo.png
4. Unrelated Changes
Many files in this PR appear unrelated to adding a new provider:
- Changes to terminal process files
- Changes to various test files
.gitignoremodifications
5. PR Template Not Completed
- No linked GitHub issue
- PR description is empty (template not filled out)
- Pre-submission checklist items are not checked
Required Actions
To make this PR acceptable:
- Remove ALL rebranding changes - Keep only the s2scopilot provider code
- Remove
ca-bundle.crt- Use environment variable or config option for custom CA certificates instead - Remove
rename.pngands2sCopilot.jpg- These do not belong in the repo - Keep ONLY the actual provider implementation files:
packages/types/src/providers/s2scopilot.tspackages/types/src/provider-settings.ts(only s2scopilot schema additions)src/api/providers/s2scopilot.tssrc/api/providers/fetchers/s2scopilot.tssrc/api/index.ts(only the s2scopilot case)src/api/providers/fetchers/modelCache.ts(only s2scopilot additions)webview-ui/src/components/settings/providers/S2sCopilot.tsx- Related webview component updates
- Fill out the PR template properly
- Create a linked issue if this is a feature request
It appears this branch is from a customized fork intended for internal use at an enterprise. The provider addition itself may be valuable, but it needs to be cleanly separated from the internal customizations.
Related GitHub Issue
Closes: #
Roo Code Task Context (Optional)
Description
Test Procedure
Pre-Submission Checklist
Screenshots / Videos
Documentation Updates
Additional Notes
Get in Touch