-
Notifications
You must be signed in to change notification settings - Fork 46.3k
feat(platform): add AI copilot chat assistant for agent discovery and creation #11698
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
base: dev
Are you sure you want to change the base?
Conversation
…utoGPT into hackathon/copilot
…o be able to run library agents directly
…utoGPT into hackathon/copilot
…utoGPT into hackathon/copilot
|
Thanks for your PR submission! This appears to be adding a significant copilot/assistant feature to the platform, but there are several critical issues that need to be addressed before this can be considered for merging: Required Changes
Once you've updated the PR with this information, we'd be happy to review the technical implementation. The code itself looks interesting, but we need the proper context and documentation to evaluate it effectively. |
| """ | ||
| try: | ||
| stats = await store_embeddings.get_embedding_stats() | ||
| return stats |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Stack trace information
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
In general, to fix this kind of issue, you should avoid including raw exception messages or stack traces in HTTP responses. Instead, log the detailed error on the server and return a sanitized, generic error object to the client. This maintains debuggability for developers while preventing information disclosure.
For this specific code:
-
In
backend/api/features/store/embeddings.py:get_embedding_statscurrently returns a dict with an"error": str(e)field when an exception occurs. This dict is what propagates to the admin route and then to the client. We should remove the raw exception text from the returned data and replace it with a generic message (for example,"error": "Failed to get embedding stats"). The detailed error is already logged vialogger.error, which is safe to keep.- Similarly,
backfill_missing_embeddingsreturns"error": str(e). Even though the provided alert focuses on the stats endpoint, this function has the same pattern and should be sanitized for consistency and safety.
-
In
backend/api/features/admin/store_admin_routes.py, the route-level exception handling is already safe: it logs the exception and raises anHTTPExceptionwith a genericdetailmessage. No changes are needed there.
Concretely:
- Edit
get_embedding_stats’sexceptblock (lines 311–319) to return a dict withoutstr(e)and with a fixed, generic error string. - Edit
backfill_missing_embeddings’sexceptblock (lines 385–392) to do the same. - No new imports or methods are required; we reuse existing logging and types.
-
Copy modified line R318 -
Copy modified line R391
| @@ -315,7 +315,7 @@ | ||
| "with_embeddings": 0, | ||
| "without_embeddings": 0, | ||
| "coverage_percent": 0, | ||
| "error": str(e), | ||
| "error": "Failed to get embedding stats", | ||
| } | ||
|
|
||
|
|
||
| @@ -388,7 +388,7 @@ | ||
| "processed": 0, | ||
| "success": 0, | ||
| "failed": 0, | ||
| "error": str(e), | ||
| "error": "Failed to backfill embeddings", | ||
| } | ||
|
|
||
|
|
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.
@Swiftyos does this api needs to be publicly available? is this even used at all?
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.
Ask❓
@Swiftyos does this api needs to be publicly available? is this even used at all?
Answer:
This endpoint is in admin/store_admin_routes.py under the admin router, so it should not be publicly available. It’s a maintenance/observability endpoint (embedding coverage/health) and doesn’t look like something a normal client needs. If the admin router is already protected (admin auth / internal-only), then it’s fine; if not, it should be gated the same way as other admin endpoints.
On “is this even used?”: nothing in the diff you shared shows a frontend caller, and the endpoint name (/embeddings/stats) suggests it’s intended for operational dashboards or manual checks during the embeddings backfill work. If there’s no consumer, we should either:
- remove it until we actually need it, or
- keep it but document/confirm the intended consumer (admin UI, scripts, monitoring), and add a small test or a note in the PR description.
Also, the code-scanning comment is relevant here: the handler currently has a try: and then return stats, but no visible except block in the hunk. If an exception is raised, FastAPI will return a 500 and (depending on environment/config) may expose stack trace details. To address that:
- add an
except Exception as e:that logs internally and returns a genericHTTPException(status_code=500, detail="Failed to fetch embedding stats")(no raw exception text), and - ensure the route is protected (admin auth) so even metadata like “coverage %” isn’t exposed to the public.
| result = await store_embeddings.backfill_missing_embeddings( | ||
| batch_size=batch_size | ||
| ) | ||
| return result |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Stack trace information
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
In general, to fix this problem we should avoid returning raw exception messages to the client. Instead, we should log detailed error information on the server, and return only a generic, non-sensitive message (or structured error code) in the API response. That ensures developers can still debug issues from logs, but callers do not see internal details.
For this specific case, the minimal change is inside backfill_missing_embeddings in autogpt_platform/backend/backend/api/features/store/embeddings.py. In the except Exception as e block, we currently both log the error and return a dict containing "error": str(e). We should keep logging (possibly enhancing it to use logger.exception to capture a stack trace) but change the returned dict to omit str(e) and instead include a generic error message that does not disclose implementation details. The admin route in store_admin_routes.py can then safely return this dict as-is without exposing sensitive information.
Concretely:
- In
embeddings.py, lines 385–392: replacelogger.error(f"Failed to backfill embeddings: {e}")withlogger.exception("Failed to backfill embeddings")to log the full stack trace server-side, and replace the returned dict so that it no longer includes the raw exception string. For example, return:return { "processed": 0, "success": 0, "failed": 0, "error": "Failed to backfill embeddings", }
- No changes are needed in
store_admin_routes.pybecause once the returned dict no longer contains sensitive data, returning it from the endpoint is safe. - No new imports or helper methods are required; we only adjust logging and the content of the returned dict.
-
Copy modified lines R386-R388 -
Copy modified line R393
| @@ -383,12 +383,14 @@ | ||
| } | ||
|
|
||
| except Exception as e: | ||
| logger.error(f"Failed to backfill embeddings: {e}") | ||
| # Log full exception details (including stack trace) on the server, | ||
| # but return only a generic error message to the caller. | ||
| logger.exception("Failed to backfill embeddings") | ||
| return { | ||
| "processed": 0, | ||
| "success": 0, | ||
| "failed": 0, | ||
| "error": str(e), | ||
| "error": "Failed to backfill embeddings", | ||
| } | ||
|
|
||
|
|
|
This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request. |
- Updated all chat tools imports from backend.server.v2 to backend.api.features - Updated store imports (backfill_embeddings, db, hybrid_search) - Fixed CredentialsInput import in setup-wizard page
|
Thank you for your contribution! Before this PR can be considered for merging, several issues need to be addressed: Missing DescriptionPlease add a clear description of what this PR is implementing. From the code, it looks like you're adding a copilot/chat feature with various capabilities, but this needs to be explicitly stated and explained. Incomplete ChecklistThe PR template checklist needs to be filled out completely. This includes:
Title FormatThe PR title needs to follow conventional commit format. Based on the changes, something like ScopeThis appears to be a very large PR adding multiple features. Please explain:
Test PlanPlease add a test plan detailing how you've tested these changes and how reviewers can verify functionality. Once these items are addressed, we can proceed with a more detailed technical review of the implementation. The code itself looks interesting, but we need proper context and documentation to effectively review it. |
|
Conflicts have been resolved! 🎉 A maintainer will review the pull request shortly. |
|
Thank you for this comprehensive PR adding an AI copilot chat assistant! The implementation looks solid, but before this can be merged, a few items need to be addressed:
The code itself looks well-structured with a robust implementation of the chat assistant, including:
Once the description and checklist are completed, this PR should be ready for a more detailed review. |
| """ | ||
| try: | ||
| stats = await store_embeddings.get_embedding_stats() | ||
| return stats |
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.
@Swiftyos does this api needs to be publicly available? is this even used at all?
| User User @relation(fields: [userId], references: [id], onDelete: Cascade) | ||
| // User info | ||
| userName String? |
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.
why do we need user name here? is the user information already have this ?
| // User info | ||
| userName String? | ||
| jobTitle String? |
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.
I'm pretty sure the list of these will keep growing and changing, should we just have a simple 1 json column businessInfo/queryDetail or somethign and have a class will all of these fields to make them flexible ?
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.
Continuing this idea, I think we can allow this table for more guided prompting use cases not only for business understanding with merging evrything into a single oclumn and the additional of type column.
| // Stores vector embeddings for semantic search of store listings | ||
| // Uses pgvector extension for efficient similarity search | ||
| model StoreListingEmbedding { | ||
| id String @id @default(uuid()) |
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.
if storeListingVersionId already unique, why do we need another id at all? why not set the storeListingVersionId as id
| sequence Int | ||
| @@unique([sessionId, sequence]) | ||
| @@index([sessionId, sequence]) |
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.
no need for index, if you have unique
| Session ChatSession @relation(fields: [sessionId], references: [id], onDelete: Cascade) | ||
| // Message content | ||
| role String // "user", "assistant", "system", "tool", "function" |
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.
all these are definitely growing too, should we collapse them into one single json for simplicity and have a typed class for it ?
| functionCall Json? // Deprecated but kept for compatibility | ||
| // Ordering within session | ||
| sequence Int |
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.
This is a good idea, but what happens when we compact the conversation? how are we going to handle it?
| // Rate limiting counters (stored as JSON maps) | ||
| successfulAgentRuns Json @default("{}") // Map of graph_id -> count | ||
| successfulAgentSchedules Json @default("{}") // Map of graph_id -> count |
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.
do these two info need to live here? I'm pretty sure the rate-limit is user level not session level? we should share this info accross users, we need to align this.
| INDEX_PATH = Path(__file__).parent / "blocks_index.json" | ||
|
|
||
| # Stopwords for tokenization (same as index_blocks.py) | ||
| STOPWORDS = { |
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.
There are huge amount of duplicated code here being used accross files, I think this can cause issue in the future, we need to share & re-use
| ) | ||
|
|
||
| # Block IDs for various fixes | ||
| STORE_VALUE_BLOCK_ID = "1ff065e9-88e8-4358-9d82-8dc91f622ba9" |
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.
why can't we just point the id directly using the class.id ?
|
This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request. |
|
Conflicts have been resolved! 🎉 A maintainer will review the pull request shortly. |
|
Thank you for submitting this comprehensive PR for the AI copilot chat assistant! The code and documentation look excellent and well-structured. I noticed you mentioned at the top of the PR that this is "Not to be merged!" and will be split into multiple PRs. That's a good approach given the size and complexity of the changes. The implementation is thorough, with all the needed components:
Your test plan is comprehensive and the checklist is complete. The configuration requirements are clearly documented. When you're ready to split this into smaller PRs, I'd suggest considering logical boundaries like:
This would make the reviews more focused and manageable while maintaining the overall architecture vision. |
|
This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request. |
|
Thank you for your detailed PR submission! The implementation of the AutoGPT Copilot feature looks comprehensive and well-structured. However, I noticed you've explicitly stated "Not to be merged!" at the top of your PR description, indicating this is meant to be split into smaller PRs. This PR introduces a significant amount of new code across multiple components:
Breaking this into smaller, more focused PRs would indeed make review and testing more manageable. I recommend:
The code quality looks good, but reviewing such a large PR all at once is challenging. Please proceed with your plan to split this into multiple PRs for easier review. |
|
Thanks for this comprehensive PR that adds the AI copilot chat assistant (Otto) to AutoGPT! The implementation is thorough and well-documented. I see you've marked this as "Not to be merged!" and that you plan to split it into many PRs, which is a good approach given the size of the changes (the PR is marked as size/xl). The PR itself looks good from a review standpoint:
Since you're planning to split this into multiple PRs, I would recommend:
This will make the review process more manageable and help ensure each component gets proper attention before merging. Overall, excellent work on implementing this feature! The code quality looks good, and the approach of splitting it into smaller PRs is the right way to proceed with a change of this magnitude. |
…arch - Add SET LOCAL search_path TO platform, public; to queries using vector types - This ensures the vector type is found while keeping operators working - Fixes hybrid search on databases using platform schema Co-Authored-By: Claude Opus 4.5 <[email protected]>
Not to be merged!
Splitting this up into many PR's
Summary
This PR introduces AutoGPT Copilot ("Otto"), an AI assistant that helps users discover, create, and deploy custom business automation agents through a conversational chat interface.
What it does
The copilot provides an intelligent chat experience that:
Changes 🏗️
Backend (
backend/api/features/chat/):service.py- Core streaming chat service with OpenAI-compatible tool callingroutes.py- REST API endpoints for sessions and streamingconfig.py- Configuration for LLM providers (OpenRouter/OpenAI)model.py- Chat session and message data modelsdb.py- Prisma database operations for persistenceresponse_model.py- Streaming response types (SSE)tools/- 10 executable tools:find_agent.py- Search marketplace agentsfind_library_agent.py- Search user's saved agentsfind_block.py- Search available blockscreate_agent.py- Generate agents from descriptionsedit_agent.py- Modify existing agentsrun_agent.py- Execute agents with auto credential detectionrun_block.py- Execute single blocksagent_output.py- Retrieve execution resultssearch_docs.py- Search platform documentationadd_understanding.py- Save user business contextprompts/- System prompts for chat persona and onboardingFrontend (
frontend/src/components/contextual/Chat/):Chat.tsx- Main chat component with header and sessions drawerChatContainer.tsx- Message rendering with streaming supportuseChatSession.ts- Session lifecycle managementuseChatStream.ts- SSE streaming and message parsingDatabase:
ChatSessionandChatMessagePrisma modelsKey Features
Checklist 📋
For code changes:
For configuration changes:
.env.defaultis updated or already compatible with my changesdocker-compose.ymlis updated or already compatible with my changesConfiguration Requirements
Environment Variables:
CHAT_API_KEYorOPEN_ROUTER_API_KEYorOPENAI_API_KEY- LLM API keyCHAT_BASE_URL- Optional base URL (defaults to OpenRouter)CHAT_MODEL- Model selection (default:anthropic/claude-sonnet-4)CHAT_TITLE_MODEL- Fast model for title generation (default:openai/gpt-4o-mini)CHAT_SESSION_TTL- Redis session lifetime (default: 43200 seconds = 12 hours)Feature Flag:
CHATflag in LaunchDarkly controls visibility of the chat featureInfrastructure: