Skip to content
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

fix: cursor use ollama verify issue #28

Closed
wants to merge 0 commits into from

Conversation

jincdream
Copy link

@jincdream jincdream commented Dec 10, 2024

fixes: #27

This pull request includes significant changes to the curxy project, focusing on refactoring the proxy application and updating the project metadata. The most important changes include modifying the project name and version, refactoring the proxy application into a class, and updating utility functions.

Project metadata updates:

  • deno.json: Changed the project name from @ryoppippi/curxy to @jinc/curxy and updated the version from 0.1.8 to 0.2.0.

Refactoring the proxy application:

  • main.ts: Replaced createApp function import with ProxyApp class import and updated the instantiation and usage accordingly. [1] [2]
  • proxy.ts: Refactored the proxy application into a ProxyApp class, encapsulating all related methods and properties, and updated the request handling logic.

Utility function enhancements:

  • util.ts: Added isCursorRequest and isOpenAIModel utility functions and updated chooseEndpoint to use isOpenAIModel. [1] [2]

Summary by CodeRabbit

  • New Features

    • Updated versioning for the package.
    • Introduced new utility functions for URL validation and endpoint selection.
  • Refactor

    • Transitioned proxy server initialization to an object-oriented design with the ProxyApp class.
    • Improved organization of request handling and response management.
  • Bug Fixes

    • Enhanced error handling for model fetching.

Copy link

coderabbitai bot commented Dec 10, 2024

Walkthrough

The pull request introduces an update to the deno.json version from 0.1.8 to 0.2.0. Significant changes are made in main.ts and proxy.ts, where the application transitions to an object-oriented design through the introduction of the ProxyApp class. This class encapsulates application logic and request handling methods. Additionally, new utility functions are added in util.ts for enhanced URL validation. The overall structure remains intact while improving maintainability.

Changes

File Change Summary
deno.json Version updated from 0.1.8 to 0.2.0. No structural changes to exports, tasks, imports, or publish.
main.ts Replaced createApp function with ProxyApp class instantiation. Adjusted error handling and formatting.
proxy.ts Refactored to implement ProxyApp class. Added methods for request handling, CORS management, and model verification. Improved error handling for model fetching.
util.ts Added functions isCursorRequest and isOpenAIModel. Updated chooseEndpoint to use isOpenAIModel.

Possibly related PRs

  • fix(ollama): support for OPTIONS and GET requests #16: The changes in proxy.ts related to handling OPTIONS and GET requests are directly connected to the modifications in the createApp method and the overall structure of the ProxyApp class in the main PR, as both involve enhancements to request handling and CORS support.

Suggested reviewers

  • ryoppippi

Poem

In the code where rabbits play,
A new class hops in today.
With methods neat and functions bright,
Our proxy's now a joyful sight!
Version bumped, the paths are clear,
Let's celebrate with a happy cheer! 🐇✨


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 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.
    • Generate unit testing code for this file.
    • 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 generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • 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 generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @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. (Experiment)
  • @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.

Copy link

@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: 3

🧹 Outside diff range and nitpick comments (5)
proxy.ts (3)

37-37: Replace 'void' with 'undefined' in return type to avoid confusion

Using void in a union type can be confusing. Consider replacing void with undefined for clarity.

Apply this diff to update the return type:

-private handleAllRequest(c: any, next: () => Promise<any>): Response | Promise<void | Response> {
+private handleAllRequest(c: any, next: () => Promise<any>): Response | Promise<Response | undefined> {
🧰 Tools
🪛 Biome (1.9.4)

[error] 37-37: void is confusing inside a union type.

Unsafe fix: Use undefined instead.

(lint/suspicious/noConfusingVoidType)


97-100: Improve type safety by specifying explicit types instead of 'any'

The method isVerifyRequest uses parameters typed as any. Specifying explicit types enhances type safety and code readability.

Consider defining a type or interface for the context c and the json object. For example:

interface RequestContext {
  req: Request;
  res: Response;
  // other properties...
}

interface JsonBody {
  model: string;
  stream: boolean;
  // other properties...
}

Then, update the method signature:

-private isVerifyRequest(c: any, json: any) {
+private isVerifyRequest(c: RequestContext, json: JsonBody) {

190-207: Handle potential errors when parsing JSON in 'getModelsList'

In the getModelsList method, if the response is not valid JSON, res.json() could throw an error. Consider adding a check to handle this scenario gracefully.

Apply this diff to catch JSON parsing errors:

 try {
   const res = await fetch(req);
-  const data = await res.json();
+  const data = await res.json().catch(() => null);
+  if (!data) {
+    throw new Error("Invalid JSON response");
+  }
   return data.models || [];
 } catch (error) {
main.ts (1)

90-90: Fix typo: 'OpenAl' should be 'OpenAI'

There's a typo in the console output. The term 'OpenAl' should be corrected to 'OpenAI' to reflect the proper name.

Apply this diff to correct the typo:

 italic(`Override OpenAl Base URL`)
+italic(`Override OpenAI Base URL`)
util.ts (1)

42-44: Avoid exporting from a test file by separating tests into a dedicated file

Static analysis indicates that exports are present in a test file, which is discouraged. Consider moving the test cases to a separate file to adhere to best practices.

Steps to refactor:

  1. Create a new file named util.test.ts for the test cases.
  2. Move all the Deno.test blocks from util.ts to util.test.ts.
  3. Ensure the imports in util.test.ts are updated accordingly.
  4. Keep the utility functions and exports in util.ts.

This separation enhances code organization and maintainability.

🧰 Tools
🪛 Biome (1.9.4)

[error] 43-44: Do not export from a test file.

(lint/suspicious/noExportsInTest)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 78e79df and ec22a62.

⛔ Files ignored due to path filters (1)
  • deno.lock is excluded by !**/*.lock
📒 Files selected for processing (4)
  • deno.json (1 hunks)
  • main.ts (2 hunks)
  • proxy.ts (1 hunks)
  • util.ts (2 hunks)
✅ Files skipped from review due to trivial changes (1)
  • deno.json
🧰 Additional context used
🪛 Biome (1.9.4)
util.ts

[error] 43-44: Do not export from a test file.

(lint/suspicious/noExportsInTest)

proxy.ts

[error] 37-37: void is confusing inside a union type.

Unsafe fix: Use undefined instead.

(lint/suspicious/noConfusingVoidType)

proxy.ts Outdated Show resolved Hide resolved
proxy.ts Outdated Show resolved Hide resolved
util.ts Outdated Show resolved Hide resolved
Copy link

@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

🧹 Outside diff range and nitpick comments (5)
proxy.ts (5)

Line range hint 1-30: LGTM! Consider adding TypeScript interfaces for better type safety.

The class structure is well-organized with proper encapsulation and documentation. However, consider defining interfaces for the configuration and request/response types to improve type safety.

Add these type definitions at the beginning of the file:

interface ProxyConfig {
  openAIEndpoint: string;
  ollamaEndpoint: string;
  OPENAI_API_KEY?: string;
}

interface OllamaModel {
  name: string;
  // add other relevant fields
}
🧰 Tools
🪛 Biome (1.9.4)

[error] 37-37: void is confusing inside a union type.

Unsafe fix: Use undefined instead.

(lint/suspicious/noConfusingVoidType)


110-137: Enhance error handling in POST request processing.

While the request handling is good, consider adding try-catch blocks and more specific error responses.

Apply this improvement:

 private async handlePostRequest(c: any): Promise<Response> {
+  try {
     const json = await c.req.raw.clone().json();
     assert(json, is.ObjectOf({ model: is.String }));
     const endpoint = chooseEndpoint({
       model: json.model,
       ollamaEndpoint: this.ollamaEndpoint,
       openAIEndpoint: this.openAIEndpoint,
     });

     let res = await this.handleOpenAIModelVerify(c, json);
     if (res) {
       return res;
     }

     const origin = new URL(this.ollamaEndpoint).origin;
     const url = convertToCustomEndpoint(c.req.url, parseURL(endpoint));
     const reqHeaders = this.setCORSHeaders(c.req.raw, origin);
     reqHeaders.set('Origin', origin);
     const req = new Request(url, {
       ...c.req.raw,
       method: "POST",
       body: JSON.stringify(json),
       headers: reqHeaders,
       mode: 'no-cors'
     });
     return fetch(req);
+  } catch (error) {
+    console.error('Error processing POST request:', error);
+    return new Response(
+      JSON.stringify({ error: 'Failed to process request' }),
+      {
+        status: 500,
+        headers: { 'Content-Type': 'application/json' }
+      }
+    );
+  }
 }

65-72: Add type definitions for CORS configuration.

Consider adding type definitions for CORS configuration to make the code more maintainable.

Add these types:

interface CORSConfig {
  allowedOrigins: string | string[];
  allowedMethods: string[];
  allowedHeaders: string[];
  allowCredentials: boolean;
}

Then use them in the method:

-  private setCORSHeaders(res: Response, origin: string): Headers {
+  private setCORSHeaders(res: Response, origin: string, config?: Partial<CORSConfig>): Headers {
     const headers = new Headers(res.headers);
-    headers.set('Access-Control-Allow-Origin', origin || '*');
+    headers.set('Access-Control-Allow-Origin', origin || config?.allowedOrigins || '*');
     // ... rest of the method
   }

74-78: Translate Chinese comments to English for consistency.

The codebase appears to be primarily in English, but these comments are in Chinese. Consider translating them for better maintainability.

Replace with:

-  /**
-   * 处理验证请求的模型
-   * @param c 
-   * @param json 
-   * @returns 
-   */
+  /**
+   * Handles model verification requests
+   * @param c - The context object
+   * @param json - The request body
+   * @returns Response or null if verification succeeds
+   */

37-37: Replace void with undefined in union type.

Using void in a union type can be confusing. Consider using undefined instead for better type clarity.

-  private handleAllRequest(c: any, next: () => Promise<any>): Response | Promise<void | Response> {
+  private handleAllRequest(c: any, next: () => Promise<any>): Response | Promise<undefined | Response> {
🧰 Tools
🪛 Biome (1.9.4)

[error] 37-37: void is confusing inside a union type.

Unsafe fix: Use undefined instead.

(lint/suspicious/noConfusingVoidType)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between ec22a62 and dfd2eec.

📒 Files selected for processing (1)
  • proxy.ts (1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
proxy.ts

[error] 37-37: void is confusing inside a union type.

Unsafe fix: Use undefined instead.

(lint/suspicious/noConfusingVoidType)

🔇 Additional comments (2)
proxy.ts (2)

41-42: Security improvement: API key validation added.

Good addition of the API key validation check, addressing the previous security concern about unauthorized access.


194-211: Method name should be 'getModelsList' for consistency.

The method name should reflect that it's fetching models, not modules.

Copy link

@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

🧹 Outside diff range and nitpick comments (3)
proxy.ts (3)

37-37: Consider refining the return type signature

The return type Response | Promise<void | Response> could be simplified to Promise<Response | undefined> for better type clarity.

-  private handleAllRequest(c: any, next: () => Promise<any>): Response | Promise<void | Response> {
+  private handleAllRequest(c: any, next: () => Promise<any>): Promise<Response | undefined> {
🧰 Tools
🪛 Biome (1.9.4)

[error] 37-37: void is confusing inside a union type.

Unsafe fix: Use undefined instead.

(lint/suspicious/noConfusingVoidType)


96-100: Translate comments to English for consistency

The Chinese comment should be translated to English to maintain documentation consistency.

-   * 验证是否是Cursor的请求
+   * Verifies if this is a Cursor request

79-94: Enhance error handling with more specific messages

The error handling could be more informative by including additional context about why the model wasn't found.

-        console.error(`Ollama Model not found: ${json.model}`);
-        return new Response(JSON.stringify({ error: "Model not found" }), {
+        const errorMessage = `Model '${json.model}' not found in available models: ${models.map((m: any) => m.name).join(', ')}`;
+        console.error(errorMessage);
+        return new Response(JSON.stringify({ error: errorMessage }), {
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between dfd2eec and a0b7d0a.

📒 Files selected for processing (1)
  • proxy.ts (1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
proxy.ts

[error] 37-37: void is confusing inside a union type.

Unsafe fix: Use undefined instead.

(lint/suspicious/noConfusingVoidType)

🔇 Additional comments (2)
proxy.ts (2)

Line range hint 1-29: LGTM! Well-structured class initialization

The class structure is well-documented with proper JSDoc comments and the constructor correctly initializes all required fields.

🧰 Tools
🪛 Biome (1.9.4)

[error] 37-37: void is confusing inside a union type.

Unsafe fix: Use undefined instead.

(lint/suspicious/noConfusingVoidType)


41-42: LGTM! Proper API key validation

Good addition of API key validation before proceeding with authentication.

proxy.ts Outdated
Comment on lines 110 to 137
private async handlePostRequest(c: any): Promise<Response> {
const json = await c.req.raw.clone().json();
const { model } = json;

// Validate the request payload
assert(json, is.ObjectOf({ model: is.String }));

const endpoint = chooseEndpoint({
model,
ollamaEndpoint,
openAIEndpoint,
model: json.model,
ollamaEndpoint: this.ollamaEndpoint,
openAIEndpoint: this.openAIEndpoint,
});

let res = await this.handleOpenAIModelVerify(c, json);
if (res) {
return res;
}

const origin = new URL(this.ollamaEndpoint).origin;
const url = convertToCustomEndpoint(c.req.url, parseURL(endpoint));
const req = new Request(url, c.req.raw);
req.headers.set("Host", ollamaEndpoint);
return fetch(req);
});
const reqHeaders = this.setCORSHeaders(c.req.raw, origin);
// 设置Origin头, 否则ollama会报错
reqHeaders.set('Origin', origin);
const req = new Request(url, {
...c.req.raw,
method: "POST",
body: JSON.stringify(json),
headers: reqHeaders,
mode: 'no-cors'
});
return fetch(req)
}
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve error handling and documentation

Several improvements could be made to this method:

  1. Add proper error handling for the JSON parsing
  2. Translate the Chinese comment to English
  3. Consider adding request timeout handling
 private async handlePostRequest(c: any): Promise<Response> {
-    const json = await c.req.raw.clone().json();
-    assert(json, is.ObjectOf({ model: is.String }));
+    try {
+      const json = await c.req.raw.clone().json();
+      assert(json, is.ObjectOf({ model: is.String }));
+    } catch (error) {
+      return new Response(
+        JSON.stringify({ error: 'Invalid request format' }),
+        { status: 400, headers: { 'Content-Type': 'application/json' } }
+      );
+    }

-    // 设置Origin头, 否则ollama会报错
+    // Set Origin header to prevent Ollama errors

     const req = new Request(url, {
       ...c.req.raw,
       method: "POST",
       body: JSON.stringify(json),
       headers: reqHeaders,
-      mode: 'no-cors'
+      mode: 'no-cors',
+      signal: AbortSignal.timeout(30000) // 30 second timeout
     });
-    return fetch(req)
+    try {
+      return await fetch(req);
+    } catch (error) {
+      return new Response(
+        JSON.stringify({ error: 'Failed to proxy request' }),
+        { status: 502, headers: { 'Content-Type': 'application/json' } }
+      );
+    }

Committable suggestion skipped: line range outside the PR's diff.

@ryoppippi
Copy link
Owner

Thank you for your hard work.
I'll modify the styling or comments, then accept it

@ryoppippi
Copy link
Owner

OK, I got what you want.
But there are lots of type errors.
I'll fix it

@ryoppippi
Copy link
Owner

ryoppippi commented Dec 11, 2024

@jincdream
So, if you have time, folllowing the tasks

@ryoppippi
Copy link
Owner

@jincdream
I'll work it on this weekend.
If you can work by the weekend, that is fine!
If not, I'll merge it and modify by myself

@ryoppippi
Copy link
Owner

@ryoppippi
Copy link
Owner

@jincdream I'm sorry, I force pushed.....
I'll create another PR!

@ryoppippi
Copy link
Owner

I accidentally blew up the entire contents of your MAIN branch.
I'm so sorry.
Your commits are still in this repo, so you can restore them at any time.
I'm so sorry.

@jincdream

https://github.com/ryoppippi/curxy/tree/pr-28

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.

TypeError: Failed to fetch
2 participants