Make review queue jobs resumable and lease-aware#11
Conversation
![codra-app-personal[bot]](https://avatars.githubusercontent.com/in/3347243?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codra Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35d06d55fd
ℹ️ About Codra in GitHub
Your team has set up Codra to review pull requests in this repo. Reviews are triggered when you:
- Open a pull request for review
- Mark a draft as ready
- Comment "@codra-app review"
If Codra has suggestions, it will comment; otherwise it will react with 👍.
Codra can also answer questions or update the PR. Try commenting "@codra-app address that feedback".
There was a problem hiding this comment.
Codra Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35d06d55fd
ℹ️ About Codra in GitHub
Your team has set up Codra to review pull requests in this repo. Reviews are triggered when you:
- Open a pull request for review
- Mark a draft as ready
- Comment "@codra-app review"
If Codra has suggestions, it will comment; otherwise it will react with 👍.
Codra can also answer questions or update the PR. Try commenting "@codra-app address that feedback".
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The runOpportunisticJobMaintenance call is not wrapped in a try-catch block. If this function throws an error, it will reject the promise returned by runWithDb, potentially interrupting the current batch processing or causing the worker to fail.
| Wrap the maintenance call in a try-catch block to ensure that maintenance failures do not break the processing of the current batch. |
| message.ack(); | ||
| continue; | ||
| } | ||
| if (!parseResult.success) { |
There was a problem hiding this comment.
The logic for handling invalid queue messages was changed from message.ack() to message.retry(). While this allows the message to reach a DLQ, it consumes retry credits and CPU time. If the schema is fundamentally invalid, acknowledging the message is the standard pattern to stop the retry loop.
| if (!parseResult.success) { | |
| Revert to `message.ack()` for invalid schemas to prevent unnecessary retries and resource consumption. |
| const text = content | ||
| .map((part) => { | ||
| if (isText(part)) return part; | ||
| if (part && typeof part === 'object' && isText((part as any).text)) return (part as any).text; |
There was a problem hiding this comment.
The function uses '(part as any).text' to access the text property of a message part. This bypasses TypeScript's type checking and can lead to runtime errors if the object structure changes or if 'part' is not shaped as expected.
| if (part && typeof part === 'object' && isText((part as any).text)) return (part as any).text; | |
| if (part && typeof part === 'object' && 'text' in part && isText(part.text)) return part.text; |
| return null; | ||
| } | ||
|
|
||
| function extractCloudflareText(result: any, model: string): string { |
There was a problem hiding this comment.
The 'extractCloudflareText' function defines its first parameter 'result' as 'any'. Since Cloudflare Workers AI has a predictable response schema, this should be replaced with a proper interface or a record type to ensure type safety when accessing nested properties like 'choices' or 'result.response'.
| function extractCloudflareText(result: any, model: string): string { | |
| function extractCloudflareText(result: unknown, model: string): string { |
| const startTime = Date.now(); | ||
| const url = `https://generativelanguage.googleapis.com/v1beta/models/${model}:generateContent?key=${env.GEMINI_API_KEY}`; | ||
| const maxRetries = 2; | ||
| const maxRetries = GOOGLE_MAX_RETRIES; |
There was a problem hiding this comment.
The variable lastError is explicitly typed as any on line 20. This bypasses TypeScript's type checking and can lead to runtime errors when accessing properties of lastError later in the code. Using unknown is the modern TypeScript standard for variables that hold potential error objects, as it requires explicit type narrowing before use.
| const maxRetries = GOOGLE_MAX_RETRIES; | |
| let lastError: unknown; |
| const app = new Hono<AppEnv>(); | ||
|
|
||
| app.get('/', async (c) => { | ||
| await runOpportunisticJobMaintenance(c.env); |
There was a problem hiding this comment.
The calls to runOpportunisticJobMaintenance(c.env) are awaited directly within the request handlers. If the maintenance logic fails (e.g., database timeout, lock contention, or internal error), it will throw an exception that results in a 500 Internal Server Error for the user. Since this maintenance is 'opportunistic' and not critical to the immediate request's success, it should be wrapped in a try-catch block to ensure that the API remains available even if maintenance fails.
| await runOpportunisticJobMaintenance(c.env); | |
| try { | |
| await runOpportunisticJobMaintenance(c.env); | |
| } catch (e) { | |
| console.error('Opportunistic job maintenance failed:', e); | |
| } |
| @@ -110,6 +149,7 @@ export class ModelService { | |||
| const modelsToTry = [primary, ...fallbacks]; | |||
There was a problem hiding this comment.
any type for error variable
The variable lastError is declared as any (line 151). This bypasses TypeScript's type checking, potentially allowing runtime errors if the error object does not have the expected properties.
| const modelsToTry = [primary, ...fallbacks]; | |
| let lastError: unknown; |
There was a problem hiding this comment.
Codra Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35d06d55fd
ℹ️ About Codra in GitHub
Your team has set up Codra to review pull requests in this repo. Reviews are triggered when you:
- Open a pull request for review
- Mark a draft as ready
- Comment "@codra-app review"
If Codra has suggestions, it will comment; otherwise it will react with 👍.
Codra can also answer questions or update the PR. Try commenting "@codra-app address that feedback".
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The runOpportunisticJobMaintenance call is not wrapped in a try-catch block. If this function throws an error, it will reject the promise returned by runWithDb, potentially interrupting the current batch processing or causing the worker to fail.
| Wrap the maintenance call in a try-catch block to ensure that maintenance failures do not break the processing of the current batch. |
| message.ack(); | ||
| continue; | ||
| } | ||
| if (!parseResult.success) { |
There was a problem hiding this comment.
The logic for handling invalid queue messages was changed from message.ack() to message.retry(). While this allows the message to reach a DLQ, it consumes retry credits and CPU time. If the schema is fundamentally invalid, acknowledging the message is the standard pattern to stop the retry loop.
| if (!parseResult.success) { | |
| Revert to `message.ack()` for invalid schemas to prevent unnecessary retries and resource consumption. |
| const text = content | ||
| .map((part) => { | ||
| if (isText(part)) return part; | ||
| if (part && typeof part === 'object' && isText((part as any).text)) return (part as any).text; |
There was a problem hiding this comment.
The function uses '(part as any).text' to access the text property of a message part. This bypasses TypeScript's type checking and can lead to runtime errors if the object structure changes or if 'part' is not shaped as expected.
| if (part && typeof part === 'object' && isText((part as any).text)) return (part as any).text; | |
| if (part && typeof part === 'object' && 'text' in part && isText(part.text)) return part.text; |
| return null; | ||
| } | ||
|
|
||
| function extractCloudflareText(result: any, model: string): string { |
There was a problem hiding this comment.
The 'extractCloudflareText' function defines its first parameter 'result' as 'any'. Since Cloudflare Workers AI has a predictable response schema, this should be replaced with a proper interface or a record type to ensure type safety when accessing nested properties like 'choices' or 'result.response'.
| function extractCloudflareText(result: any, model: string): string { | |
| function extractCloudflareText(result: unknown, model: string): string { |
| const startTime = Date.now(); | ||
| const url = `https://generativelanguage.googleapis.com/v1beta/models/${model}:generateContent?key=${env.GEMINI_API_KEY}`; | ||
| const maxRetries = 2; | ||
| const maxRetries = GOOGLE_MAX_RETRIES; |
There was a problem hiding this comment.
The variable lastError is explicitly typed as any on line 20. This bypasses TypeScript's type checking and can lead to runtime errors when accessing properties of lastError later in the code. Using unknown is the modern TypeScript standard for variables that hold potential error objects, as it requires explicit type narrowing before use.
| const maxRetries = GOOGLE_MAX_RETRIES; | |
| let lastError: unknown; |
| const app = new Hono<AppEnv>(); | ||
|
|
||
| app.get('/', async (c) => { | ||
| await runOpportunisticJobMaintenance(c.env); |
There was a problem hiding this comment.
The calls to runOpportunisticJobMaintenance(c.env) are awaited directly within the request handlers. If the maintenance logic fails (e.g., database timeout, lock contention, or internal error), it will throw an exception that results in a 500 Internal Server Error for the user. Since this maintenance is 'opportunistic' and not critical to the immediate request's success, it should be wrapped in a try-catch block to ensure that the API remains available even if maintenance fails.
| await runOpportunisticJobMaintenance(c.env); | |
| try { | |
| await runOpportunisticJobMaintenance(c.env); | |
| } catch (e) { | |
| console.error('Opportunistic job maintenance failed:', e); | |
| } |
| @@ -110,6 +149,7 @@ export class ModelService { | |||
| const modelsToTry = [primary, ...fallbacks]; | |||
There was a problem hiding this comment.
any type for error variable
The variable lastError is declared as any (line 151). This bypasses TypeScript's type checking, potentially allowing runtime errors if the error object does not have the expected properties.
| const modelsToTry = [primary, ...fallbacks]; | |
| let lastError: unknown; |
There was a problem hiding this comment.
Codra Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35d06d55fd
ℹ️ About Codra in GitHub
Your team has set up Codra to review pull requests in this repo. Reviews are triggered when you:
- Open a pull request for review
- Mark a draft as ready
- Comment "@codra-app review"
If Codra has suggestions, it will comment; otherwise it will react with 👍.
Codra can also answer questions or update the PR. Try commenting "@codra-app address that feedback".
| export async function upsertFileReview( | ||
| env: Pick<AppBindings, 'HYPERDRIVE'>, | ||
| jobId: string, | ||
| input: { |
There was a problem hiding this comment.
While the function signature defines the shape of input, the variable input inside the function is inferred. In strict TypeScript mode, relying on object literal inference can sometimes lead to issues if the literal doesn't match the interface exactly.
| input: { | |
| Explicitly type the parameter or the variable, e.g., `input: UpsertFileReviewInput` if a dedicated interface is created, or ensure the literal passed in matches the type definition strictly. |
| } catch (err) { | ||
| // Non-fatal: log and continue processing the batch. | ||
| logger.error('Failed to recover stale jobs', err instanceof Error ? err : new Error(String(err))); | ||
| } |
There was a problem hiding this comment.
The runOpportunisticJobMaintenance call was moved from the start of the queue function to the end. This is a critical logic error. If a job is stuck in 'running' state due to a previous crash, it will not be recovered until the next batch is processed. If the queue is empty, stale jobs will remain stuck indefinitely, violating the 'resumable' goal.
| } | |
| Move `await runOpportunisticJobMaintenance(env);` back to the beginning of the `queue` function, before processing messages, to ensure stale jobs are recovered immediately. |
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The runOpportunisticJobMaintenance call is not wrapped in a try-catch block. If this function throws an error, it will reject the promise returned by runWithDb, potentially interrupting the current batch processing or causing the worker to fail.
| Wrap the maintenance call in a try-catch block to ensure that maintenance failures do not break the processing of the current batch. |
| message.ack(); | ||
| continue; | ||
| } | ||
| if (!parseResult.success) { |
There was a problem hiding this comment.
The logic for handling invalid queue messages was changed from message.ack() to message.retry(). While this allows the message to reach a DLQ, it consumes retry credits and CPU time. If the schema is fundamentally invalid, acknowledging the message is the standard pattern to stop the retry loop.
| if (!parseResult.success) { | |
| Revert to `message.ack()` for invalid schemas to prevent unnecessary retries and resource consumption. |
| const text = content | ||
| .map((part) => { | ||
| if (isText(part)) return part; | ||
| if (part && typeof part === 'object' && isText((part as any).text)) return (part as any).text; |
There was a problem hiding this comment.
The function uses '(part as any).text' to access the text property of a message part. This bypasses TypeScript's type checking and can lead to runtime errors if the object structure changes or if 'part' is not shaped as expected.
| if (part && typeof part === 'object' && isText((part as any).text)) return (part as any).text; | |
| if (part && typeof part === 'object' && 'text' in part && isText(part.text)) return part.text; |
| super(message); | ||
| this.name = 'RetryableModelError'; | ||
| if (cause !== undefined) { | ||
| (this as any).cause = cause; |
There was a problem hiding this comment.
RetryableModelError constructor
The code uses (this as any) to assign the cause property (line 26). This is unnecessary and unsafe. The Error class supports the cause property natively in modern JavaScript/TypeScript.
| (this as any).cause = cause; | |
| Object.defineProperty(this, 'cause', { value: cause, writable: true, configurable: true }); |
| } | ||
|
|
||
| export function isRetryableModelError(error: unknown) { | ||
| return Boolean(error && typeof error === 'object' && (error as any).retryable === true); |
There was a problem hiding this comment.
isRetryableModelError
The function uses (error as any) to check for the retryable property (line 32). This defeats the purpose of using a type-safe language. It should use instanceof or a type guard.
| return Boolean(error && typeof error === 'object' && (error as any).retryable === true); | |
| return error instanceof RetryableModelError; |
| async send(message: any) { | ||
| this.sent.push(message); | ||
| async send(message: any, options?: { delaySeconds?: number }) { | ||
| this.sent.push({ ...message, options }); |
There was a problem hiding this comment.
The code uses the spread operator { ...message, options } to store the sent message. If message is a primitive (e.g., a string or number), the spread operator will not behave as expected (e.g., a string will be spread into indexed characters). Furthermore, if message contains a property named 'options', it will be overwritten by the options argument. It is safer to store the message and options as distinct properties in a wrapper object.
| this.sent.push({ ...message, options }); | |
| this.sent.push({ message, options }); |
| @@ -49,8 +49,8 @@ export class MockAssets { | |||
| export class MockQueue { | |||
| public readonly sent: any[] = []; | |||
There was a problem hiding this comment.
The sent array and the message parameter are typed as any. This bypasses TypeScript's type checking and can lead to runtime errors in tests. Since this is a Mock class, it should ideally use generics to allow the caller to specify the expected message type.
| public readonly sent: any[] = []; | |
| export class MockQueue<T = any> { | |
| public readonly sent: Array<{ message: T; options?: { delaySeconds?: number } }> = []; | |
| async send(message: T, options?: { delaySeconds?: number }) { | |
| this.sent.push({ message, options }); | |
| } | |
| } |
| ], | ||
| usage: { prompt_tokens: 1, completion_tokens: 4096 }, | ||
| }; | ||
| }, |
There was a problem hiding this comment.
The use of 'as any' to mock the Cloudflare AI binding (lines 45, 64, 82) bypasses TypeScript's type checking. While common in tests, it's better to use 'Partial' or a specific interface for the binding to ensure the mock remains compatible with the actual API as it evolves.
| }, | |
| // Example: Cast to a partial of the expected AI binding type | |
| AI: { | |
| async run() { | |
| // ... | |
| }, | |
| } as Partial<CloudflareAIBinding> |
There was a problem hiding this comment.
Codra Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f89d03ec01
ℹ️ About Codra in GitHub
Your team has set up Codra to review pull requests in this repo. Reviews are triggered when you:
- Open a pull request for review
- Mark a draft as ready
- Comment "@codra-app review"
If Codra has suggestions, it will comment; otherwise it will react with 👍.
Codra can also answer questions or update the PR. Try commenting "@codra-app address that feedback".
| }; | ||
|
|
||
| function normalizeGlobalConfig(config: any): ModelRouteConfig { | ||
| export function normalizeGlobalConfig(config: any): ModelRouteConfig { |
There was a problem hiding this comment.
The function 'normalizeGlobalConfig' uses 'any' for the 'config' parameter. This bypasses TypeScript's type checking and can lead to runtime errors if the input structure changes. It is better to use 'Partial' or 'unknown' with a type guard to ensure type safety.
| export function normalizeGlobalConfig(config: any): ModelRouteConfig { | |
| export function normalizeGlobalConfig(config: Partial<ModelRouteConfig>): ModelRouteConfig { |
| const inherited = parentReviews.get(file.path); | ||
| if (inherited) { | ||
| if (!canInheritParentFileReview(config, inherited)) { | ||
| logger.info(`Ignoring inherited review for ${file.path}; parent model ${inherited.model_used} is not in the current model strategy`); |
There was a problem hiding this comment.
The code uses 'true as any' when setting the value in 'currentReviews.set(file.path, true as any)' and 'currentReviews.set(file.path, inherited)'. This bypasses TypeScript's type checking and can lead to runtime errors if the Map is expected to contain specific object structures. It's better to use a proper sentinel value or ensure the Map type is correctly defined for the purpose.
| logger.info(`Ignoring inherited review for ${file.path}; parent model ${inherited.model_used} is not in the current model strategy`); | |
| Instead of using 'true as any', define a type for the Map values that accommodates both the review object and a marker for 'processed', or use a Set for tracking processed paths separately. |
| } | ||
|
|
||
| let eventName = message.eventName; | ||
| let payload = message.payload as GitHubWebhookPayload | undefined; |
There was a problem hiding this comment.
The job lease is claimed at the beginning of 'runReviewJob' using 'claimJobLease'. However, if the process crashes or fails before 'releaseJobLease' or 'heartbeatJobLease' is called, the lease might remain active until it expires naturally. While the implementation uses a lease-aware approach, the error handling logic for 'isRetryableModelError' calls 'releaseJobLease' but other non-retryable errors might skip it if not carefully structured in every possible execution path (though current try-catch seems to cover it).
| let payload = message.payload as GitHubWebhookPayload | undefined; | |
| Ensure all exit paths in the catch block explicitly handle lease release, or use a 'try...finally' block to guarantee 'releaseJobLease' is called when the job is finished (either successfully, via supersession, or via terminal failure). |
| return { action: 'ack' }; | ||
| } catch (error) { | ||
| const messageText = error instanceof Error ? error.message : 'Unknown review failure'; | ||
| if (messageText === 'JOB_SUPERSEDED') { |
There was a problem hiding this comment.
When a job is superseded ('JOB_SUPERSEDED' error), the code calls 'releaseJobLease' and returns 'ack'. However, it does not explicitly mark the job as 'superseded' in the database if it hasn't been already (the supersession is typically handled by the function that creates the new job, but the current job's local state handling could be more robust to ensure no orphaned 'running' statuses persist in sub-steps).
| if (messageText === 'JOB_SUPERSEDED') { | |
| Verify that 'releaseJobLease' or the superseding logic correctly transitions the status of the superseded job to a terminal state to prevent any potential state mismatch. |
| return { | ||
| async query<T>(sqlText: string, params: unknown[] = []) { | ||
| return (await sql.unsafe(sqlText, params.map(normalizeParam) as any[], { prepare: true })) as T[]; | ||
| return (await sql.unsafe(sqlText, params.map(normalizeParam) as any[], { prepare: false })) as T[]; |
There was a problem hiding this comment.
The use of 'as any[]' when mapping parameters bypasses TypeScript's type checking. Since 'params' is already 'unknown[]', and 'normalizeParam' likely returns a type compatible with the database driver, casting to 'unknown[]' or omitting the cast (if the function return type allows) would be safer and more aligned with TypeScript best practices.
| return (await sql.unsafe(sqlText, params.map(normalizeParam) as any[], { prepare: false })) as T[]; | |
| return (await sql.unsafe(sqlText, params.map(normalizeParam), { prepare: false })) as T[]; |
| const text = content | ||
| .map((part) => { | ||
| if (isText(part)) return part; | ||
| if (part && typeof part === 'object' && isText((part as any).text)) return (part as any).text; |
There was a problem hiding this comment.
The functions extractCloudflareText and extractMessageContent use the any type for the result and part parameters. This bypasses TypeScript's type checking and increases the risk of runtime errors if the API response structure changes unexpectedly. Since Cloudflare's AI responses follow specific schemas (e.g., Chat Completion API), these should be defined as interfaces or a union of possible response shapes.
| if (part && typeof part === 'object' && isText((part as any).text)) return (part as any).text; | |
| interface CloudflareResponse { | |
| response?: string; | |
| result?: { response?: string }; | |
| choices?: Array<{ | |
| message?: { content?: string | Array<{ text?: string } | string>; reasoning?: string; reasoning_content?: string }; | |
| finish_reason?: string; | |
| stop_reason?: string; | |
| }>; | |
| } | |
| function extractCloudflareText(result: CloudflareResponse | string, model: string): string { ... } |
| const CLOUDFLARE_MAX_RETRIES = 1; | ||
|
|
||
| function isText(value: unknown): value is string { | ||
| return typeof value === 'string' && value.trim().length > 0; |
There was a problem hiding this comment.
The isText helper returns false if a string is empty or only whitespace (value.trim().length > 0). In extractMessageContent, if the AI returns an array of empty strings, the .join('').trim() result will be an empty string, causing the function to return null (line 26). This subsequently triggers a generic 'empty response' error in extractCloudflareText. While likely desired, it treats 'empty content' as a failure rather than a valid (albeit empty) response.
| const startTime = Date.now(); | ||
| const url = `https://generativelanguage.googleapis.com/v1beta/models/${model}:generateContent?key=${env.GEMINI_API_KEY}`; | ||
| const maxRetries = 2; | ||
| const maxRetries = GOOGLE_MAX_RETRIES; |
There was a problem hiding this comment.
The variable 'lastError' is explicitly typed as 'any'. This bypasses TypeScript's type checking and can lead to runtime errors if the error object does not have the expected properties. It is recommended to use 'unknown' or 'Error | unknown' and employ type narrowing (e.g., 'if (lastError instanceof Error)') when accessing properties.
| const maxRetries = GOOGLE_MAX_RETRIES; | |
| let lastError: unknown; |
There was a problem hiding this comment.
Codra Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f89d03ec01
ℹ️ About Codra in GitHub
Your team has set up Codra to review pull requests in this repo. Reviews are triggered when you:
- Open a pull request for review
- Mark a draft as ready
- Comment "@codra-app review"
If Codra has suggestions, it will comment; otherwise it will react with 👍.
Codra can also answer questions or update the PR. Try commenting "@codra-app address that feedback".
| }; | ||
|
|
||
| function normalizeGlobalConfig(config: any): ModelRouteConfig { | ||
| export function normalizeGlobalConfig(config: any): ModelRouteConfig { |
There was a problem hiding this comment.
The function 'normalizeGlobalConfig' uses 'any' for the 'config' parameter. This bypasses TypeScript's type checking and can lead to runtime errors if the input structure changes. It is better to use 'Partial' or 'unknown' with a type guard to ensure type safety.
| export function normalizeGlobalConfig(config: any): ModelRouteConfig { | |
| export function normalizeGlobalConfig(config: Partial<ModelRouteConfig>): ModelRouteConfig { |
| const inherited = parentReviews.get(file.path); | ||
| if (inherited) { | ||
| if (!canInheritParentFileReview(config, inherited)) { | ||
| logger.info(`Ignoring inherited review for ${file.path}; parent model ${inherited.model_used} is not in the current model strategy`); |
There was a problem hiding this comment.
The code uses 'true as any' when setting the value in 'currentReviews.set(file.path, true as any)' and 'currentReviews.set(file.path, inherited)'. This bypasses TypeScript's type checking and can lead to runtime errors if the Map is expected to contain specific object structures. It's better to use a proper sentinel value or ensure the Map type is correctly defined for the purpose.
| logger.info(`Ignoring inherited review for ${file.path}; parent model ${inherited.model_used} is not in the current model strategy`); | |
| Instead of using 'true as any', define a type for the Map values that accommodates both the review object and a marker for 'processed', or use a Set for tracking processed paths separately. |
| } | ||
|
|
||
| let eventName = message.eventName; | ||
| let payload = message.payload as GitHubWebhookPayload | undefined; |
There was a problem hiding this comment.
The job lease is claimed at the beginning of 'runReviewJob' using 'claimJobLease'. However, if the process crashes or fails before 'releaseJobLease' or 'heartbeatJobLease' is called, the lease might remain active until it expires naturally. While the implementation uses a lease-aware approach, the error handling logic for 'isRetryableModelError' calls 'releaseJobLease' but other non-retryable errors might skip it if not carefully structured in every possible execution path (though current try-catch seems to cover it).
| let payload = message.payload as GitHubWebhookPayload | undefined; | |
| Ensure all exit paths in the catch block explicitly handle lease release, or use a 'try...finally' block to guarantee 'releaseJobLease' is called when the job is finished (either successfully, via supersession, or via terminal failure). |
| return { action: 'ack' }; | ||
| } catch (error) { | ||
| const messageText = error instanceof Error ? error.message : 'Unknown review failure'; | ||
| if (messageText === 'JOB_SUPERSEDED') { |
There was a problem hiding this comment.
When a job is superseded ('JOB_SUPERSEDED' error), the code calls 'releaseJobLease' and returns 'ack'. However, it does not explicitly mark the job as 'superseded' in the database if it hasn't been already (the supersession is typically handled by the function that creates the new job, but the current job's local state handling could be more robust to ensure no orphaned 'running' statuses persist in sub-steps).
| if (messageText === 'JOB_SUPERSEDED') { | |
| Verify that 'releaseJobLease' or the superseding logic correctly transitions the status of the superseded job to a terminal state to prevent any potential state mismatch. |
| return { | ||
| async query<T>(sqlText: string, params: unknown[] = []) { | ||
| return (await sql.unsafe(sqlText, params.map(normalizeParam) as any[], { prepare: true })) as T[]; | ||
| return (await sql.unsafe(sqlText, params.map(normalizeParam) as any[], { prepare: false })) as T[]; |
There was a problem hiding this comment.
The use of 'as any[]' when mapping parameters bypasses TypeScript's type checking. Since 'params' is already 'unknown[]', and 'normalizeParam' likely returns a type compatible with the database driver, casting to 'unknown[]' or omitting the cast (if the function return type allows) would be safer and more aligned with TypeScript best practices.
| return (await sql.unsafe(sqlText, params.map(normalizeParam) as any[], { prepare: false })) as T[]; | |
| return (await sql.unsafe(sqlText, params.map(normalizeParam), { prepare: false })) as T[]; |
| const text = content | ||
| .map((part) => { | ||
| if (isText(part)) return part; | ||
| if (part && typeof part === 'object' && isText((part as any).text)) return (part as any).text; |
There was a problem hiding this comment.
The functions extractCloudflareText and extractMessageContent use the any type for the result and part parameters. This bypasses TypeScript's type checking and increases the risk of runtime errors if the API response structure changes unexpectedly. Since Cloudflare's AI responses follow specific schemas (e.g., Chat Completion API), these should be defined as interfaces or a union of possible response shapes.
| if (part && typeof part === 'object' && isText((part as any).text)) return (part as any).text; | |
| interface CloudflareResponse { | |
| response?: string; | |
| result?: { response?: string }; | |
| choices?: Array<{ | |
| message?: { content?: string | Array<{ text?: string } | string>; reasoning?: string; reasoning_content?: string }; | |
| finish_reason?: string; | |
| stop_reason?: string; | |
| }>; | |
| } | |
| function extractCloudflareText(result: CloudflareResponse | string, model: string): string { ... } |
| const CLOUDFLARE_MAX_RETRIES = 1; | ||
|
|
||
| function isText(value: unknown): value is string { | ||
| return typeof value === 'string' && value.trim().length > 0; |
There was a problem hiding this comment.
The isText helper returns false if a string is empty or only whitespace (value.trim().length > 0). In extractMessageContent, if the AI returns an array of empty strings, the .join('').trim() result will be an empty string, causing the function to return null (line 26). This subsequently triggers a generic 'empty response' error in extractCloudflareText. While likely desired, it treats 'empty content' as a failure rather than a valid (albeit empty) response.
| const startTime = Date.now(); | ||
| const url = `https://generativelanguage.googleapis.com/v1beta/models/${model}:generateContent?key=${env.GEMINI_API_KEY}`; | ||
| const maxRetries = 2; | ||
| const maxRetries = GOOGLE_MAX_RETRIES; |
There was a problem hiding this comment.
The variable 'lastError' is explicitly typed as 'any'. This bypasses TypeScript's type checking and can lead to runtime errors if the error object does not have the expected properties. It is recommended to use 'unknown' or 'Error | unknown' and employ type narrowing (e.g., 'if (lastError instanceof Error)') when accessing properties.
| const maxRetries = GOOGLE_MAX_RETRIES; | |
| let lastError: unknown; |
| const app = new Hono<AppEnv>(); | ||
|
|
||
| app.get('/', async (c) => { | ||
| await runOpportunisticJobMaintenance(c.env); |
There was a problem hiding this comment.
The calls to runOpportunisticJobMaintenance(c.env) on lines 13 and 23 are awaited without a try-catch block. Since this is described as 'opportunistic maintenance', it likely performs background cleanup tasks. If this function throws an error (e.g., due to a database timeout or connection issue), it will cause the entire API request to fail with a 500 error, preventing users from accessing job lists or details for a non-critical background task.
| await runOpportunisticJobMaintenance(c.env); | |
| try { | |
| await runOpportunisticJobMaintenance(c.env); | |
| } catch (e) { | |
| console.error('Opportunistic maintenance failed:', e); | |
| } |
| @@ -43,7 +54,7 @@ export function createJobsRouter() { | |||
| trigger: 'retry', | |||
| headRef: rawSource.head_ref, | |||
There was a problem hiding this comment.
On line 57, the code accesses currentConfig.parsedJson. However, the result of loadRepoConfig (line 39) is not checked for null or undefined. If the configuration fails to load or the repository is not found, currentConfig may be null, leading to a runtime crash. Previously, the code used a nullish coalescing operator with defaultRepoConfig to ensure a fallback value.
| headRef: rawSource.head_ref, | |
| configSnapshot: currentConfig?.parsedJson ?? defaultRepoConfig, |
There was a problem hiding this comment.
Codra Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f89d03ec01
ℹ️ About Codra in GitHub
Your team has set up Codra to review pull requests in this repo. Reviews are triggered when you:
- Open a pull request for review
- Mark a draft as ready
- Comment "@codra-app review"
If Codra has suggestions, it will comment; otherwise it will react with 👍.
Codra can also answer questions or update the PR. Try commenting "@codra-app address that feedback".
| }; | ||
|
|
||
| function normalizeGlobalConfig(config: any): ModelRouteConfig { | ||
| export function normalizeGlobalConfig(config: any): ModelRouteConfig { |
There was a problem hiding this comment.
The function 'normalizeGlobalConfig' uses 'any' for the 'config' parameter. This bypasses TypeScript's type checking and can lead to runtime errors if the input structure changes. It is better to use 'Partial' or 'unknown' with a type guard to ensure type safety.
| export function normalizeGlobalConfig(config: any): ModelRouteConfig { | |
| export function normalizeGlobalConfig(config: Partial<ModelRouteConfig>): ModelRouteConfig { |
| const inherited = parentReviews.get(file.path); | ||
| if (inherited) { | ||
| if (!canInheritParentFileReview(config, inherited)) { | ||
| logger.info(`Ignoring inherited review for ${file.path}; parent model ${inherited.model_used} is not in the current model strategy`); |
There was a problem hiding this comment.
The code uses 'true as any' when setting the value in 'currentReviews.set(file.path, true as any)' and 'currentReviews.set(file.path, inherited)'. This bypasses TypeScript's type checking and can lead to runtime errors if the Map is expected to contain specific object structures. It's better to use a proper sentinel value or ensure the Map type is correctly defined for the purpose.
| logger.info(`Ignoring inherited review for ${file.path}; parent model ${inherited.model_used} is not in the current model strategy`); | |
| Instead of using 'true as any', define a type for the Map values that accommodates both the review object and a marker for 'processed', or use a Set for tracking processed paths separately. |
| } | ||
|
|
||
| let eventName = message.eventName; | ||
| let payload = message.payload as GitHubWebhookPayload | undefined; |
There was a problem hiding this comment.
The job lease is claimed at the beginning of 'runReviewJob' using 'claimJobLease'. However, if the process crashes or fails before 'releaseJobLease' or 'heartbeatJobLease' is called, the lease might remain active until it expires naturally. While the implementation uses a lease-aware approach, the error handling logic for 'isRetryableModelError' calls 'releaseJobLease' but other non-retryable errors might skip it if not carefully structured in every possible execution path (though current try-catch seems to cover it).
| let payload = message.payload as GitHubWebhookPayload | undefined; | |
| Ensure all exit paths in the catch block explicitly handle lease release, or use a 'try...finally' block to guarantee 'releaseJobLease' is called when the job is finished (either successfully, via supersession, or via terminal failure). |
| return { action: 'ack' }; | ||
| } catch (error) { | ||
| const messageText = error instanceof Error ? error.message : 'Unknown review failure'; | ||
| if (messageText === 'JOB_SUPERSEDED') { |
There was a problem hiding this comment.
When a job is superseded ('JOB_SUPERSEDED' error), the code calls 'releaseJobLease' and returns 'ack'. However, it does not explicitly mark the job as 'superseded' in the database if it hasn't been already (the supersession is typically handled by the function that creates the new job, but the current job's local state handling could be more robust to ensure no orphaned 'running' statuses persist in sub-steps).
| if (messageText === 'JOB_SUPERSEDED') { | |
| Verify that 'releaseJobLease' or the superseding logic correctly transitions the status of the superseded job to a terminal state to prevent any potential state mismatch. |
| return { | ||
| async query<T>(sqlText: string, params: unknown[] = []) { | ||
| return (await sql.unsafe(sqlText, params.map(normalizeParam) as any[], { prepare: true })) as T[]; | ||
| return (await sql.unsafe(sqlText, params.map(normalizeParam) as any[], { prepare: false })) as T[]; |
There was a problem hiding this comment.
The use of 'as any[]' when mapping parameters bypasses TypeScript's type checking. Since 'params' is already 'unknown[]', and 'normalizeParam' likely returns a type compatible with the database driver, casting to 'unknown[]' or omitting the cast (if the function return type allows) would be safer and more aligned with TypeScript best practices.
| return (await sql.unsafe(sqlText, params.map(normalizeParam) as any[], { prepare: false })) as T[]; | |
| return (await sql.unsafe(sqlText, params.map(normalizeParam), { prepare: false })) as T[]; |
| @@ -43,7 +54,7 @@ export function createJobsRouter() { | |||
| trigger: 'retry', | |||
| headRef: rawSource.head_ref, | |||
There was a problem hiding this comment.
On line 57, the code accesses currentConfig.parsedJson. However, the result of loadRepoConfig (line 39) is not checked for null or undefined. If the configuration fails to load or the repository is not found, currentConfig may be null, leading to a runtime crash. Previously, the code used a nullish coalescing operator with defaultRepoConfig to ensure a fallback value.
| headRef: rawSource.head_ref, | |
| configSnapshot: currentConfig?.parsedJson ?? defaultRepoConfig, |
| super(message); | ||
| this.name = 'RetryableModelError'; | ||
| if (cause !== undefined) { | ||
| (this as any).cause = cause; |
There was a problem hiding this comment.
The RetryableModelError constructor and isRetryableModelError function use type assertions (as any) to access the cause property and retryable property. This bypasses TypeScript's type system and violates the guideline to avoid any where possible. While cause is optional, it should be handled via a custom interface or Object.defineProperty to maintain type safety.
| (this as any).cause = cause; | |
| Define a custom interface extending Error with the 'cause' property, or use Object.defineProperty(this, 'cause', { value: cause }) to avoid the 'any' cast. |
|
|
||
| return ( | ||
| isGoogleRateLimitError(error) || | ||
| /\b50[0-9]\b/.test(message) || |
There was a problem hiding this comment.
The function isTransientModelFailure calls message.toLowerCase() multiple times (lines 66-76) within the return statement. This creates unnecessary CPU overhead and string allocations, degrading performance on high-error scenarios.
| /\b50[0-9]\b/.test(message) || | |
| Move `const lower = message.toLowerCase();` outside the return statement to compute it only once. |
| } | ||
| attempts++; | ||
| if (isCloudflareModel(currentModel) && isCloudflareAllocationError(error)) { | ||
| unavailableProviders.add('cloudflare'); |
There was a problem hiding this comment.
If the modelsToTry array is empty, the for loop does not execute. Consequently, lastError remains undefined and sawTransientFailure remains false. The code then throws lastError (line 213/265), which results in throwing undefined instead of a proper Error object. This could break downstream error handling logic.
| unavailableProviders.add('cloudflare'); | |
| Initialize `lastError` to a default error or throw a specific error if the model list is empty before the loop. |
| @@ -110,6 +149,7 @@ export class ModelService { | |||
| const modelsToTry = [primary, ...fallbacks]; | |||
There was a problem hiding this comment.
The variables lastError in generateReview (line 151) and generateSummary (line 225) are typed as any. This reduces type safety and makes it harder to catch type errors at compile time.
| const modelsToTry = [primary, ...fallbacks]; | |
| Use a more specific type, such as `unknown` or `Error`, depending on the expected error types. |
There was a problem hiding this comment.
Codra Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e642bb6d34
ℹ️ About Codra in GitHub
Your team has set up Codra to review pull requests in this repo. Reviews are triggered when you:
- Open a pull request for review
- Mark a draft as ready
- Comment "@codra-app review"
If Codra has suggestions, it will comment; otherwise it will react with 👍.
Codra can also answer questions or update the PR. Try commenting "@codra-app address that feedback".
| }; | ||
|
|
||
| function normalizeGlobalConfig(config: any): ModelRouteConfig { | ||
| export function normalizeGlobalConfig(config: any): ModelRouteConfig { |
There was a problem hiding this comment.
The function signature still uses 'config: any', which bypasses TypeScript's type system. This can lead to runtime errors if invalid data structures are passed, as the compiler will not enforce the expected shape of the object.
| export function normalizeGlobalConfig(config: any): ModelRouteConfig { | |
| interface GlobalConfig { main?: string; fallbacks?: string[]; size_overrides?: number; } | |
| export function normalizeGlobalConfig(config: GlobalConfig): ModelRouteConfig { ... } |
| if (!isSupportedGitHubWebhookEvent(eventName)) { | ||
| logger.info(`Queue message ignored: unsupported GitHub event ${eventName}`); | ||
| return; | ||
| await releaseJobLease(env, job.id, leaseOwner); |
There was a problem hiding this comment.
In runReviewJob, if a retryable model error occurs (lines 205-213), the code enqueues a continuation and then releases the lease. However, if the error is not retryable, it proceeds to failJobAndCheckRun (line 217). There is a risk that if the error occurs during the releaseJobLease or immediately after, the job state might become inconsistent if multiple workers attempt to claim/release simultaneously. While claimJobLease uses a UUID, ensuring the release call is wrapped in a robust finally block or consistent error handler across all phases would be safer.
| await releaseJobLease(env, job.id, leaseOwner); | |
| Wrap the main execution logic in a try...finally block to ensure 'releaseJobLease' is always called unless the error is meant to persist the lease (though the current logic seems to release on most error paths). |
| const addModel = (model: string | null | undefined) => { | ||
| if (model) models.add(normalizeModelId(model)); | ||
| }; | ||
|
|
There was a problem hiding this comment.
The string 'gemma-4-31b-it' is hardcoded in multiple locations (lines 62, 530) as a fallback. If the preferred model configuration is missing, the system defaults to this specific model. This should ideally be a centralized constant or a configuration-driven default to prevent maintenance issues and ensure consistency across the codebase.
| const DEFAULT_MODEL_ID = 'gemma-4-31b-it'; // Move to a shared config/constants file |
| return null; | ||
| } | ||
|
|
||
| const pr = await github.getPullRequest(job.owner, job.repo, job.prNumber); |
There was a problem hiding this comment.
In runReviewJob, loadRepoConfig is called (lines 261-265). However, the job object itself contains a configSnapshot (line 359). While the code correctly prioritizes the freshly loaded config for the current execution, there is a slight architectural inconsistency in whether the job should rely on its snapshot or a fresh fetch, which might lead to unexpected behavior if repository settings change mid-review.
| return { | ||
| async query<T>(sqlText: string, params: unknown[] = []) { | ||
| return (await sql.unsafe(sqlText, params.map(normalizeParam) as any[], { prepare: true })) as T[]; | ||
| return (await sql.unsafe(sqlText, params.map(normalizeParam) as any[], { prepare: false })) as T[]; |
There was a problem hiding this comment.
The use of 'as any[]' when mapping parameters on line 22 bypasses TypeScript's type checking. To adhere to type safety guidelines, use 'unknown[]' or a more specific type that matches the expected input of the sql.unsafe method.
| return (await sql.unsafe(sqlText, params.map(normalizeParam) as any[], { prepare: false })) as T[]; | |
| return (await sql.unsafe(sqlText, params.map(normalizeParam) as unknown[], { prepare: false })) as T[]; |
| export async function upsertFileReview( | ||
| env: Pick<AppBindings, 'HYPERDRIVE'>, | ||
| jobId: string, | ||
| input: { |
There was a problem hiding this comment.
The input parameter in upsertFileReview is defined as a generic object literal. This reduces type safety and makes it harder for the TypeScript compiler to catch errors in the caller regarding the structure of the input object.
| input: { | |
| Define a specific interface for the input object to enforce structure and improve type safety. |
|
|
||
| /** Max wall-clock time allowed for a single Google AI Studio call (120 s). */ | ||
| const GOOGLE_TIMEOUT_MS = 120_000; | ||
| /** Max wall-clock time allowed for a single Google AI Studio call. */ |
There was a problem hiding this comment.
The timeout for Google API calls was reduced from 120,000ms (2 minutes) to 45,000ms (45 seconds). This is a 62.5% reduction. If the model takes longer than 45 seconds to process a request (e.g., due to network latency or complex model operations), the request will fail with a timeout error. This could lead to a spike in failed review jobs.
| /** Max wall-clock time allowed for a single Google AI Studio call. */ | |
| Review if 45s is sufficient for the specific model and network conditions, or increase it if the previous 120s was necessary. |
| const startTime = Date.now(); | ||
| const url = `https://generativelanguage.googleapis.com/v1beta/models/${model}:generateContent?key=${env.GEMINI_API_KEY}`; | ||
| const maxRetries = 2; | ||
| let lastError: any; |
There was a problem hiding this comment.
The maxRetries constant was changed from 2 to 1. The loop condition attempt <= maxRetries means the total number of attempts changed from 3 (attempts 0, 1, 2) to 2 (attempts 0, 1). This reduces the resilience against transient network failures. Verify if this reduction is intentional for the 'lease-aware' requirement or if the retry count should be preserved.
| let lastError: any; | |
| Ensure the reduction in retries aligns with the new timeout duration to avoid unnecessary failures. |
| @@ -46,7 +48,7 @@ export async function reviewWithGoogle( | |||
| ], | |||
| generationConfig: { | |||
| responseMimeType: 'application/json', | |||
There was a problem hiding this comment.
The maxOutputTokens configuration was reduced from 4096 to 3072. This reduction might cause the model to truncate the response if the generated content exceeds 3072 tokens, potentially losing important information in the review output.
| responseMimeType: 'application/json', | |
| Verify that 3072 tokens is sufficient for the expected output length of the model. |
| const url = `https://generativelanguage.googleapis.com/v1beta/models/${model}:generateContent?key=${env.GEMINI_API_KEY}`; | ||
| const maxRetries = 2; | ||
| let lastError: any; | ||
| const maxRetries = GOOGLE_MAX_RETRIES; |
There was a problem hiding this comment.
The variable lastError was changed from type any to unknown. This is a best practice in TypeScript to prevent implicit any types and improve type safety, as unknown requires type narrowing before use.
| const maxRetries = GOOGLE_MAX_RETRIES; | |
| Ensure the variable is properly narrowed if used in error handling logic. |
There was a problem hiding this comment.
Codra Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e642bb6d34
ℹ️ About Codra in GitHub
Your team has set up Codra to review pull requests in this repo. Reviews are triggered when you:
- Open a pull request for review
- Mark a draft as ready
- Comment "@codra-app review"
If Codra has suggestions, it will comment; otherwise it will react with 👍.
Codra can also answer questions or update the PR. Try commenting "@codra-app address that feedback".
| }; | ||
|
|
||
| function normalizeGlobalConfig(config: any): ModelRouteConfig { | ||
| export function normalizeGlobalConfig(config: any): ModelRouteConfig { |
There was a problem hiding this comment.
The function signature still uses 'config: any', which bypasses TypeScript's type system. This can lead to runtime errors if invalid data structures are passed, as the compiler will not enforce the expected shape of the object.
| export function normalizeGlobalConfig(config: any): ModelRouteConfig { | |
| interface GlobalConfig { main?: string; fallbacks?: string[]; size_overrides?: number; } | |
| export function normalizeGlobalConfig(config: GlobalConfig): ModelRouteConfig { ... } |
| if (!isSupportedGitHubWebhookEvent(eventName)) { | ||
| logger.info(`Queue message ignored: unsupported GitHub event ${eventName}`); | ||
| return; | ||
| await releaseJobLease(env, job.id, leaseOwner); |
There was a problem hiding this comment.
In runReviewJob, if a retryable model error occurs (lines 205-213), the code enqueues a continuation and then releases the lease. However, if the error is not retryable, it proceeds to failJobAndCheckRun (line 217). There is a risk that if the error occurs during the releaseJobLease or immediately after, the job state might become inconsistent if multiple workers attempt to claim/release simultaneously. While claimJobLease uses a UUID, ensuring the release call is wrapped in a robust finally block or consistent error handler across all phases would be safer.
| await releaseJobLease(env, job.id, leaseOwner); | |
| Wrap the main execution logic in a try...finally block to ensure 'releaseJobLease' is always called unless the error is meant to persist the lease (though the current logic seems to release on most error paths). |
| const addModel = (model: string | null | undefined) => { | ||
| if (model) models.add(normalizeModelId(model)); | ||
| }; | ||
|
|
There was a problem hiding this comment.
The string 'gemma-4-31b-it' is hardcoded in multiple locations (lines 62, 530) as a fallback. If the preferred model configuration is missing, the system defaults to this specific model. This should ideally be a centralized constant or a configuration-driven default to prevent maintenance issues and ensure consistency across the codebase.
| const DEFAULT_MODEL_ID = 'gemma-4-31b-it'; // Move to a shared config/constants file |
| return null; | ||
| } | ||
|
|
||
| const pr = await github.getPullRequest(job.owner, job.repo, job.prNumber); |
There was a problem hiding this comment.
In runReviewJob, loadRepoConfig is called (lines 261-265). However, the job object itself contains a configSnapshot (line 359). While the code correctly prioritizes the freshly loaded config for the current execution, there is a slight architectural inconsistency in whether the job should rely on its snapshot or a fresh fetch, which might lead to unexpected behavior if repository settings change mid-review.
| return { | ||
| async query<T>(sqlText: string, params: unknown[] = []) { | ||
| return (await sql.unsafe(sqlText, params.map(normalizeParam) as any[], { prepare: true })) as T[]; | ||
| return (await sql.unsafe(sqlText, params.map(normalizeParam) as any[], { prepare: false })) as T[]; |
There was a problem hiding this comment.
The use of 'as any[]' when mapping parameters on line 22 bypasses TypeScript's type checking. To adhere to type safety guidelines, use 'unknown[]' or a more specific type that matches the expected input of the sql.unsafe method.
| return (await sql.unsafe(sqlText, params.map(normalizeParam) as any[], { prepare: false })) as T[]; | |
| return (await sql.unsafe(sqlText, params.map(normalizeParam) as unknown[], { prepare: false })) as T[]; |
| export async function upsertFileReview( | ||
| env: Pick<AppBindings, 'HYPERDRIVE'>, | ||
| jobId: string, | ||
| input: { |
There was a problem hiding this comment.
The input parameter in upsertFileReview is defined as a generic object literal. This reduces type safety and makes it harder for the TypeScript compiler to catch errors in the caller regarding the structure of the input object.
| input: { | |
| Define a specific interface for the input object to enforce structure and improve type safety. |
|
|
||
| /** Max wall-clock time allowed for a single Google AI Studio call (120 s). */ | ||
| const GOOGLE_TIMEOUT_MS = 120_000; | ||
| /** Max wall-clock time allowed for a single Google AI Studio call. */ |
There was a problem hiding this comment.
The timeout for Google API calls was reduced from 120,000ms (2 minutes) to 45,000ms (45 seconds). This is a 62.5% reduction. If the model takes longer than 45 seconds to process a request (e.g., due to network latency or complex model operations), the request will fail with a timeout error. This could lead to a spike in failed review jobs.
| /** Max wall-clock time allowed for a single Google AI Studio call. */ | |
| Review if 45s is sufficient for the specific model and network conditions, or increase it if the previous 120s was necessary. |
| const startTime = Date.now(); | ||
| const url = `https://generativelanguage.googleapis.com/v1beta/models/${model}:generateContent?key=${env.GEMINI_API_KEY}`; | ||
| const maxRetries = 2; | ||
| let lastError: any; |
There was a problem hiding this comment.
The maxRetries constant was changed from 2 to 1. The loop condition attempt <= maxRetries means the total number of attempts changed from 3 (attempts 0, 1, 2) to 2 (attempts 0, 1). This reduces the resilience against transient network failures. Verify if this reduction is intentional for the 'lease-aware' requirement or if the retry count should be preserved.
| let lastError: any; | |
| Ensure the reduction in retries aligns with the new timeout duration to avoid unnecessary failures. |
| @@ -46,7 +48,7 @@ export async function reviewWithGoogle( | |||
| ], | |||
| generationConfig: { | |||
| responseMimeType: 'application/json', | |||
There was a problem hiding this comment.
The maxOutputTokens configuration was reduced from 4096 to 3072. This reduction might cause the model to truncate the response if the generated content exceeds 3072 tokens, potentially losing important information in the review output.
| responseMimeType: 'application/json', | |
| Verify that 3072 tokens is sufficient for the expected output length of the model. |
| const url = `https://generativelanguage.googleapis.com/v1beta/models/${model}:generateContent?key=${env.GEMINI_API_KEY}`; | ||
| const maxRetries = 2; | ||
| let lastError: any; | ||
| const maxRetries = GOOGLE_MAX_RETRIES; |
There was a problem hiding this comment.
The variable lastError was changed from type any to unknown. This is a best practice in TypeScript to prevent implicit any types and improve type safety, as unknown requires type narrowing before use.
| const maxRetries = GOOGLE_MAX_RETRIES; | |
| Ensure the variable is properly narrowed if used in error handling logic. |
There was a problem hiding this comment.
Codra Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e642bb6d34
ℹ️ About Codra in GitHub
Your team has set up Codra to review pull requests in this repo. Reviews are triggered when you:
- Open a pull request for review
- Mark a draft as ready
- Comment "@codra-app review"
If Codra has suggestions, it will comment; otherwise it will react with 👍.
Codra can also answer questions or update the PR. Try commenting "@codra-app address that feedback".
| }; | ||
|
|
||
| function normalizeGlobalConfig(config: any): ModelRouteConfig { | ||
| export function normalizeGlobalConfig(config: any): ModelRouteConfig { |
There was a problem hiding this comment.
The function signature still uses 'config: any', which bypasses TypeScript's type system. This can lead to runtime errors if invalid data structures are passed, as the compiler will not enforce the expected shape of the object.
| export function normalizeGlobalConfig(config: any): ModelRouteConfig { | |
| interface GlobalConfig { main?: string; fallbacks?: string[]; size_overrides?: number; } | |
| export function normalizeGlobalConfig(config: GlobalConfig): ModelRouteConfig { ... } |
| if (!isSupportedGitHubWebhookEvent(eventName)) { | ||
| logger.info(`Queue message ignored: unsupported GitHub event ${eventName}`); | ||
| return; | ||
| await releaseJobLease(env, job.id, leaseOwner); |
There was a problem hiding this comment.
In runReviewJob, if a retryable model error occurs (lines 205-213), the code enqueues a continuation and then releases the lease. However, if the error is not retryable, it proceeds to failJobAndCheckRun (line 217). There is a risk that if the error occurs during the releaseJobLease or immediately after, the job state might become inconsistent if multiple workers attempt to claim/release simultaneously. While claimJobLease uses a UUID, ensuring the release call is wrapped in a robust finally block or consistent error handler across all phases would be safer.
| await releaseJobLease(env, job.id, leaseOwner); | |
| Wrap the main execution logic in a try...finally block to ensure 'releaseJobLease' is always called unless the error is meant to persist the lease (though the current logic seems to release on most error paths). |
| const addModel = (model: string | null | undefined) => { | ||
| if (model) models.add(normalizeModelId(model)); | ||
| }; | ||
|
|
There was a problem hiding this comment.
The string 'gemma-4-31b-it' is hardcoded in multiple locations (lines 62, 530) as a fallback. If the preferred model configuration is missing, the system defaults to this specific model. This should ideally be a centralized constant or a configuration-driven default to prevent maintenance issues and ensure consistency across the codebase.
| const DEFAULT_MODEL_ID = 'gemma-4-31b-it'; // Move to a shared config/constants file |
| return null; | ||
| } | ||
|
|
||
| const pr = await github.getPullRequest(job.owner, job.repo, job.prNumber); |
There was a problem hiding this comment.
In runReviewJob, loadRepoConfig is called (lines 261-265). However, the job object itself contains a configSnapshot (line 359). While the code correctly prioritizes the freshly loaded config for the current execution, there is a slight architectural inconsistency in whether the job should rely on its snapshot or a fresh fetch, which might lead to unexpected behavior if repository settings change mid-review.
| return { | ||
| async query<T>(sqlText: string, params: unknown[] = []) { | ||
| return (await sql.unsafe(sqlText, params.map(normalizeParam) as any[], { prepare: true })) as T[]; | ||
| return (await sql.unsafe(sqlText, params.map(normalizeParam) as any[], { prepare: false })) as T[]; |
There was a problem hiding this comment.
The use of 'as any[]' when mapping parameters on line 22 bypasses TypeScript's type checking. To adhere to type safety guidelines, use 'unknown[]' or a more specific type that matches the expected input of the sql.unsafe method.
| return (await sql.unsafe(sqlText, params.map(normalizeParam) as any[], { prepare: false })) as T[]; | |
| return (await sql.unsafe(sqlText, params.map(normalizeParam) as unknown[], { prepare: false })) as T[]; |
| } catch (error) { | ||
| logger.error('Queue message processing failed; retrying', error instanceof Error ? error : new Error(String(error))); | ||
| message.retry(); | ||
| try { |
There was a problem hiding this comment.
runReviewJob
The code accesses result.action and result.delaySeconds without explicitly defining the return type of runReviewJob. While TypeScript infers types, relying on implicit any or unknown return types reduces type safety. It is recommended to define the return type to ensure the object structure matches expectations.
| try { | |
| Define the return type of `runReviewJob` (e.g., `Promise<{ action: 'ack' | 'retry'; delaySeconds?: number }>`). |
|
|
||
| /** Max wall-clock time allowed for a single Google AI Studio call (120 s). */ | ||
| const GOOGLE_TIMEOUT_MS = 120_000; | ||
| /** Max wall-clock time allowed for a single Google AI Studio call. */ |
There was a problem hiding this comment.
The timeout for Google API calls was reduced from 120,000ms (2 minutes) to 45,000ms (45 seconds). This is a 62.5% reduction. If the model takes longer than 45 seconds to process a request (e.g., due to network latency or complex model operations), the request will fail with a timeout error. This could lead to a spike in failed review jobs.
| /** Max wall-clock time allowed for a single Google AI Studio call. */ | |
| Review if 45s is sufficient for the specific model and network conditions, or increase it if the previous 120s was necessary. |
| const startTime = Date.now(); | ||
| const url = `https://generativelanguage.googleapis.com/v1beta/models/${model}:generateContent?key=${env.GEMINI_API_KEY}`; | ||
| const maxRetries = 2; | ||
| let lastError: any; |
There was a problem hiding this comment.
The maxRetries constant was changed from 2 to 1. The loop condition attempt <= maxRetries means the total number of attempts changed from 3 (attempts 0, 1, 2) to 2 (attempts 0, 1). This reduces the resilience against transient network failures. Verify if this reduction is intentional for the 'lease-aware' requirement or if the retry count should be preserved.
| let lastError: any; | |
| Ensure the reduction in retries aligns with the new timeout duration to avoid unnecessary failures. |
| @@ -46,7 +48,7 @@ export async function reviewWithGoogle( | |||
| ], | |||
| generationConfig: { | |||
| responseMimeType: 'application/json', | |||
There was a problem hiding this comment.
The maxOutputTokens configuration was reduced from 4096 to 3072. This reduction might cause the model to truncate the response if the generated content exceeds 3072 tokens, potentially losing important information in the review output.
| responseMimeType: 'application/json', | |
| Verify that 3072 tokens is sufficient for the expected output length of the model. |
| const url = `https://generativelanguage.googleapis.com/v1beta/models/${model}:generateContent?key=${env.GEMINI_API_KEY}`; | ||
| const maxRetries = 2; | ||
| let lastError: any; | ||
| const maxRetries = GOOGLE_MAX_RETRIES; |
There was a problem hiding this comment.
The variable lastError was changed from type any to unknown. This is a best practice in TypeScript to prevent implicit any types and improve type safety, as unknown requires type narrowing before use.
| const maxRetries = GOOGLE_MAX_RETRIES; | |
| Ensure the variable is properly narrowed if used in error handling logic. |
There was a problem hiding this comment.
Codra Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e642bb6d34
ℹ️ About Codra in GitHub
Your team has set up Codra to review pull requests in this repo. Reviews are triggered when you:
- Open a pull request for review
- Mark a draft as ready
- Comment "@codra-app review"
If Codra has suggestions, it will comment; otherwise it will react with 👍.
Codra can also answer questions or update the PR. Try commenting "@codra-app address that feedback".
| }; | ||
|
|
||
| function normalizeGlobalConfig(config: any): ModelRouteConfig { | ||
| export function normalizeGlobalConfig(config: any): ModelRouteConfig { |
There was a problem hiding this comment.
The function signature still uses 'config: any', which bypasses TypeScript's type system. This can lead to runtime errors if invalid data structures are passed, as the compiler will not enforce the expected shape of the object.
| export function normalizeGlobalConfig(config: any): ModelRouteConfig { | |
| interface GlobalConfig { main?: string; fallbacks?: string[]; size_overrides?: number; } | |
| export function normalizeGlobalConfig(config: GlobalConfig): ModelRouteConfig { ... } |
| if (!isSupportedGitHubWebhookEvent(eventName)) { | ||
| logger.info(`Queue message ignored: unsupported GitHub event ${eventName}`); | ||
| return; | ||
| await releaseJobLease(env, job.id, leaseOwner); |
There was a problem hiding this comment.
In runReviewJob, if a retryable model error occurs (lines 205-213), the code enqueues a continuation and then releases the lease. However, if the error is not retryable, it proceeds to failJobAndCheckRun (line 217). There is a risk that if the error occurs during the releaseJobLease or immediately after, the job state might become inconsistent if multiple workers attempt to claim/release simultaneously. While claimJobLease uses a UUID, ensuring the release call is wrapped in a robust finally block or consistent error handler across all phases would be safer.
| await releaseJobLease(env, job.id, leaseOwner); | |
| Wrap the main execution logic in a try...finally block to ensure 'releaseJobLease' is always called unless the error is meant to persist the lease (though the current logic seems to release on most error paths). |
| const addModel = (model: string | null | undefined) => { | ||
| if (model) models.add(normalizeModelId(model)); | ||
| }; | ||
|
|
There was a problem hiding this comment.
The string 'gemma-4-31b-it' is hardcoded in multiple locations (lines 62, 530) as a fallback. If the preferred model configuration is missing, the system defaults to this specific model. This should ideally be a centralized constant or a configuration-driven default to prevent maintenance issues and ensure consistency across the codebase.
| const DEFAULT_MODEL_ID = 'gemma-4-31b-it'; // Move to a shared config/constants file |
| return null; | ||
| } | ||
|
|
||
| const pr = await github.getPullRequest(job.owner, job.repo, job.prNumber); |
There was a problem hiding this comment.
In runReviewJob, loadRepoConfig is called (lines 261-265). However, the job object itself contains a configSnapshot (line 359). While the code correctly prioritizes the freshly loaded config for the current execution, there is a slight architectural inconsistency in whether the job should rely on its snapshot or a fresh fetch, which might lead to unexpected behavior if repository settings change mid-review.
| return { | ||
| async query<T>(sqlText: string, params: unknown[] = []) { | ||
| return (await sql.unsafe(sqlText, params.map(normalizeParam) as any[], { prepare: true })) as T[]; | ||
| return (await sql.unsafe(sqlText, params.map(normalizeParam) as any[], { prepare: false })) as T[]; |
There was a problem hiding this comment.
The use of 'as any[]' when mapping parameters on line 22 bypasses TypeScript's type checking. To adhere to type safety guidelines, use 'unknown[]' or a more specific type that matches the expected input of the sql.unsafe method.
| return (await sql.unsafe(sqlText, params.map(normalizeParam) as any[], { prepare: false })) as T[]; | |
| return (await sql.unsafe(sqlText, params.map(normalizeParam) as unknown[], { prepare: false })) as T[]; |
| } catch (error) { | ||
| logger.error('Queue message processing failed; retrying', error instanceof Error ? error : new Error(String(error))); | ||
| message.retry(); | ||
| try { |
There was a problem hiding this comment.
runReviewJob
The code accesses result.action and result.delaySeconds without explicitly defining the return type of runReviewJob. While TypeScript infers types, relying on implicit any or unknown return types reduces type safety. It is recommended to define the return type to ensure the object structure matches expectations.
| try { | |
| Define the return type of `runReviewJob` (e.g., `Promise<{ action: 'ack' | 'retry'; delaySeconds?: number }>`). |
|
|
||
| /** Max wall-clock time allowed for a single Google AI Studio call (120 s). */ | ||
| const GOOGLE_TIMEOUT_MS = 120_000; | ||
| /** Max wall-clock time allowed for a single Google AI Studio call. */ |
There was a problem hiding this comment.
The timeout for Google API calls was reduced from 120,000ms (2 minutes) to 45,000ms (45 seconds). This is a 62.5% reduction. If the model takes longer than 45 seconds to process a request (e.g., due to network latency or complex model operations), the request will fail with a timeout error. This could lead to a spike in failed review jobs.
| /** Max wall-clock time allowed for a single Google AI Studio call. */ | |
| Review if 45s is sufficient for the specific model and network conditions, or increase it if the previous 120s was necessary. |
| const startTime = Date.now(); | ||
| const url = `https://generativelanguage.googleapis.com/v1beta/models/${model}:generateContent?key=${env.GEMINI_API_KEY}`; | ||
| const maxRetries = 2; | ||
| let lastError: any; |
There was a problem hiding this comment.
The maxRetries constant was changed from 2 to 1. The loop condition attempt <= maxRetries means the total number of attempts changed from 3 (attempts 0, 1, 2) to 2 (attempts 0, 1). This reduces the resilience against transient network failures. Verify if this reduction is intentional for the 'lease-aware' requirement or if the retry count should be preserved.
| let lastError: any; | |
| Ensure the reduction in retries aligns with the new timeout duration to avoid unnecessary failures. |
| @@ -46,7 +48,7 @@ export async function reviewWithGoogle( | |||
| ], | |||
| generationConfig: { | |||
| responseMimeType: 'application/json', | |||
There was a problem hiding this comment.
The maxOutputTokens configuration was reduced from 4096 to 3072. This reduction might cause the model to truncate the response if the generated content exceeds 3072 tokens, potentially losing important information in the review output.
| responseMimeType: 'application/json', | |
| Verify that 3072 tokens is sufficient for the expected output length of the model. |
| const url = `https://generativelanguage.googleapis.com/v1beta/models/${model}:generateContent?key=${env.GEMINI_API_KEY}`; | ||
| const maxRetries = 2; | ||
| let lastError: any; | ||
| const maxRetries = GOOGLE_MAX_RETRIES; |
There was a problem hiding this comment.
The variable lastError was changed from type any to unknown. This is a best practice in TypeScript to prevent implicit any types and improve type safety, as unknown requires type narrowing before use.
| const maxRetries = GOOGLE_MAX_RETRIES; | |
| Ensure the variable is properly narrowed if used in error handling logic. |
1 participant
Description
This PR makes review jobs resilient to worker crashes, duplicate queue deliveries, and transient model/provider failures.
Closes #9
Type of change
How Has This Been Tested?
Checklist: