Skip to content

Conversation

@thisisfixer
Copy link
Contributor

@thisisfixer thisisfixer commented Aug 5, 2025

🏷️ Ticket

https://www.notion.so/Be-able-to-create-app-json-and-functions-json-for-remote-MCP-Server-2468378d6a478063892cc5b4ecb1787c

πŸ“ Description

This PR covers the first part of the effort to create App and Functions that are generated from a remote MCP Server.
(code is a bit messy for backward compatibility, need further cleanup after feature release)

  • handle the different OAuth2 pattern (dynamic registration)
  • change the function schema validation rules to cater to different types of protocol (e.g, visible field should only apply to rest protocol function). Some test cases are modified accordingly.
  • TBD: frontend schema filter logic need to be updated accordingly: if visible not present, assume all properties are visible
  • automate part of the process with cli command.
  • create runbook to generate app.json and functions.json file: https://www.notion.so/Add-Remote-MCP-Server-based-App-Functions-2458378d6a4780a9ae2ce7bfdf485582

Summary by cubic

Added a CLI command to generate app.json and functions.json files from a remote MCP server, supporting OAuth2 dynamic registration and automating part of the integration process.

  • New Features
    • New CLI tool connects to a remote MCP server, lists available tools, and generates functions.json for a specified app.
    • Supports OAuth2 authentication with dynamic client registration.
    • Handles cases where client_secret is optional for remote MCP apps.
    • Prints a summary and warnings for manual fixes if needed.

Summary by CodeRabbit

  • New Features

    • Added a CLI command to generate function definition files by querying an MCP server.
  • Improvements

    • Made OAuth2 client secret optional to better support dynamic client registration.
    • Enhanced function parameter validation with protocol-specific rules.
    • Updated input handling to inject default values only for REST protocol functions.
  • Other Changes

    • Added MCP protocol support in function metadata and system enums.
    • Improved visibility handling to show all properties if no visibility is specified.
    • Minor internal updates to authentication parameter ordering (no user-facing impact).
    • Updated mock app connector to use an optional input string parameter instead of a required one.

@vercel
Copy link

vercel bot commented Aug 5, 2025

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

Name Status Preview Comments Updated (UTC)
aci-dev-portal βœ… Ready (Inspect) Visit Preview πŸ’¬ Add feedback Aug 6, 2025 9:31pm

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Aug 5, 2025

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

A new CLI command was added to generate a JSON file of function definitions by querying an MCP server. Supporting changes include making the client_secret parameter optional in OAuth2-related schemas and the OAuth2Manager class, with corresponding constructor and argument order updates across relevant files. Additionally, a new enum value MCP was added to the protocol enum and database enum type, and the function metadata schema was extended to support MCP-specific metadata. Validation logic for function parameters was enhanced to support MCP protocol schemas, and filtering behavior for visible properties was updated to treat absence of the "visible" field as all properties visible. The mock app connector's echo function was modified to use an optional string parameter, with corresponding test updates.

Changes

Cohort / File(s) Change Summary
CLI Command Addition
backend/aci/cli/aci.py, backend/aci/cli/commands/mcp.py
Introduced a new CLI command generate_functions_file_from_mcp_server for generating function definitions from an MCP server. Registered the command in the main CLI group and implemented its logic, including server communication, OAuth2 authentication, error handling, response parsing (supporting JSON and SSE), content normalization, and file generation with validation and warnings.
OAuth2 Schema Updates
backend/aci/common/schemas/security_scheme.py
Made the client_secret field optional in the OAuth2Scheme and OAuth2SchemeCredentials classes, with a default of None. Added a TODO comment in OAuth2SchemeOverride regarding making client_secret optional depending on app data.
OAuth2 Manager Refactor
backend/aci/server/oauth2_manager.py
Updated the OAuth2Manager constructor to accept an optional client_secret parameter with default None. Updated constructor docstring accordingly and added a TODO comment about potentially adding a "resource" parameter for MCP compliance.
OAuth2 Argument Order Updates
backend/aci/server/routes/linked_accounts.py, backend/aci/server/security_credentials_manager.py
Reordered the client_secret argument in OAuth2Manager instantiations to match the updated constructor signature. No changes to logic or control flow.
Protocol Enum and DB Migration
backend/aci/common/enums.py, backend/aci/alembic/versions/2025_08_06_1817-c80cbb3ec1e9_add_new_enum_value_mcp_for_protocol.py
Added a new enum member MCP to the Protocol enum and introduced a corresponding new enum value in the PostgreSQL protocol enum type via an Alembic migration script. The downgrade function is empty (no removal logic).
Function Metadata Schema Update
backend/aci/common/schemas/function.py
Added a new Pydantic model MCPMetadata with MCP-specific fields. Updated the FunctionUpsert model to accept MCPMetadata in its protocol_data union and extended validation logic to map the MCP protocol to this new metadata class. Simplified parameter schema validation to use a unified function.
Function Parameter Validation Refactor
backend/aci/common/validator.py
Replaced previous common validation with protocol-dispatched validation functions for REST, CONNECTOR, and MCP protocols. Added strict REST validation including root-level schema checks and recursive validation of properties and visibility. Non-REST validation enforces schema constraints and required properties presence.
Visible Properties Filtering Update
backend/aci/common/processor.py, backend/aci/common/tests/test_processor_filter_visible_properties.py
Modified filter_visible_properties to treat absence of the "visible" field as all properties visible instead of filtering out all. Updated corresponding test to expect unchanged schema when "visible" is missing.
Mock App Connector Update
backend/aci/server/app_connectors/mock_app_connector.py, backend/aci/server/tests/dummy_apps/mock_app_connector/functions.json, backend/aci/server/tests/routes/functions/test_functions_execute_mock_app_connector_with_no_auth_account.py, backend/aci/server/tests/routes/functions/test_functions_execute_mock_app_connector_with_oauth2_account.py
Changed the echo method parameter from a required invisible string to an optional string with a default value. Updated the function JSON schema accordingly by renaming the parameter and removing it from required fields. Expanded test parameterizations to cover default and custom optional string values and updated expected security scheme metadata in test assertions.
Function Input Preprocessing Update
backend/aci/server/function_executors/base_executor.py
Modified input preprocessing to inject non-visible default values only for REST protocol functions, adding clarifying comments. Logging of input after defaults injection was also moved inside this conditional block.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant CLI
    participant MCPServer

    User->>CLI: Run generate_functions_file_from_mcp_server
    CLI->>MCPServer: Attempt tools/list (JSON-RPC)
    alt Initialization required
        MCPServer-->>CLI: HTTP error indicating init needed
        CLI->>MCPServer: initialize (JSON-RPC)
        MCPServer-->>CLI: Initialization response (with optional session ID)
    end
    CLI->>MCPServer: tools/list (JSON-RPC, with session ID if any)
    MCPServer-->>CLI: List of tools
    CLI->>CLI: Generate functions.json file from tool definitions
    CLI-->>User: Output diagnostics and file location
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~15 minutes

Poem

From server to schema, a hop and a skip,
The rabbit brings tools in a JSON zip.
Optional secrets now tucked away,
OAuth2 flows made easier today.
With a shiny new CLI hop in its stride,
The bunny delivers functions with pride!
πŸ‡βœ¨

Note

⚑️ Unit Test Generation is now available in beta!

Learn more here, or try it out under "Finishing Touches" below.

✨ Finishing Touches
  • πŸ“ Generate Docstrings
πŸ§ͺ Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch mcp/integration-via-remote-mcp-part1

πŸͺ§ Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai generate unit tests to generate unit tests for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@recurseml
Copy link

recurseml bot commented Aug 5, 2025

✨ No issues found! Your code is sparkling clean! ✨

Need help? Join our Discord for support!
https://discord.gg/qEjHQk64Z9

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (1)
backend/aci/cli/commands/mcp.py (1)

160-160: Address pagination support.

The TODO comment indicates pagination isn't handled yet, which could be problematic for MCP servers with many tools.

Would you like me to help implement pagination support or create an issue to track this requirement?

πŸ“œ Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

πŸ“₯ Commits

Reviewing files that changed from the base of the PR and between 610467f and 75461fb.

πŸ“’ Files selected for processing (6)
  • backend/aci/cli/aci.py (2 hunks)
  • backend/aci/cli/commands/mcp.py (1 hunks)
  • backend/aci/common/schemas/security_scheme.py (3 hunks)
  • backend/aci/server/oauth2_manager.py (3 hunks)
  • backend/aci/server/routes/linked_accounts.py (2 hunks)
  • backend/aci/server/security_credentials_manager.py (1 hunks)
🧰 Additional context used
πŸ““ Path-based instructions (4)
backend/**/*.py

πŸ“„ CodeRabbit Inference Engine (CLAUDE.md)

backend/**/*.py: Backend Python code must be compatible with Python 3.12+
Backend must use ruff for formatting and linting
Backend must use mypy for type checking

Files:

  • backend/aci/server/routes/linked_accounts.py
  • backend/aci/cli/aci.py
  • backend/aci/server/security_credentials_manager.py
  • backend/aci/server/oauth2_manager.py
  • backend/aci/common/schemas/security_scheme.py
  • backend/aci/cli/commands/mcp.py
backend/aci/cli/*.py

πŸ“„ CodeRabbit Inference Engine (CLAUDE.md)

Backend admin CLI commands should be implemented in backend/aci/cli/

Files:

  • backend/aci/cli/aci.py
backend/aci/server/*.py

πŸ“„ CodeRabbit Inference Engine (CLAUDE.md)

Backend FastAPI application code should be located in backend/aci/server/

Files:

  • backend/aci/server/security_credentials_manager.py
  • backend/aci/server/oauth2_manager.py
backend/aci/common/schemas/*.py

πŸ“„ CodeRabbit Inference Engine (CLAUDE.md)

Backend Pydantic models for validation should be defined in backend/aci/common/schemas/

Files:

  • backend/aci/common/schemas/security_scheme.py
🧠 Learnings (7)
πŸ“š Learning: in backend/aci/server/routes/linked_accounts.py, the scm (security credentials manager) module alrea...
Learnt from: alex-aipolabs
PR: aipotheosis-labs/aci#475
File: backend/aci/server/routes/linked_accounts.py:748-769
Timestamp: 2025-06-10T16:14:41.324Z
Learning: In backend/aci/server/routes/linked_accounts.py, the SCM (security credentials manager) module already handles errors internally, so additional error handling around `scm.get_security_credentials` calls is not needed.

Applied to files:

  • backend/aci/server/routes/linked_accounts.py
  • backend/aci/server/security_credentials_manager.py
πŸ“š Learning: in the oauth2 flow implementation for aci, the `rewrite_oauth2_authorization_url` method is called e...
Learnt from: thisisfixer
PR: aipotheosis-labs/aci#339
File: backend/aci/server/oauth2_manager.py:95-105
Timestamp: 2025-05-08T19:33:00.496Z
Learning: In the OAuth2 flow implementation for ACI, the `rewrite_oauth2_authorization_url` method is called externally after obtaining the authorization URL from `create_authorization_url`, rather than being integrated directly within the `create_authorization_url` method itself.

Applied to files:

  • backend/aci/server/routes/linked_accounts.py
  • backend/aci/server/security_credentials_manager.py
  • backend/aci/server/oauth2_manager.py
πŸ“š Learning: oauth2 errors in this codebase should use the oauth2error exception class with a 500 status code, ev...
Learnt from: thisisfixer
PR: aipotheosis-labs/aci#339
File: backend/aci/server/routes/linked_accounts.py:480-507
Timestamp: 2025-05-08T19:22:35.378Z
Learning: OAuth2 errors in this codebase should use the OAuth2Error exception class with a 500 status code, even for client-side issues like missing parameters or provider-returned errors.

Applied to files:

  • backend/aci/server/routes/linked_accounts.py
πŸ“š Learning: applies to backend/aci/cli/*.py : backend admin cli commands should be implemented in backend/aci/cl...
Learnt from: CR
PR: aipotheosis-labs/aci#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-05T09:39:27.629Z
Learning: Applies to backend/aci/cli/*.py : Backend admin CLI commands should be implemented in backend/aci/cli/

Applied to files:

  • backend/aci/cli/aci.py
  • backend/aci/cli/commands/mcp.py
πŸ“š Learning: applies to backend/aci/server/*.py : backend fastapi application code should be located in backend/a...
Learnt from: CR
PR: aipotheosis-labs/aci#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-05T09:39:27.629Z
Learning: Applies to backend/aci/server/*.py : Backend FastAPI application code should be located in backend/aci/server/

Applied to files:

  • backend/aci/cli/aci.py
πŸ“š Learning: applies to backend/aci/common/schemas/*.py : backend pydantic models for validation should be define...
Learnt from: CR
PR: aipotheosis-labs/aci#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-05T09:39:27.629Z
Learning: Applies to backend/aci/common/schemas/*.py : Backend Pydantic models for validation should be defined in backend/aci/common/schemas/

Applied to files:

  • backend/aci/common/schemas/security_scheme.py
πŸ“š Learning: aci integration requires security schemes to be defined as "api_key" with bearer prefix format rathe...
Learnt from: TooonyChen
PR: aipotheosis-labs/aci#569
File: backend/apps/x_com/app.json:8-14
Timestamp: 2025-08-01T13:44:52.669Z
Learning: ACI integration requires security schemes to be defined as "api_key" with Bearer prefix format rather than "http_bearer" scheme for proper functioning.

Applied to files:

  • backend/aci/common/schemas/security_scheme.py
⏰ 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). (4)
  • GitHub Check: cubic Β· AI code reviewer
  • GitHub Check: Compose Tests
  • GitHub Check: Format & Lint
  • GitHub Check: Format, Lint, and Test
πŸ”‡ Additional comments (14)
backend/aci/cli/aci.py (2)

11-11: LGTM!

The import addition follows the established pattern and is correctly placed alphabetically within the imports.


38-38: LGTM!

The command registration follows the established pattern and correctly adds the new MCP-related CLI functionality to the main CLI interface.

backend/aci/server/security_credentials_manager.py (1)

151-162: LGTM!

The parameter reordering correctly aligns with the updated OAuth2Manager constructor signature where client_secret is now optional and positioned after refresh_token_url. The functionality remains unchanged.

backend/aci/server/routes/linked_accounts.py (2)

354-363: LGTM!

The parameter reordering in the OAuth2Manager constructor call correctly aligns with the updated signature where client_secret is positioned after refresh_token_url.


495-504: LGTM!

The parameter reordering in the OAuth2Manager constructor call is consistent with the updated signature and maintains the OAuth2 callback flow functionality.

backend/aci/common/schemas/security_scheme.py (3)

48-55: LGTM!

The change to make client_secret optional is well-documented and appropriately supports MCP servers using dynamic client registration. The validation constraints are preserved for when the field is provided.


107-110: Good forward-thinking documentation.

The TODO comment appropriately identifies that OAuth2SchemeOverride.client_secret may need similar treatment depending on the OAuth2Scheme configuration. This shows good architectural awareness.


187-187: LGTM!

Making client_secret optional in the credentials model is consistent with the scheme definition and supports the MCP server dynamic registration pattern.

backend/aci/server/oauth2_manager.py (3)

17-27: LGTM!

The constructor signature change properly makes client_secret optional with appropriate typing (str | None) and default value (None). This supports MCP servers with dynamic client registration.


38-38: LGTM!

The updated docstring clearly documents when client_secret is optional, providing valuable context for MCP server-based applications using dynamic client registration.


101-101: Good forward-thinking comment.

The TODO about potentially needing the "resource" parameter shows good awareness of MCP specification requirements and suggests this area may need future attention.

backend/aci/cli/commands/mcp.py (3)

1-15: LGTM!

The imports are well-organized and appropriate for the CLI functionality. The MCP protocol version constant is clearly defined.


115-157: LGTM!

The initialize function is well-implemented with proper JSON-RPC 2.0 structure, appropriate error handling, and clean session ID extraction from response headers.


161-200: LGTM!

The tools listing function is well-implemented with proper JSON-RPC structure, comprehensive error handling, and appropriate type safety measures.

Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cubic analysis

2 issues found across 6 files β€’ Review in cubic

React with πŸ‘ or πŸ‘Ž to teach cubic. You can also tag @cubic-dev-ai to give feedback, ask questions, or re-run the review.

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
@cursor
Copy link

cursor bot commented Aug 6, 2025

🚨 Bugbot Trial Expired

Your Bugbot trial has expired. Please purchase a license in the Cursor dashboard to continue using Bugbot.

thisisfixer and others added 2 commits August 6, 2025 11:30
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
@cursor
Copy link

cursor bot commented Aug 6, 2025

🚨 Bugbot Trial Expired

Your Bugbot trial has expired. Please purchase a license in the Cursor dashboard to continue using Bugbot.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (4)
backend/aci/cli/commands/mcp.py (4)

30-33: Incomplete implementation for declared security schemes.

The help text declares support for "api-key" and "no-auth" security schemes, but only "oauth2" is implemented in the function body.


106-114: Broaden exception handling for initialization check.

The function only catches httpx.HTTPStatusError but other exceptions like connection errors, timeouts, or JSON-RPC errors could occur during the tools listing attempt.


212-221: Intentional double underscore creation triggers unnecessary warning.

The code intentionally creates function names with double underscores (f"{app_name}__{tool['name']}") but then warns about consecutive underscores, creating noise in the output.


51-57: Good error handling but incomplete implementation.

The use of click.BadParameter for CLI validation is appropriate. However, the function only implements OAuth2 authentication despite advertising "api-key" and "no-auth" in the CLI options.

🧹 Nitpick comments (1)
backend/aci/cli/commands/mcp.py (1)

162-202: LGTM! Solid tools listing implementation.

The function properly implements JSON-RPC 2.0 protocol with comprehensive error handling and response validation. The pagination TODO is noted for future enhancement.

πŸ“œ Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

πŸ“₯ Commits

Reviewing files that changed from the base of the PR and between 75461fb and 42c9ed2.

πŸ“’ Files selected for processing (1)
  • backend/aci/cli/commands/mcp.py (1 hunks)
🧰 Additional context used
πŸ““ Path-based instructions (1)
backend/**/*.py

πŸ“„ CodeRabbit Inference Engine (CLAUDE.md)

backend/**/*.py: Backend Python code must be compatible with Python 3.12+
Backend must use ruff for formatting and linting
Backend must use mypy for type checking

Files:

  • backend/aci/cli/commands/mcp.py
🧠 Learnings (5)
πŸ“š Learning: applies to backend/aci/cli/*.py : backend admin cli commands should be implemented in backend/aci/cl...
Learnt from: CR
PR: aipotheosis-labs/aci#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-05T09:39:27.629Z
Learning: Applies to backend/aci/cli/*.py : Backend admin CLI commands should be implemented in backend/aci/cli/

Applied to files:

  • backend/aci/cli/commands/mcp.py
πŸ“š Learning: aci integration requires security schemes to be defined as "api_key" with bearer prefix format rathe...
Learnt from: TooonyChen
PR: aipotheosis-labs/aci#569
File: backend/apps/x_com/app.json:8-14
Timestamp: 2025-08-01T13:44:52.669Z
Learning: ACI integration requires security schemes to be defined as "api_key" with Bearer prefix format rather than "http_bearer" scheme for proper functioning.

Applied to files:

  • backend/aci/cli/commands/mcp.py
πŸ“š Learning: in backend/aci/server/routes/linked_accounts.py, the scm (security credentials manager) module alrea...
Learnt from: alex-aipolabs
PR: aipotheosis-labs/aci#475
File: backend/aci/server/routes/linked_accounts.py:748-769
Timestamp: 2025-06-10T16:14:41.324Z
Learning: In backend/aci/server/routes/linked_accounts.py, the SCM (security credentials manager) module already handles errors internally, so additional error handling around `scm.get_security_credentials` calls is not needed.

Applied to files:

  • backend/aci/cli/commands/mcp.py
πŸ“š Learning: applies to backend/aci/common/schemas/*.py : backend pydantic models for validation should be define...
Learnt from: CR
PR: aipotheosis-labs/aci#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-05T09:39:27.629Z
Learning: Applies to backend/aci/common/schemas/*.py : Backend Pydantic models for validation should be defined in backend/aci/common/schemas/

Applied to files:

  • backend/aci/cli/commands/mcp.py
πŸ“š Learning: x.com api requires oauth 2.0 authorization code flow with pkce for user context endpoints, which is ...
Learnt from: TooonyChen
PR: aipotheosis-labs/aci#569
File: backend/apps/x_com/functions.json:221-274
Timestamp: 2025-08-01T13:49:10.008Z
Learning: X.com API requires OAuth 2.0 Authorization Code Flow with PKCE for user context endpoints, which is incompatible with ACI's current OAuth 2.0 User Context implementation, creating specific authentication gaps for certain endpoints like `/2/tweets/{id}/liking_users`.

Applied to files:

  • backend/aci/cli/commands/mcp.py
πŸ”‡ Additional comments (5)
backend/aci/cli/commands/mcp.py (5)

1-15: LGTM! Clean imports and setup.

The imports are well-organized and appropriate for the CLI functionality. The MCP protocol version constant is properly defined.


60-102: LGTM! Well-structured OAuth2 handler with good diagnostics.

The function properly orchestrates the MCP workflow with performance timing, error handling, and comprehensive reporting. The fix for the tools[0] access issue is correctly implemented.


117-159: LGTM! Proper JSON-RPC 2.0 implementation.

The function correctly implements the MCP initialize protocol with proper JSON-RPC formatting, error handling, and session management.


205-259: LGTM! Comprehensive function generation with good validation.

The function creation logic is well-implemented with proper naming conventions, validation warnings, and file organization. The output directory structure and JSON formatting are appropriate.


261-282: LGTM! Proper event-stream handling with clear exception.

The function now correctly raises a clear exception for unsupported event-stream responses instead of returning an empty dictionary, which addresses the previous concern about downstream processing issues.

@cursor
Copy link

cursor bot commented Aug 6, 2025

🚨 Bugbot Trial Expired

Your Bugbot trial has expired. Please purchase a license in the Cursor dashboard to continue using Bugbot.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (3)
backend/aci/cli/commands/mcp.py (3)

231-231: Intentional double underscore creation triggers unnecessary warning.

The code intentionally creates function names with double underscores (f"{app_name}__{tool['name']}") but the sanitization function will warn about consecutive underscores, creating noise in the output.

Use a single underscore separator to avoid the warning:

-        function_name = f"{app_name}__{_sanitize_orginal_mcp_tool_name(tool['name'])}"
+        function_name = f"{app_name}_{_sanitize_orginal_mcp_tool_name(tool['name'])}"

Or if double underscores are intentional, adjust the warning logic to be more specific about problematic patterns.


56-59: Incomplete implementation for declared security schemes.

The help text declares support for "api-key" and "no-auth" security schemes, but only "oauth2" is implemented in the function body.

Either implement the missing schemes or remove them from the choice options:

-    type=click.Choice(["oauth2", "api-key", "no-auth"], case_sensitive=False),
+    type=click.Choice(["oauth2"], case_sensitive=False),

127-135: Broaden exception handling for initialization check.

The function only catches httpx.HTTPStatusError but other exceptions like connection errors, timeouts, or JSON-RPC errors could occur during the tools listing attempt.

    try:
        _list_tools(mcp_server_url, access_token)
        rprint("  └─ [green]Initialize is not required[/green]")
        return False
-    except httpx.HTTPStatusError:
+    except (httpx.HTTPStatusError, httpx.RequestError, click.ClickException):
        rprint("  └─ [yellow]Initialize is required[/yellow]")
        return True
🧹 Nitpick comments (1)
backend/aci/cli/commands/mcp.py (1)

295-295: Fix typo in function name.

The function name has a typo: "orginal" should be "original".

-def _sanitize_orginal_mcp_tool_name(mcp_tool_name: str) -> str:
+def _sanitize_original_mcp_tool_name(mcp_tool_name: str) -> str:

Also update the function call on line 231:

-        function_name = f"{app_name}__{_sanitize_orginal_mcp_tool_name(tool['name'])}"
+        function_name = f"{app_name}__{_sanitize_original_mcp_tool_name(tool['name'])}"
πŸ“œ Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

πŸ“₯ Commits

Reviewing files that changed from the base of the PR and between d68e6fd and f43ced3.

πŸ“’ Files selected for processing (1)
  • backend/aci/cli/commands/mcp.py (1 hunks)
🧰 Additional context used
πŸ““ Path-based instructions (1)
backend/**/*.py

πŸ“„ CodeRabbit Inference Engine (CLAUDE.md)

backend/**/*.py: Backend Python code must be compatible with Python 3.12+
Backend must use ruff for formatting and linting
Backend must use mypy for type checking

Files:

  • backend/aci/cli/commands/mcp.py
🧠 Learnings (5)
πŸ“š Learning: applies to backend/aci/cli/*.py : backend admin cli commands should be implemented in backend/aci/cl...
Learnt from: CR
PR: aipotheosis-labs/aci#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-05T09:39:27.629Z
Learning: Applies to backend/aci/cli/*.py : Backend admin CLI commands should be implemented in backend/aci/cli/

Applied to files:

  • backend/aci/cli/commands/mcp.py
πŸ“š Learning: aci integration requires security schemes to be defined as "api_key" with bearer prefix format rathe...
Learnt from: TooonyChen
PR: aipotheosis-labs/aci#569
File: backend/apps/x_com/app.json:8-14
Timestamp: 2025-08-01T13:44:52.669Z
Learning: ACI integration requires security schemes to be defined as "api_key" with Bearer prefix format rather than "http_bearer" scheme for proper functioning.

Applied to files:

  • backend/aci/cli/commands/mcp.py
πŸ“š Learning: in backend/aci/server/routes/linked_accounts.py, the scm (security credentials manager) module alrea...
Learnt from: alex-aipolabs
PR: aipotheosis-labs/aci#475
File: backend/aci/server/routes/linked_accounts.py:748-769
Timestamp: 2025-06-10T16:14:41.324Z
Learning: In backend/aci/server/routes/linked_accounts.py, the SCM (security credentials manager) module already handles errors internally, so additional error handling around `scm.get_security_credentials` calls is not needed.

Applied to files:

  • backend/aci/cli/commands/mcp.py
πŸ“š Learning: applies to backend/aci/common/schemas/*.py : backend pydantic models for validation should be define...
Learnt from: CR
PR: aipotheosis-labs/aci#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-05T09:39:27.629Z
Learning: Applies to backend/aci/common/schemas/*.py : Backend Pydantic models for validation should be defined in backend/aci/common/schemas/

Applied to files:

  • backend/aci/cli/commands/mcp.py
πŸ“š Learning: x.com api requires oauth 2.0 authorization code flow with pkce for user context endpoints, which is ...
Learnt from: TooonyChen
PR: aipotheosis-labs/aci#569
File: backend/apps/x_com/functions.json:221-274
Timestamp: 2025-08-01T13:49:10.008Z
Learning: X.com API requires OAuth 2.0 Authorization Code Flow with PKCE for user context endpoints, which is incompatible with ACI's current OAuth 2.0 User Context implementation, creating specific authentication gaps for certain endpoints like `/2/tweets/{id}/liking_users`.

Applied to files:

  • backend/aci/cli/commands/mcp.py
⏰ 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). (3)
  • GitHub Check: Format, Lint, and Test
  • GitHub Check: Format & Lint
  • GitHub Check: Compose Tests
πŸ”‡ Additional comments (6)
backend/aci/cli/commands/mcp.py (6)

1-15: LGTM!

The imports are appropriate for the functionality and follow Python conventions. The MCP protocol version constant is well-defined.


17-40: LGTM!

The app name validation is thorough and uses appropriate error handling with click.BadParameter. The regex pattern and validation rules are clearly implemented.


86-124: LGTM!

The OAuth2 handler function is well-structured with clear orchestration of the MCP interaction flow. The timing metrics and summary report provide good visibility into the process.


138-181: LGTM!

The MCP initialization function properly implements JSON-RPC 2.0 protocol with appropriate error handling and session ID extraction. The implementation follows the specification correctly.


184-224: LGTM!

The tools listing function properly implements JSON-RPC 2.0 protocol with appropriate error handling and response validation. The use of cast for type safety is appropriate. The TODO for pagination handling is acceptable for initial implementation.


273-293: LGTM!

The MCP response parsing function properly handles both JSON and event-stream formats. The event-stream parsing correctly extracts JSON data from SSE format and includes appropriate error handling.

@cursor
Copy link

cursor bot commented Aug 6, 2025

🚨 Bugbot Trial Expired

Your Bugbot trial has expired. Please purchase a license in the Cursor dashboard to continue using Bugbot.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (3)
backend/aci/cli/commands/mcp.py (3)

232-232: Inconsistent underscore separator creates unnecessary warnings.

The function intentionally uses double underscores (__) as separator, but this creates consecutive underscores that will trigger warnings in the sanitization function, causing unnecessary noise in the output.

Apply this diff to use single underscore:

-        function_name = f"{app_name}__{_sanitize_orginal_tool_name(tool['name'])}"
+        function_name = f"{app_name}_{_sanitize_orginal_tool_name(tool['name'])}"

57-85: Remove unsupported security schemes from CLI choices.

The CLI advertises "api-key" and "no-auth" security schemes but only implements "oauth2" authentication. This will cause runtime failures when users select the unsupported options.

Apply this diff to match implementation with advertised choices:

-    type=click.Choice(["oauth2", "api-key", "no-auth"], case_sensitive=False),
+    type=click.Choice(["oauth2"], case_sensitive=False),

128-137: Broaden exception handling for initialization check.

The function only catches httpx.HTTPStatusError but other exceptions like connection errors, timeouts, or JSON-RPC errors could occur during the tools listing attempt, which should also indicate that initialization is required.

Apply this diff to catch broader exceptions:

     try:
         _list_tools(mcp_server_url, access_token)
         rprint("  └─ [green]Initialize is not required[/green]")
         return False
-    except httpx.HTTPStatusError:
+    except (httpx.HTTPStatusError, httpx.RequestError, click.ClickException):
         rprint("  └─ [yellow]Initialize is required[/yellow]")
         return True
🧹 Nitpick comments (2)
backend/aci/cli/commands/mcp.py (2)

184-186: Consider implementing pagination support.

The TODO comment indicates pagination is not yet handled, which could be an issue for MCP servers with many tools.

Would you like me to help implement pagination support or create an issue to track this enhancement?


327-327: Fix typo in function name.

The function name contains a typo: "orginal" should be "original".

Apply this diff to fix the typo:

-def _sanitize_orginal_tool_name(original_tool_name: str) -> str:
+def _sanitize_original_tool_name(original_tool_name: str) -> str:

Also update the call site on line 232:

-        function_name = f"{app_name}__{_sanitize_orginal_tool_name(tool['name'])}"
+        function_name = f"{app_name}__{_sanitize_original_tool_name(tool['name'])}"
πŸ“œ Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

πŸ“₯ Commits

Reviewing files that changed from the base of the PR and between f43ced3 and 2dee4d9.

πŸ“’ Files selected for processing (1)
  • backend/aci/cli/commands/mcp.py (1 hunks)
🧰 Additional context used
πŸ““ Path-based instructions (1)
backend/**/*.py

πŸ“„ CodeRabbit Inference Engine (CLAUDE.md)

backend/**/*.py: Backend Python code must be compatible with Python 3.12+
Backend must use ruff for formatting and linting
Backend must use mypy for type checking

Files:

  • backend/aci/cli/commands/mcp.py
🧠 Learnings (6)
πŸ““ Common learnings
Learnt from: CR
PR: aipotheosis-labs/aci#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-05T09:39:27.629Z
Learning: Applies to backend/apps/*/functions.json : Integration functions.json files must define API endpoints and their schemas
πŸ“š Learning: applies to backend/aci/cli/*.py : backend admin cli commands should be implemented in backend/aci/cl...
Learnt from: CR
PR: aipotheosis-labs/aci#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-05T09:39:27.629Z
Learning: Applies to backend/aci/cli/*.py : Backend admin CLI commands should be implemented in backend/aci/cli/

Applied to files:

  • backend/aci/cli/commands/mcp.py
πŸ“š Learning: aci integration requires security schemes to be defined as "api_key" with bearer prefix format rathe...
Learnt from: TooonyChen
PR: aipotheosis-labs/aci#569
File: backend/apps/x_com/app.json:8-14
Timestamp: 2025-08-01T13:44:52.669Z
Learning: ACI integration requires security schemes to be defined as "api_key" with Bearer prefix format rather than "http_bearer" scheme for proper functioning.

Applied to files:

  • backend/aci/cli/commands/mcp.py
πŸ“š Learning: in backend/aci/server/routes/linked_accounts.py, the scm (security credentials manager) module alrea...
Learnt from: alex-aipolabs
PR: aipotheosis-labs/aci#475
File: backend/aci/server/routes/linked_accounts.py:748-769
Timestamp: 2025-06-10T16:14:41.324Z
Learning: In backend/aci/server/routes/linked_accounts.py, the SCM (security credentials manager) module already handles errors internally, so additional error handling around `scm.get_security_credentials` calls is not needed.

Applied to files:

  • backend/aci/cli/commands/mcp.py
πŸ“š Learning: applies to backend/aci/common/schemas/*.py : backend pydantic models for validation should be define...
Learnt from: CR
PR: aipotheosis-labs/aci#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-05T09:39:27.629Z
Learning: Applies to backend/aci/common/schemas/*.py : Backend Pydantic models for validation should be defined in backend/aci/common/schemas/

Applied to files:

  • backend/aci/cli/commands/mcp.py
πŸ“š Learning: x.com api requires oauth 2.0 authorization code flow with pkce for user context endpoints, which is ...
Learnt from: TooonyChen
PR: aipotheosis-labs/aci#569
File: backend/apps/x_com/functions.json:221-274
Timestamp: 2025-08-01T13:49:10.008Z
Learning: X.com API requires OAuth 2.0 Authorization Code Flow with PKCE for user context endpoints, which is incompatible with ACI's current OAuth 2.0 User Context implementation, creating specific authentication gaps for certain endpoints like `/2/tweets/{id}/liking_users`.

Applied to files:

  • backend/aci/cli/commands/mcp.py
⏰ 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). (3)
  • GitHub Check: Format, Lint, and Test
  • GitHub Check: Compose Tests
  • GitHub Check: Format & Lint
πŸ”‡ Additional comments (9)
backend/aci/cli/commands/mcp.py (9)

1-16: LGTM!

The imports are appropriate for the CLI command functionality, and the MCP protocol version constant is properly defined.


18-41: LGTM!

The app name validation is comprehensive, checking for valid characters, consecutive underscores, and leading/trailing underscores with clear error messages.


87-125: LGTM!

The OAuth2 handler properly orchestrates the MCP server workflow with good timing metrics and status reporting.


139-182: LGTM!

The initialize function properly implements JSON-RPC 2.0 initialize request with appropriate error handling and session ID extraction.


187-225: LGTM!

The tools listing function properly implements JSON-RPC 2.0 with good error handling and response validation.


227-231: LGTM!

The function file creation logic properly generates the functions.json structure with appropriate metadata, validation, and directory handling.

Also applies to: 233-282


284-304: LGTM!

The response parsing function properly handles both JSON and event-stream formats with appropriate error handling for invalid SSE responses.


306-325: LGTM!

The content normalization function effectively strips formatting differences while preserving semantic content for change detection.


328-352: LGTM!

The tool name sanitization logic properly handles character conversion, consecutive underscores, and edge cases with appropriate warnings.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (5)
backend/aci/cli/commands/mcp.py (5)

62-65: Fix advertised but unimplemented security schemes

The CLI advertises "api-key" and "no-auth" security schemes but only "oauth2" is implemented, causing runtime failures when users select other options.

Remove unsupported options from the choice list:

-    type=click.Choice(["oauth2", "api-key", "no-auth"], case_sensitive=False),
+    type=click.Choice(["oauth2"], case_sensitive=False),

88-89: Use click.BadParameter for consistent CLI error handling

Replace ValueError with click.BadParameter for consistent CLI error patterns:

-            raise click.BadParameter(f"Unsupported security scheme: {security_scheme}")
+            raise click.BadParameter(f"Unsupported security scheme: {security_scheme}")

139-141: Broaden exception handling for initialization check

The function only catches httpx.HTTPStatusError but should handle other connection-related errors:

-    except httpx.HTTPStatusError:
+    except (httpx.HTTPStatusError, httpx.RequestError, click.ClickException):

237-237: Intentional double underscore creation triggers validation warnings

The function name construction uses double underscores (f"{app_name}__{tool_name}") which intentionally creates consecutive underscores, but this conflicts with the app name validation that prohibits consecutive underscores.

Use single underscore to avoid validation conflicts:

-        function_name = f"{app_name}__{_sanitize_orginal_tool_name(tool['name'])}"
+        function_name = f"{app_name}_{_sanitize_orginal_tool_name(tool['name'])}"

300-315: Implement proper event-stream parsing

The function now properly parses event-stream responses instead of returning empty dict, which resolves the downstream processing issue.

🧹 Nitpick comments (1)
backend/aci/cli/commands/mcp.py (1)

339-339: Fix typo in function name

Function name contains a typo: "orginal" should be "original".

-def _sanitize_orginal_tool_name(original_tool_name: str) -> str:
+def _sanitize_original_tool_name(original_tool_name: str) -> str:

Also update the call site at line 237:

-        function_name = f"{app_name}__{_sanitize_orginal_tool_name(tool['name'])}"
+        function_name = f"{app_name}_{_sanitize_original_tool_name(tool['name'])}"
πŸ“œ Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

πŸ“₯ Commits

Reviewing files that changed from the base of the PR and between 2dee4d9 and 38b5e5a.

πŸ“’ Files selected for processing (1)
  • backend/aci/cli/commands/mcp.py (1 hunks)
🧰 Additional context used
πŸ““ Path-based instructions (1)
backend/**/*.py

πŸ“„ CodeRabbit Inference Engine (CLAUDE.md)

backend/**/*.py: Backend Python code must be compatible with Python 3.12+
Backend must use ruff for formatting and linting
Backend must use mypy for type checking

Files:

  • backend/aci/cli/commands/mcp.py
🧠 Learnings (5)
πŸ“š Learning: applies to backend/aci/cli/*.py : backend admin cli commands should be implemented in backend/aci/cl...
Learnt from: CR
PR: aipotheosis-labs/aci#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-05T09:39:27.629Z
Learning: Applies to backend/aci/cli/*.py : Backend admin CLI commands should be implemented in backend/aci/cli/

Applied to files:

  • backend/aci/cli/commands/mcp.py
πŸ“š Learning: aci integration requires security schemes to be defined as "api_key" with bearer prefix format rathe...
Learnt from: TooonyChen
PR: aipotheosis-labs/aci#569
File: backend/apps/x_com/app.json:8-14
Timestamp: 2025-08-01T13:44:52.669Z
Learning: ACI integration requires security schemes to be defined as "api_key" with Bearer prefix format rather than "http_bearer" scheme for proper functioning.

Applied to files:

  • backend/aci/cli/commands/mcp.py
πŸ“š Learning: in backend/aci/server/routes/linked_accounts.py, the scm (security credentials manager) module alrea...
Learnt from: alex-aipolabs
PR: aipotheosis-labs/aci#475
File: backend/aci/server/routes/linked_accounts.py:748-769
Timestamp: 2025-06-10T16:14:41.324Z
Learning: In backend/aci/server/routes/linked_accounts.py, the SCM (security credentials manager) module already handles errors internally, so additional error handling around `scm.get_security_credentials` calls is not needed.

Applied to files:

  • backend/aci/cli/commands/mcp.py
πŸ“š Learning: applies to backend/aci/common/schemas/*.py : backend pydantic models for validation should be define...
Learnt from: CR
PR: aipotheosis-labs/aci#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-05T09:39:27.629Z
Learning: Applies to backend/aci/common/schemas/*.py : Backend Pydantic models for validation should be defined in backend/aci/common/schemas/

Applied to files:

  • backend/aci/cli/commands/mcp.py
πŸ“š Learning: x.com api requires oauth 2.0 authorization code flow with pkce for user context endpoints, which is ...
Learnt from: TooonyChen
PR: aipotheosis-labs/aci#569
File: backend/apps/x_com/functions.json:221-274
Timestamp: 2025-08-01T13:49:10.008Z
Learning: X.com API requires OAuth 2.0 Authorization Code Flow with PKCE for user context endpoints, which is incompatible with ACI's current OAuth 2.0 User Context implementation, creating specific authentication gaps for certain endpoints like `/2/tweets/{id}/liking_users`.

Applied to files:

  • backend/aci/cli/commands/mcp.py
⏰ 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). (3)
  • GitHub Check: Format, Lint, and Test
  • GitHub Check: Format & Lint
  • GitHub Check: Compose Tests
πŸ”‡ Additional comments (4)
backend/aci/cli/commands/mcp.py (4)

1-21: LGTM - Clean imports and configuration setup

The imports are well-organized and the OpenAI client initialization follows the expected pattern for accessing configuration.


23-46: LGTM - Robust app name validation

The validation function properly handles all edge cases for app name formatting including character validation, consecutive underscores, and leading/trailing underscores. Error messages are clear and actionable.


371-371: Ignore the β€œgpt-4.1” model availability warning
The OpenAI API does support β€œgpt-4.1” as a valid model identifier, so no changes are needed here.

Likely an incorrect or invalid review comment.


371-376: Ignore the suggested change: openai_client.responses.create is a valid OpenAI Python client method

The OpenAI Python library ships a dedicated Responses API (responses.create) that returns an output_text field, distinct from the chat.completions.create endpoint. This method is officially documented and not deprecated, so no changes are needed here.

Likely an incorrect or invalid review comment.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
backend/aci/common/schemas/function.py (1)

28-32: Well-defined MCPMetadata class.

The MCPMetadata class is properly structured with descriptive field names and appropriate types. Consider adding field descriptions using Pydantic's Field() with description parameter for better API documentation.

 class MCPMetadata(BaseModel):
-    original_tool_name: str
-    original_tool_description_hash: str
-    original_tool_input_schema_hash: str
-    need_initialize: bool
+    original_tool_name: str = Field(description="Original name of the MCP tool")
+    original_tool_description_hash: str = Field(description="Hash of the original tool description")
+    original_tool_input_schema_hash: str = Field(description="Hash of the original tool input schema")
+    need_initialize: bool = Field(description="Whether the tool requires initialization")
πŸ“œ Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

πŸ“₯ Commits

Reviewing files that changed from the base of the PR and between 38b5e5a and f96a3e9.

πŸ“’ Files selected for processing (3)
  • backend/aci/alembic/versions/2025_08_06_1817-c80cbb3ec1e9_add_new_enum_value_mcp_for_protocol.py (1 hunks)
  • backend/aci/common/enums.py (1 hunks)
  • backend/aci/common/schemas/function.py (3 hunks)
βœ… Files skipped from review due to trivial changes (1)
  • backend/aci/common/enums.py
🧰 Additional context used
πŸ““ Path-based instructions (2)
backend/**/*.py

πŸ“„ CodeRabbit Inference Engine (CLAUDE.md)

backend/**/*.py: Backend Python code must be compatible with Python 3.12+
Backend must use ruff for formatting and linting
Backend must use mypy for type checking

Files:

  • backend/aci/common/schemas/function.py
  • backend/aci/alembic/versions/2025_08_06_1817-c80cbb3ec1e9_add_new_enum_value_mcp_for_protocol.py
backend/aci/common/schemas/*.py

πŸ“„ CodeRabbit Inference Engine (CLAUDE.md)

Backend Pydantic models for validation should be defined in backend/aci/common/schemas/

Files:

  • backend/aci/common/schemas/function.py
🧠 Learnings (1)
πŸ“š Learning: applies to backend/aci/common/schemas/*.py : backend pydantic models for validation should be define...
Learnt from: CR
PR: aipotheosis-labs/aci#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-05T09:39:27.629Z
Learning: Applies to backend/aci/common/schemas/*.py : Backend Pydantic models for validation should be defined in backend/aci/common/schemas/

Applied to files:

  • backend/aci/common/schemas/function.py
⏰ 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). (3)
  • GitHub Check: Format, Lint, and Test
  • GitHub Check: Format & Lint
  • GitHub Check: Compose Tests
πŸ”‡ Additional comments (2)
backend/aci/common/schemas/function.py (2)

50-51: Correct union type extension for MCP protocol support.

The protocol_data field has been properly extended to include MCPMetadata in the union type. The placement between RestMetadata and ConnectorMetadata is logical, maintaining the structured validation before the catch-all ConnectorMetadata.


83-83: Proper protocol validation mapping for MCP.

The addition of Protocol.MCP to MCPMetadata mapping in the validation method ensures type consistency between the protocol field and protocol_data field for MCP functions.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
backend/aci/common/validator.py (1)

19-54: Non-REST protocol validation is well-implemented.

The validation logic appropriately handles CONNECTOR and MCP protocols with more flexible rules than REST. The recursive validation pattern and descriptive error messages are excellent.

Consider addressing the TODO comment about visible field validation (lines 34-38) in a future PR to fully enforce protocol-specific constraints for CONNECTOR/MCP protocols.

πŸ“œ Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

πŸ“₯ Commits

Reviewing files that changed from the base of the PR and between f96a3e9 and e765913.

πŸ“’ Files selected for processing (9)
  • backend/aci/common/processor.py (1 hunks)
  • backend/aci/common/schemas/function.py (5 hunks)
  • backend/aci/common/tests/test_processor_filter_visible_properties.py (1 hunks)
  • backend/aci/common/validator.py (4 hunks)
  • backend/aci/server/app_connectors/mock_app_connector.py (1 hunks)
  • backend/aci/server/function_executors/base_executor.py (3 hunks)
  • backend/aci/server/tests/dummy_apps/mock_app_connector/functions.json (1 hunks)
  • backend/aci/server/tests/routes/functions/test_functions_execute_mock_app_connector_with_no_auth_account.py (1 hunks)
  • backend/aci/server/tests/routes/functions/test_functions_execute_mock_app_connector_with_oauth2_account.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • backend/aci/common/schemas/function.py
🧰 Additional context used
πŸ““ Path-based instructions (2)
backend/**/*.py

πŸ“„ CodeRabbit Inference Engine (CLAUDE.md)

backend/**/*.py: Backend Python code must be compatible with Python 3.12+
Backend must use ruff for formatting and linting
Backend must use mypy for type checking

Files:

  • backend/aci/common/tests/test_processor_filter_visible_properties.py
  • backend/aci/common/processor.py
  • backend/aci/server/function_executors/base_executor.py
  • backend/aci/server/tests/routes/functions/test_functions_execute_mock_app_connector_with_no_auth_account.py
  • backend/aci/server/app_connectors/mock_app_connector.py
  • backend/aci/server/tests/routes/functions/test_functions_execute_mock_app_connector_with_oauth2_account.py
  • backend/aci/common/validator.py
backend/**/*test*.py

πŸ“„ CodeRabbit Inference Engine (CLAUDE.md)

Backend must use pytest for testing in an isolated test environment

Files:

  • backend/aci/common/tests/test_processor_filter_visible_properties.py
  • backend/aci/server/tests/routes/functions/test_functions_execute_mock_app_connector_with_no_auth_account.py
  • backend/aci/server/tests/routes/functions/test_functions_execute_mock_app_connector_with_oauth2_account.py
🧠 Learnings (6)
πŸ““ Common learnings
Learnt from: CR
PR: aipotheosis-labs/aci#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-05T09:39:27.629Z
Learning: Applies to backend/apps/*/functions.json : Integration functions.json files must define API endpoints and their schemas
πŸ“š Learning: in backend/aci/server/routes/linked_accounts.py, the scm (security credentials manager) module alrea...
Learnt from: alex-aipolabs
PR: aipotheosis-labs/aci#475
File: backend/aci/server/routes/linked_accounts.py:748-769
Timestamp: 2025-06-10T16:14:41.324Z
Learning: In backend/aci/server/routes/linked_accounts.py, the SCM (security credentials manager) module already handles errors internally, so additional error handling around `scm.get_security_credentials` calls is not needed.

Applied to files:

  • backend/aci/server/tests/routes/functions/test_functions_execute_mock_app_connector_with_no_auth_account.py
πŸ“š Learning: applies to backend/apps/*/functions.json : integration functions.json files must define api endpoint...
Learnt from: CR
PR: aipotheosis-labs/aci#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-05T09:39:27.629Z
Learning: Applies to backend/apps/*/functions.json : Integration functions.json files must define API endpoints and their schemas

Applied to files:

  • backend/aci/server/tests/dummy_apps/mock_app_connector/functions.json
πŸ“š Learning: in the oauth2 flow implementation for aci, the `rewrite_oauth2_authorization_url` method is called e...
Learnt from: thisisfixer
PR: aipotheosis-labs/aci#339
File: backend/aci/server/oauth2_manager.py:95-105
Timestamp: 2025-05-08T19:33:00.496Z
Learning: In the OAuth2 flow implementation for ACI, the `rewrite_oauth2_authorization_url` method is called externally after obtaining the authorization URL from `create_authorization_url`, rather than being integrated directly within the `create_authorization_url` method itself.

Applied to files:

  • backend/aci/server/tests/routes/functions/test_functions_execute_mock_app_connector_with_oauth2_account.py
πŸ“š Learning: applies to backend/aci/common/schemas/*.py : backend pydantic models for validation should be define...
Learnt from: CR
PR: aipotheosis-labs/aci#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-05T09:39:27.629Z
Learning: Applies to backend/aci/common/schemas/*.py : Backend Pydantic models for validation should be defined in backend/aci/common/schemas/

Applied to files:

  • backend/aci/common/validator.py
πŸ“š Learning: the validate_user_access_to_org function in backend/aci/server/acl.py is planned to be removed in th...
Learnt from: alex-aipolabs
PR: aipotheosis-labs/aci#453
File: backend/aci/server/acl.py:22-24
Timestamp: 2025-06-04T17:35:08.995Z
Learning: The validate_user_access_to_org function in backend/aci/server/acl.py is planned to be removed in the near future, so documentation improvements are not needed for this function.

Applied to files:

  • backend/aci/common/validator.py
🧬 Code Graph Analysis (4)
backend/aci/server/function_executors/base_executor.py (2)
backend/aci/common/enums.py (1)
  • Protocol (24-35)
backend/aci/common/processor.py (1)
  • inject_required_but_invisible_defaults (59-85)
backend/aci/server/tests/routes/functions/test_functions_execute_mock_app_connector_with_no_auth_account.py (1)
backend/aci/common/schemas/security_scheme.py (2)
  • NoAuthScheme (143-150)
  • NoAuthSchemeCredentials (206-214)
backend/aci/server/tests/routes/functions/test_functions_execute_mock_app_connector_with_oauth2_account.py (1)
backend/aci/common/schemas/security_scheme.py (2)
  • OAuth2Scheme (28-83)
  • OAuth2SchemeCredentials (180-195)
backend/aci/common/validator.py (1)
backend/aci/common/enums.py (2)
  • HttpLocation (38-43)
  • Protocol (24-35)
⏰ 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). (3)
  • GitHub Check: Format, Lint, and Test
  • GitHub Check: Compose Tests
  • GitHub Check: Format & Lint
πŸ”‡ Additional comments (14)
backend/aci/server/app_connectors/mock_app_connector.py (2)

41-41: LGTM! Parameter change is consistent.

The parameter has been properly renamed and made optional with a sensible default value.


49-49: LGTM! Return value key updated consistently.

The return dictionary key has been updated to match the renamed parameter.

backend/aci/common/tests/test_processor_filter_visible_properties.py (1)

34-34: LGTM! Test correctly reflects the new filtering behavior.

The assertion properly verifies that when the "visible" field is absent, all properties remain visible, which aligns with the updated processor logic.

backend/aci/common/processor.py (1)

22-33: LGTM! Backward compatibility change for MCP protocol support.

The logic correctly handles the case where "visible" field is missing by treating all properties as visible. The comprehensive comments and TODO note indicate this is a temporary solution for MCP protocol integration.

backend/aci/server/function_executors/base_executor.py (3)

8-8: LGTM! Import added for protocol-specific logic.

The Protocol enum import is necessary for the conditional logic that follows.


55-56: LGTM! Clarifying comment added.

The comment properly explains the current limitation and future intention for the filter_visible_properties function.


73-82: LGTM! Protocol-specific default injection.

The conditional logic correctly restricts default value injection to REST protocol functions only, which is appropriate for MCP integration. The debug logging is properly moved inside the conditional block.

backend/aci/server/tests/routes/functions/test_functions_execute_mock_app_connector_with_no_auth_account.py (2)

26-30: LGTM! Test case updated to match parameter changes.

The first test case properly reflects the parameter rename and includes appropriate security scheme metadata validation.


32-50: LGTM! Additional test case for custom optional parameter value.

The second test case provides good coverage by testing custom values for the optional parameter while maintaining consistent security scheme metadata validation.

backend/aci/server/tests/dummy_apps/mock_app_connector/functions.json (1)

32-38: LGTM! Parameter schema correctly updated for optional handling.

The parameter rename from "input_required_invisible_string" to "input_optional_string" and its removal from the required array accurately reflects the intended optional nature. The updated description and repositioned "additionalProperties" field maintain schema validity while supporting the new optional parameter pattern.

backend/aci/server/tests/routes/functions/test_functions_execute_mock_app_connector_with_oauth2_account.py (1)

11-51: Excellent test coverage for optional parameter scenarios.

The expanded parameterization effectively tests both default value injection (when the optional parameter is omitted) and custom value handling (when explicitly provided). The consistent parameter naming and proper inclusion of OAuth2 security scheme metadata in expected responses ensure thorough validation of the updated mock connector behavior.

backend/aci/common/validator.py (3)

4-16: Well-designed protocol-specific validation entry point.

The new validate_function_parameters_schema function provides clean protocol-based dispatch using a match statement, properly handling REST, CONNECTOR, and MCP protocols with appropriate error handling for invalid protocols.


57-127: REST protocol validation maintains backward compatibility.

The REST-specific validation function correctly preserves all existing strict validation rules while properly calling the root-level validator and maintaining recursive validation with the REST-specific function. This ensures backward compatibility for existing REST protocol functions.


130-167: Root-level REST validation is properly separated and implemented.

The root-level validation function cleanly handles top-level schema constraints, properly using the HttpLocation enum for allowed keys and providing specific error messages for various validation failures.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant