-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat: support custom OPENAI_BASE_URL for self-hosted OpenAI-compatible LLM endpoints (#1996) #2090
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: main
Are you sure you want to change the base?
Changes from all commits
07a82ac
7e94c28
59b69c5
e186c9f
d745ba9
b19720b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,65 @@ | ||||||
| import { readFileSync, readdirSync, statSync } from "node:fs"; | ||||||
| import { join } from "node:path"; | ||||||
| import { describe, expect, it } from "vitest"; | ||||||
| import { RATE_LIMIT_IDS } from "../../lib/rate-limit"; | ||||||
|
|
||||||
| // Rate limit IDs declared in advance for firewall rules or separate app packages | ||||||
| // that are intentionally not yet wired in apps/web endpoints. | ||||||
|
Comment on lines
+6
to
+7
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
These comments only narrate the self-descriptive
Suggested change
Context Used: AGENTS.md (source) Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/web/__tests__/unit/rate-limit-ids.test.ts
Line: 6-7
Comment:
**Comments duplicate constant purpose**
These comments only narrate the self-descriptive `UNWIRED_RATE_LIMIT_IDS` constant rather than preserving a non-obvious invariant or workaround, adding redundant text that must be maintained.
```suggestion
```
**Context Used:** AGENTS.md ([source](https://github.com/capsoftware/cap/blob/main/AGENTS.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
||||||
| const UNWIRED_RATE_LIMIT_IDS = new Set([ | ||||||
| "AUTH_OTP_VERIFY", | ||||||
| "AUTH_OTP_SEND", | ||||||
| "LOOM_DOWNLOAD", | ||||||
| "MESSENGER_MESSAGE", | ||||||
| "DESKTOP_LOGS", | ||||||
| ]); | ||||||
|
|
||||||
| function getAllTsFiles(dir: string): string[] { | ||||||
| let results: string[] = []; | ||||||
| const list = readdirSync(dir); | ||||||
| for (const file of list) { | ||||||
| const filePath = join(dir, file); | ||||||
| const stat = statSync(filePath); | ||||||
| if (stat && stat.isDirectory()) { | ||||||
| if (file !== "node_modules" && file !== ".next" && file !== "dist") { | ||||||
| results = results.concat(getAllTsFiles(filePath)); | ||||||
| } | ||||||
| } else if (file.endsWith(".ts") || file.endsWith(".tsx")) { | ||||||
| if (!filePath.endsWith("lib/rate-limit.ts") && !filePath.endsWith("rate-limit-ids.test.ts")) { | ||||||
| results.push(filePath); | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
| return results; | ||||||
| } | ||||||
|
|
||||||
| describe("RATE_LIMIT_IDS reference contract", () => { | ||||||
| it("ensures every active declared RATE_LIMIT_ID is referenced outside lib/rate-limit.ts", () => { | ||||||
| const webAppDir = join(process.cwd()); | ||||||
| const tsFiles = getAllTsFiles(webAppDir); | ||||||
|
|
||||||
| let combinedSource = ""; | ||||||
| for (const file of tsFiles) { | ||||||
| combinedSource += readFileSync(file, "utf8") + "\n"; | ||||||
| } | ||||||
|
|
||||||
| const unreferencedKeys: string[] = []; | ||||||
|
|
||||||
| for (const [key, value] of Object.entries(RATE_LIMIT_IDS)) { | ||||||
| if (UNWIRED_RATE_LIMIT_IDS.has(key)) { | ||||||
| continue; | ||||||
| } | ||||||
|
|
||||||
| const hasKeyRef = combinedSource.includes(`RATE_LIMIT_IDS.${key}`); | ||||||
| const hasValueRef = combinedSource.includes(`"${value}"`) || combinedSource.includes(`'${value}'`); | ||||||
|
|
||||||
| if (!hasKeyRef && !hasValueRef) { | ||||||
| unreferencedKeys.push(key); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| expect( | ||||||
| unreferencedKeys, | ||||||
| `The following RATE_LIMIT_IDS are declared but never referenced: ${unreferencedKeys.join(", ")}`, | ||||||
| ).toEqual([]); | ||||||
| }); | ||||||
| }); | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,3 +39,36 @@ export async function PUT(request: NextRequest) { | |
|
|
||
| return Response.json(true, { status: 200 }); | ||
| } | ||
|
|
||
| export async function GET(request: NextRequest) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This new operation uses an ad-hoc Next.js handler and direct database query instead of the required Context Used: AGENTS.md (source) Knowledge Base Used: Web App (apps/web) Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/web/app/api/video/metadata/route.ts
Line: 43
Comment:
**Metadata route bypasses API architecture**
This new operation uses an ad-hoc Next.js handler and direct database query instead of the required `HttpApi` builder and backend service pattern, bypassing shared policy, typed errors, and request-context wiring.
**Context Used:** AGENTS.md ([source](https://github.com/capsoftware/cap/blob/main/AGENTS.md))
**Knowledge Base Used:** [Web App (apps/web)](https://app.greptile.com/cap/-/custom-context/knowledge-base/capsoftware/cap/-/docs/web-app.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
||
| const { searchParams } = new URL(request.url); | ||
| const videoId = searchParams.get("videoId"); | ||
|
|
||
| if (!videoId) { | ||
| return Response.json({ error: "Missing videoId parameter" }, { status: 400 }); | ||
| } | ||
|
|
||
| const query = await db().select().from(videos).where(eq(videos.id, videoId)); | ||
|
|
||
| if (query.length === 0 || !query[0]) { | ||
| return Response.json({ error: "Video not found" }, { status: 404 }); | ||
| } | ||
|
|
||
| const video = query[0]; | ||
| const user = await getCurrentUser(); | ||
|
|
||
| if (!video.public && video.ownerId !== user?.id) { | ||
| return Response.json({ error: "Unauthorized" }, { status: 401 }); | ||
| } | ||
|
Comment on lines
+60
to
+62
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a public video is password-protected, this check authorizes unauthenticated and non-owner requests solely because How this was verified: The video schema permits public videos with passwords, while the established public-view policy verifies password candidates before granting access. Knowledge Base Used: Web App (apps/web) Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/web/app/api/video/metadata/route.ts
Line: 60-62
Comment:
**Password policy bypassed for metadata**
When a public video is password-protected, this check authorizes unauthenticated and non-owner requests solely because `video.public` is true, exposing its title, AI summary, chapters, and generation status without the required password.
**How this was verified:** The video schema permits public videos with passwords, while the established public-view policy verifies password candidates before granting access.
**Knowledge Base Used:** [Web App (apps/web)](https://app.greptile.com/cap/-/custom-context/knowledge-base/capsoftware/cap/-/docs/web-app.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly. |
||
|
|
||
| const meta = (video.metadata as Record<string, any>) ?? {}; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The new Context Used: AGENTS.md (source) Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/web/app/api/video/metadata/route.ts
Line: 64
Comment:
**Explicit any breaks lint contract**
The new `Record<string, any>` cast violates the repository's enabled `noExplicitAny` rule, causing the changed route to fail the required Biome check; use a defined metadata type or `unknown` with narrowing.
**Context Used:** AGENTS.md ([source](https://github.com/capsoftware/cap/blob/main/AGENTS.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly. |
||
|
|
||
| return Response.json({ | ||
| videoId: video.id, | ||
| title: video.title || meta.aiTitle || null, | ||
| aiTitle: meta.aiTitle || null, | ||
| summary: meta.summary || null, | ||
| chapters: meta.chapters || [], | ||
| aiGenerationStatus: meta.aiGenerationStatus || "SKIPPED", | ||
| }); | ||
| } | ||
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.
When a valid share URL contains a query parameter or trailing slash,
rsplit('/')includes the query suffix or returns an empty segment, causingcap recordings infoto send the wrongvideoIdand receive a 400 or 404 instead of metadata.Knowledge Base Used: Cap CLI (
apps/cli)Prompt To Fix With AI