Conversation
…close it immediately)
…e no pending ai workflows
…nto PM-3926_ai-screening-phase
PM-3926 AI screening phase
…eeding with iterrative review
…ing escalation requests
…issions when review phase is re-opened
… of challenges going forwards, for rating purposes.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| }); | ||
|
|
||
| const reviewService = new ReviewService( | ||
| reviewClient as unknown as any, |
There was a problem hiding this comment.
[maintainability]
Casting reviewClient to any might hide potential type errors. Consider using a more specific type or interface to ensure type safety.
|
|
||
| const reviewService = new ReviewService( | ||
| reviewClient as unknown as any, | ||
| { logAction: async () => undefined } as any, |
There was a problem hiding this comment.
[💡 readability]
The logAction function is defined as async () => undefined, which might be misleading if it's expected to perform asynchronous operations. Consider clarifying its intended use or removing the async keyword if not needed.
| : `Backfill complete. rowsBuilt=${totalRowsBuilt} rowsUpserted=${totalRowsUpserted} staleRowsDeleted=${totalRowsDeleted}`, | ||
| ); | ||
| } finally { | ||
| await Promise.all([challengeClient.$disconnect(), reviewClient.$disconnect()]); |
There was a problem hiding this comment.
[correctness]
Ensure that the database connections are properly closed in all scenarios. Consider adding error handling for the disconnection process to avoid potential resource leaks.
|
|
||
| export function isFirst2FinishChallenge(type?: string): boolean { | ||
| const normalized = normalizeChallengeType(type); | ||
| return normalized === FIRST2FINISH_TYPE || normalized === TOPGEAR_TASK_TYPE; |
There was a problem hiding this comment.
[readability]
The isFirst2FinishChallenge function checks for both FIRST2FINISH_TYPE and TOPGEAR_TASK_TYPE. This seems inconsistent with the function name, which suggests it should only check for FIRST2FINISH_TYPE. Consider renaming the function or adjusting the logic to improve clarity.
| submissionId: string; | ||
| aiWorkflowRunId: string; | ||
| aiWorkflowId: string; | ||
| status: string; |
There was a problem hiding this comment.
[correctness]
Consider using a more specific type for status instead of string. If there are predefined statuses, using a union type like 'completed' | 'failed' | 'pending' can improve type safety and prevent errors.
| aiWorkflowId: string; | ||
| status: string; | ||
| score: number; | ||
| completedAt: string; |
There was a problem hiding this comment.
[correctness]
The completedAt field is typed as string. Consider using a more specific type like Date if possible, or ensure that the string is always in a consistent and parseable format (e.g., ISO 8601). This can help prevent potential issues with date handling.
| for (const resourceId of uniqueResources) { | ||
| try { | ||
| const created = await this.reviewService.createPendingReview( | ||
| const { created } = await this.reviewService.createPendingReview( |
There was a problem hiding this comment.
[correctness]
Destructuring the result of createPendingReview to directly access created is a good practice for clarity. However, ensure that createPendingReview always returns an object with a created property to avoid runtime errors.
| new Set( | ||
| (challenge.reviewers ?? []) | ||
| .map((reviewer) => reviewer.aiWorkflowId) | ||
| .filter((workflowId): workflowId is string => Boolean(workflowId)), |
There was a problem hiding this comment.
[correctness]
The type guard filter((workflowId): workflowId is string => Boolean(workflowId)) ensures that only non-null and non-undefined workflow IDs are included. However, consider explicitly checking for typeof workflowId === 'string' to ensure type safety, as Boolean(workflowId) will also filter out empty strings.
| status: ChallengeStatusEnum.COMPLETED, | ||
| trackId: '11111111-1111-1111-1111-111111111111', | ||
| typeId: '22222222-2222-2222-2222-222222222222', | ||
| metadata: { rated: 'true' }, |
There was a problem hiding this comment.
[💡 maintainability]
The use of hardcoded UUIDs for trackId and typeId in the test setup could lead to maintenance issues if these values need to be updated or reused in other tests. Consider defining these as constants at the top of the test file for better maintainability.
|
|
||
| it('skips rerate when rating metadata is absent', async () => { | ||
| const challenge = buildChallenge({ | ||
| prizeSets: [buildPlacementPrizeSet(2)], |
There was a problem hiding this comment.
[correctness]
The metadata object is being used to determine if a challenge is rated or unrated. Ensure that the logic for checking this metadata is consistent across all tests and that the metadata keys are well-defined and documented to avoid potential errors.
| const challenge = buildChallenge({ | ||
| status: ChallengeStatusEnum.ACTIVE, | ||
| numOfSubmissions: 2, | ||
| track: 'Data Science', |
There was a problem hiding this comment.
[maintainability]
The track and type values are hardcoded strings. Consider using enums or constants to define these values to prevent typos and ensure consistency across the codebase.
| } | ||
|
|
||
| for (const handle of handles) { | ||
| void this.memberApiService.refreshMemberStats(handle, challengeId); |
There was a problem hiding this comment.
[correctness]
The use of void before this.memberApiService.refreshMemberStats suppresses any unhandled promise rejections. Consider handling the promise explicitly to ensure any errors are logged or managed appropriately.
| } | ||
|
|
||
| for (const handle of handles) { | ||
| void this.memberApiService.rerateMemberStats( |
There was a problem hiding this comment.
[correctness]
The use of void before this.memberApiService.rerateMemberStats suppresses any unhandled promise rejections. Consider handling the promise explicitly to ensure any errors are logged or managed appropriately.
| @@ -381,6 +608,23 @@ export class ChallengeCompletionService { | |||
| ); | |||
| void this.financeApiService.generateChallengePayments(challengeId); | |||
There was a problem hiding this comment.
[correctness]
The use of void before this.financeApiService.generateChallengePayments suppresses any unhandled promise rejections. Consider handling the promise explicitly to ensure any errors are logged or managed appropriately.
| winner.type === PrizeSetTypeEnum.PLACEMENT, | ||
| ); | ||
| void this.syncChallengeResults( | ||
| challengeId, |
There was a problem hiding this comment.
[correctness]
The use of void before this.syncChallengeResults suppresses any unhandled promise rejections. Consider handling the promise explicitly to ensure any errors are logged or managed appropriately.
| challenge, | ||
| placementWinners, | ||
| ); | ||
| void this.triggerStatsRefreshForWinners( |
There was a problem hiding this comment.
[correctness]
The use of void before this.triggerStatsRefreshForWinners suppresses any unhandled promise rejections. Consider handling the promise explicitly to ensure any errors are logged or managed appropriately.
| void this.financeApiService.generateChallengePayments(challengeId); | ||
| await this.publishChallengeCompletionUpdate(challengeId, winners); | ||
| // Trigger member stats refresh and rerating for winning members. | ||
| void this.triggerStatsRefreshForWinners(challengeId, winners, challenge); |
There was a problem hiding this comment.
[correctness]
The use of void before this.triggerStatsRefreshForWinners suppresses any unhandled promise rejections. Consider handling the promise explicitly to ensure any errors are logged or managed appropriately.
|
|
||
| try { | ||
| const created = await this.reviewService.createPendingReview( | ||
| const { created } = await this.reviewService.createPendingReview( |
There was a problem hiding this comment.
[❗❗ correctness]
The change from const created = await this.reviewService.createPendingReview(...) to const { created } = await this.reviewService.createPendingReview(...) assumes that createPendingReview returns an object with a created property. Ensure that this assumption is correct and that the function consistently returns an object with this structure. If createPendingReview can return something other than an object or if created can be undefined, this could lead to runtime errors.
| this.challengeAllowsUnlimitedSubmissions(challenge); | ||
| const allowUnlimitedSubmissions = challengeAllowsUnlimitedSubmissions( | ||
| challenge, | ||
| (message) => this.logger.warn(message), |
There was a problem hiding this comment.
[❗❗ correctness]
The challengeAllowsUnlimitedSubmissions function now takes a logger function as a parameter. Ensure that this change is reflected wherever this utility function is used, as it could lead to runtime errors if the logger function is not provided.
| return; | ||
| } | ||
|
|
||
| if (isMarathonMatchChallenge(challenge.type) && isReviewPhase) { |
There was a problem hiding this comment.
[correctness]
The new condition for handling Marathon Match challenges in the review phase is added. Verify that this logic does not unintentionally bypass other necessary review processes for Marathon Match challenges.
| @@ -102,7 +118,7 @@ export class PhaseReviewService { | |||
| scorecardId = await this.reviewService.getScorecardIdByName( | |||
There was a problem hiding this comment.
[💡 style]
The catch block has been modified to remove the unused variable _. While this is a stylistic change, ensure that the removal does not affect any debugging or logging that might have relied on this variable.
| @@ -118,7 +134,7 @@ export class PhaseReviewService { | |||
| scorecardId = await this.reviewService.getScorecardIdByName( | |||
There was a problem hiding this comment.
[💡 style]
The catch block has been modified to remove the unused variable _. While this is a stylistic change, ensure that the removal does not affect any debugging or logging that might have relied on this variable.
| for (const resource of reviewerResources) { | ||
| try { | ||
| const created = await this.reviewService.createPendingReview( | ||
| const { created } = await this.reviewService.createPendingReview( |
There was a problem hiding this comment.
[❗❗ correctness]
The destructuring of the created property from createPendingReview might lead to issues if the function does not return an object with this property. Ensure that createPendingReview consistently returns an object with a created property.
| } | ||
|
|
||
| if (submissionIds.length && isReviewPhase) { | ||
| submissionIds = await this.excludeAiFailedReviewSubmissions( |
There was a problem hiding this comment.
[correctness]
The new logic for excluding AI failed review submissions is added. Ensure that this logic is correctly integrated with existing submission filtering logic and that it does not inadvertently exclude valid submissions.
|
|
||
| try { | ||
| const created = await this.reviewService.createPendingReview( | ||
| const { created } = await this.reviewService.createPendingReview( |
There was a problem hiding this comment.
[❗❗ correctness]
The destructuring of the created property from createPendingReview might lead to issues if the function does not return an object with this property. Ensure that createPendingReview consistently returns an object with a created property.
|
|
||
| if (this.isAiScreeningPhaseName(updatedPhase.name)) { | ||
| try { | ||
| const challenge = |
There was a problem hiding this comment.
[performance]
The getChallengeById method is called twice in the same block (lines 1345 and 1344). Consider storing the result in a variable to avoid redundant API calls, which can improve performance and reduce potential inconsistencies if the challenge data changes between calls.
| try { | ||
| const challenge = | ||
| await this.challengeApiService.getChallengeById(challengeId); | ||
| const aiWorkflowIds = this.getAiWorkflowIdsForChallenge(challenge); |
There was a problem hiding this comment.
[❗❗ correctness]
The getAiWorkflowIdsForChallenge method is called with the challenge object, which is retrieved from an API call. Ensure that the challenge object is not null or undefined before using it to prevent potential runtime errors.
| } | ||
|
|
||
| try { | ||
| parsed = JSON.parse(trimmed); |
There was a problem hiding this comment.
[❗❗ security]
Using JSON.parse on potentially untrusted input can lead to security vulnerabilities if the input is not properly sanitized. Consider validating the input format before parsing.
| } | ||
|
|
||
| if (parsed && typeof parsed === 'object') { | ||
| const record = parsed as Record<string, unknown>; |
There was a problem hiding this comment.
[correctness]
Casting parsed to Record<string, unknown> without checking its structure can lead to runtime errors if parsed is not an object. Consider adding a type guard to ensure parsed is indeed an object before casting.
| * Marathon Match outbound API configuration used by autopilot review orchestration. | ||
| */ | ||
| export default registerAs('marathonMatch', () => ({ | ||
| baseUrl: (process.env.MARATHON_MATCH_API_URL || '').trim(), |
There was a problem hiding this comment.
[correctness]
The baseUrl is being trimmed, but there is no check to ensure it is a valid URL. Consider validating the URL format to prevent potential runtime errors.
| DEFAULT_TIMEOUT_MS, | ||
| ), | ||
| systemResourceId: | ||
| (process.env.MARATHON_MATCH_SYSTEM_RESOURCE_ID || '').trim() || null, |
There was a problem hiding this comment.
[correctness]
The systemResourceId is set to null if the environment variable is not provided or is an empty string. Ensure that downstream code can handle null values appropriately to avoid potential null reference errors.
| * - `MEMBER_API_TIMEOUT_MS` (optional, default `15000`): HTTP timeout in milliseconds for member-api calls. | ||
| */ | ||
| export default registerAs('memberApi', () => ({ | ||
| baseUrl: (process.env.MEMBER_API_URL || '').trim(), |
There was a problem hiding this comment.
[correctness]
The baseUrl is being trimmed, which is good for removing accidental whitespace. However, consider validating the URL format to ensure it's a valid URL. This can prevent potential runtime errors if the URL is malformed.
| .integer() | ||
| .positive() | ||
| .default(15000), | ||
| MARATHON_MATCH_SYSTEM_RESOURCE_ID: Joi.string().optional().allow(null, ''), |
There was a problem hiding this comment.
[correctness]
Consider adding a validation rule to ensure MARATHON_MATCH_SYSTEM_RESOURCE_ID is a valid identifier if applicable, rather than just allowing any string or null. This could prevent potential issues with invalid resource IDs being used.
| }; | ||
|
|
||
| try { | ||
| token = await this.auth0Service.getAccessToken(); |
There was a problem hiding this comment.
[correctness]
Consider handling the case where getAccessToken might throw an error. Currently, if getAccessToken fails, it will not be logged, and the subsequent API call will proceed without a token, potentially leading to an unauthorized request.
| }; | ||
|
|
||
| try { | ||
| token = await this.auth0Service.getAccessToken(); |
There was a problem hiding this comment.
[correctness]
Consider handling the case where getAccessToken might throw an error. Currently, if getAccessToken fails, it will not be logged, and the subsequent API call will proceed without a token, potentially leading to an unauthorized request.
| } | ||
| } | ||
|
|
||
| private buildUrl(path: string): string | null { |
There was a problem hiding this comment.
[💡 design]
The buildUrl method returns null if baseUrl is not configured, which is checked in the calling methods. However, it might be more robust to throw an error or handle this case explicitly in the constructor to prevent potential misuse of the service without a valid baseUrl.
| ): Promise<void> { | ||
| const challengeId = challenge.id; | ||
| const systemResourceId = ( | ||
| this.configService.get<string>('marathonMatch.systemResourceId') || '' |
There was a problem hiding this comment.
[correctness]
The use of this.configService.get<string>('marathonMatch.systemResourceId') || '' could lead to unexpected behavior if the configuration value is null or undefined. Consider using a more explicit check for these values to ensure systemResourceId is correctly assigned.
| } | ||
|
|
||
| const marathonMatchConfig = | ||
| await this.marathonMatchApiService.getConfig(challengeId); |
There was a problem hiding this comment.
[correctness]
The marathonMatchConfig?.reviewScorecardId?.trim() could potentially lead to a runtime error if marathonMatchConfig is null or undefined. Consider adding a check to ensure marathonMatchConfig is defined before attempting to access reviewScorecardId.
| step: 'dispatch-success', | ||
| }, | ||
| }); | ||
| } catch (error) { |
There was a problem hiding this comment.
[💡 maintainability]
Consider adding more specific error handling for known error types to provide more granular logging and potentially different handling strategies for different error conditions.
| private readonly dbLogger: AutopilotDbLoggerService, | ||
| ) { | ||
| this.baseUrl = ( | ||
| this.configService.get<string>('memberApi.baseUrl') || '' |
There was a problem hiding this comment.
[correctness]
Consider handling the case where memberApi.baseUrl is not a valid URL. Currently, the code only trims the string but does not validate if it's a well-formed URL. This could lead to runtime errors if the URL is malformed.
| this.configService.get<string>('memberApi.baseUrl') || '' | ||
| ).trim(); | ||
| this.timeoutMs = | ||
| this.configService.get<number>('memberApi.timeoutMs') ?? 15000; |
There was a problem hiding this comment.
[💡 performance]
The default timeout of 15000ms (15 seconds) is set here. Ensure this value is appropriate for your use case, as it might be too long or too short depending on the expected response times of the member-api.
| 'Content-Type': 'application/json', | ||
| }; | ||
| if (token) { | ||
| axiosHeaders.Authorization = `Bearer ${token}`; |
There was a problem hiding this comment.
[❗❗ security]
Consider adding error handling for the getAccessToken call. If this call fails, token will be undefined, and the subsequent request will not have an Authorization header, potentially leading to unauthorized requests.
| 'Content-Type': 'application/json', | ||
| }; | ||
| if (token) { | ||
| axiosHeaders.Authorization = `Bearer ${token}`; |
There was a problem hiding this comment.
[❗❗ security]
Consider adding error handling for the getAccessToken call. If this call fails, token will be undefined, and the subsequent request will not have an Authorization header, potentially leading to unauthorized requests.
|
|
||
| const placementByUserId = new Map<string, number>(); | ||
| for (const winner of placementWinners) { | ||
| const normalizedUserId = normalizeUserId(winner.userId); |
There was a problem hiding this comment.
[❗❗ correctness]
The normalizeUserId function is used but not defined in this file. Ensure that it is imported from the correct module or defined within this file to avoid runtime errors.
| * @returns Finite timestamp or `0` when absent/invalid. | ||
| * @throws Never. | ||
| */ | ||
| function getTimestamp(value: Date | null): number { |
There was a problem hiding this comment.
[correctness]
The getTimestamp function assumes that the value is a valid Date object. Consider adding validation to ensure value is a Date instance to prevent potential runtime errors.
|
|
||
| beforeEach(() => { | ||
| const executeRawMock = jest.fn().mockResolvedValue(undefined); | ||
| const executeRawMock = jest.fn().mockResolvedValue(0); |
There was a problem hiding this comment.
[correctness]
Changing the resolved value from undefined to 0 for executeRawMock might affect tests that expect undefined. Ensure that all tests are updated to handle this change appropriately.
| callback: (tx: { | ||
| $executeRaw: typeof executeRawMock; | ||
| }) => Promise<void>, | ||
| }) => Promise<unknown>, |
There was a problem hiding this comment.
[correctness]
The return type of the callback in the transaction mock has been changed from Promise<void> to Promise<unknown>. Ensure that this change does not affect any tests that rely on the specific return type.
PM-3926 ai screening phase
| const latestIterativePhase = this.getLatestIterativePhase(challenge); | ||
|
|
||
| // If an AI Screening phase exists and hasn't completed yet, wait for it to finish | ||
| const aiScreeningPending = (challenge.phases ?? []).some( |
There was a problem hiding this comment.
[maintainability]
The check for aiScreeningPending is duplicated in multiple methods. Consider refactoring this logic into a separate method to improve maintainability and reduce code duplication.
| } | ||
|
|
||
| // If an AI Screening phase exists and hasn't completed yet, wait for it to finish | ||
| const aiScreeningPending = (challenge.phases ?? []).some( |
There was a problem hiding this comment.
[maintainability]
The check for aiScreeningPending is duplicated in multiple methods. Consider refactoring this logic into a separate method to improve maintainability and reduce code duplication.
| ); | ||
| } | ||
|
|
||
| if ( |
There was a problem hiding this comment.
[correctness]
The condition phase.name.toLowerCase().includes('screening') could potentially match unintended phase names that contain the word 'screening'. Consider using a more precise check if possible.
| ); | ||
| } | ||
|
|
||
| try { |
There was a problem hiding this comment.
[maintainability]
The try-catch block for deleteStalePendingSubmissionReviews does not rethrow the error or handle it in a way that affects the flow. If the deletion of stale reviews is critical, consider handling the error more explicitly.
There was a problem hiding this comment.
Pull request overview
This PR expands the autopilot/review domain to better support AI screening workflows, Marathon Match system review orchestration, and downstream post-completion side effects (persisting canonical challengeResult rows and triggering member stats refresh/rerate), including a backfill script for historical challenges.
Changes:
- Add review-db queries/utilities to derive and upsert canonical
challengeResultrows (plus a CLI backfill script). - Add AI Screening phase gating/early-close logic based on in-progress AI workflow runs, including a new Kafka topic + handler.
- Add Marathon Match review-phase orchestration (SYSTEM reviews) and outbound member-api integration for post-completion stats refresh/rerate.
Reviewed changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/review/review.service.ts | Adds AI decision helpers, Marathon Match readiness query, challenge-result build/sync, and AI workflow run counting; changes createPendingReview return shape. |
| src/review/review.service.spec.ts | Adds tests for challenge-result building/sync, Marathon Match readiness, AI decision filtering, and adjusts transaction mocking. |
| src/review/challenge-result.utils.ts | New utility module to select canonical submissions and build persistable challengeResult rows. |
| src/member-api/member-api.service.ts | New outbound client for member-api stats refresh + rerate with audit logging. |
| src/member-api/member-api.module.ts | New Nest module exporting MemberApiService. |
| src/marathon-match/marathon-match.module.ts | New Nest module wiring Marathon Match services. |
| src/marathon-match/marathon-match-review.service.ts | New service that creates/dispatches Marathon Match SYSTEM reviews when the review phase opens. |
| src/marathon-match/marathon-match-api.service.ts | New outbound client for Marathon Match API config lookup and system-score triggering. |
| src/kafka/types/topic-payload-map.type.ts | Adds payload typing for the new AI workflow completion topic. |
| src/kafka/consumers/autopilot.consumer.ts | Registers and dispatches the new AI workflow completion topic to the autopilot service. |
| src/kafka/constants/topics.ts | Adds AI_WORKFLOW_COMPLETED topic constant. |
| src/config/validation.ts | Adds env validation for Marathon Match + member-api integration variables. |
| src/config/sections/member-api.config.ts | New config section for member-api base URL + timeout. |
| src/config/sections/marathon-match.config.ts | New config section for Marathon Match base URL + timeout + system resource id. |
| src/config/configuration.ts | Registers new config sections (memberApi, marathonMatch). |
| src/autopilot/utils/challenge-metadata.utils.ts | New shared metadata parsing helpers (rated/unlimited submissions). |
| src/autopilot/services/scheduler.service.ts | Adds Marathon Match review close readiness gating and AI Screening close deferral/auto-close logic. |
| src/autopilot/services/scheduler.service.spec.ts | Adds tests covering Marathon Match review deferral and AI Screening deferral/auto-close behavior. |
| src/autopilot/services/phase-schedule-manager.service.ts | Adds AI Screening “close immediately after open” behavior in phase-chain path. |
| src/autopilot/services/phase-review.service.ts | Integrates Marathon Match review handling on review-phase open; filters AI-failed submissions; uses shared metadata helper; updates for new createPendingReview return type. |
| src/autopilot/services/phase-review.service.spec.ts | Adds coverage for AI-failed submission locking/skip behavior. |
| src/autopilot/services/first2finish.service.ts | Updates for new createPendingReview return type. |
| src/autopilot/services/challenge-completion.service.ts | Adds challenge-result sync + member stats refresh/rerate integrations and rated/unlimited metadata logic. |
| src/autopilot/services/challenge-completion.service.spec.ts | Adds tests for challenge-result sync + member-api calls and rated/unrated scenarios. |
| src/autopilot/services/autopilot.service.ts | Adds handler to react to AI workflow completion events by closing AI Screening early when safe. |
| src/autopilot/interfaces/autopilot.interface.ts | Adds AiWorkflowCompletedPayload. |
| src/autopilot/constants/review.constants.ts | Adds AI_SCREENING_PHASE_NAME. |
| src/autopilot/constants/challenge.constants.ts | Adds Marathon Match challenge-type identification helper. |
| src/autopilot/autopilot.module.ts | Wires new MemberApiModule and MarathonMatchModule into the autopilot module. |
| scripts/backfill-challenge-results.ts | New CLI to backfill/preview challengeResult rows using Challenge DB + Review DB URLs. |
| package.json | Adds backfill:challenge-results script entry. |
| README.md | Documents REVIEW_DB_URL, Marathon Match/member-api env vars, and the new backfill script usage. |
| .github/workflows/trivy.yaml | Bumps Trivy GitHub Action version. |
| .env.example | Adds member-api env variables. |
| .circleci/config.yml | Adds a branch filter entry for CI workflow execution. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
In getChallengeResultCandidates, the review status filter treats NULL review statuses as COMPLETED via COALESCE((r."status")::text, 'COMPLETED') = 'COMPLETED'. Elsewhere (e.g., readiness query) NULL is treated as pending, so this can incorrectly include reviews without an explicit COMPLETED status in the challenge-result aggregates. Consider filtering strictly on COMPLETED (or using COALESCE(..., 'PENDING')) to avoid counting incomplete reviews.
| /** | ||
| * Upsert canonical review-api `challengeResult` rows for one challenge and | ||
| * delete stale rows that no longer map to a participant outcome. | ||
| * @param challengeId Challenge whose rows should be synchronized. | ||
| * @param options Placement, rating, and audit context for the rows. | ||
| * @returns Summary of how many rows were built, upserted, and removed. | ||
| * @throws Error when building or persisting rows fails. | ||
| */ | ||
| async syncChallengeResultsForChallenge( | ||
| challengeId: string, | ||
| options: { | ||
| placementWinners: ChallengeResultPlacementWinner[]; | ||
| allowUnlimitedSubmissions: boolean; | ||
| ratedChallenge: boolean; | ||
| actor: string; | ||
| createdAt?: Date; | ||
| updatedAt?: Date; | ||
| }, | ||
| ): Promise<{ | ||
| rowsBuilt: number; | ||
| rowsUpserted: number; | ||
| staleRowsDeleted: number; | ||
| }> { | ||
| if (!challengeId) { | ||
| return { | ||
| rowsBuilt: 0, | ||
| rowsUpserted: 0, | ||
| staleRowsDeleted: 0, | ||
| }; | ||
| } | ||
|
|
||
| try { | ||
| const rows = await this.buildChallengeResultRecordsForChallenge( | ||
| challengeId, | ||
| options, | ||
| ); | ||
|
|
||
| if (!rows.length) { | ||
| void this.dbLogger.logAction('review.syncChallengeResults', { | ||
| challengeId, | ||
| status: 'SUCCESS', | ||
| source: ReviewService.name, | ||
| details: { | ||
| rowsBuilt: 0, | ||
| rowsUpserted: 0, | ||
| staleRowsDeleted: 0, | ||
| }, | ||
| }); | ||
| return { | ||
| rowsBuilt: 0, |
There was a problem hiding this comment.
syncChallengeResultsForChallenge upserts rows by issuing one $executeRaw per record inside a transaction loop. For challenges with many participants this becomes an N+1 write pattern and can significantly slow down completion/backfill. Consider batching into a single multi-row INSERT ... VALUES ... ON CONFLICT ... (using Prisma.join over VALUES) or another bulk upsert approach to reduce round-trips.
| nextPhases: any[], | ||
| ) => Promise<void> | void) | ||
| | null = null; | ||
| private phaseChainCallbackInitialized = false; |
There was a problem hiding this comment.
phaseChainCallbackInitialized is declared but never read or written elsewhere in this file. If it’s not needed, remove it to avoid dead state; if it is intended to guard callback registration/usage, wire it into the relevant code path so it actually enforces the intended invariant.
| private phaseChainCallbackInitialized = false; |
| ); | ||
| } | ||
| this.logger.log( | ||
| `Challenge ${challengeId} is not ACTIVE (status: ${challenge.status}); skipping finalization attempt.`, |
There was a problem hiding this comment.
When the challenge is already COMPLETED, this branch triggers finance/challenge-result sync/member refresh side effects, but then logs "skipping finalization attempt". That message is misleading operationally because work is still being performed here. Consider changing the log text (or moving it into the non-COMPLETED branch) to reflect that finalization is skipped but side effects are replayed/ensured.
| `Challenge ${challengeId} is not ACTIVE (status: ${challenge.status}); skipping finalization attempt.`, | |
| `Challenge ${challengeId} is not ACTIVE (status: ${challenge.status}); skipping main finalization workflow and only ensuring idempotent side effects (payments/results/stats).`, |
| pnpm backfill:challenge-results --dry-run --limit 10 | ||
|
|
||
| # Backfill specific completed challenges | ||
| pnpm backfill:challenge-results --challengeId <challenge-id>[,<challenge-id>] |
There was a problem hiding this comment.
The README’s new backfill examples use pnpm backfill:challenge-results ..., while earlier sections use npm run .... If the intent is to stay package-manager agnostic, consider switching these examples to npm run backfill:challenge-results -- ... (or document that pnpm is required) so the command format is consistent and works with the documented workflow.
| pnpm backfill:challenge-results --dry-run --limit 10 | |
| # Backfill specific completed challenges | |
| pnpm backfill:challenge-results --challengeId <challenge-id>[,<challenge-id>] | |
| npm run backfill:challenge-results -- --dry-run --limit 10 | |
| # Backfill specific completed challenges | |
| npm run backfill:challenge-results -- --challengeId <challenge-id>[,<challenge-id>] |
| } | ||
|
|
||
| async createPendingReview( | ||
| submissionId: string | null, | ||
| resourceId: string, | ||
| phaseId: string, | ||
| scorecardId: string, |
There was a problem hiding this comment.
createPendingReview now returns an object ({ created, reviewId }) instead of a boolean, which is a breaking change for any callers/mocks that still expect boolean. A repo search shows several Jest tests still doing createPendingReview.mockResolvedValue(true/false) (e.g. autopilot.service.spec.ts, first2finish.service.spec.ts, phase-review.service.spec.ts), which will cause runtime destructuring errors. Please update remaining callers/mocks to return the new shape (and adjust typings) to keep the test suite green.
| data.phaseId, | ||
| data.challengeId, | ||
| ); | ||
| const pendingEscalationRequests = |
There was a problem hiding this comment.
[correctness]
The method getPendingAiDecisionsEscalationsCount is called without error handling. Consider wrapping it in a try-catch block to handle potential errors gracefully, similar to other service calls in this file.
| ); | ||
|
|
||
| try { | ||
| await this.first2FinishService.handleSubmissionByChallengeId( |
There was a problem hiding this comment.
[💡 maintainability]
The call to handleSubmissionByChallengeId is wrapped in a try-catch block, but the error handling only logs the error. Consider whether additional error handling or recovery logic is needed, especially if this operation is critical.
|
|
||
| describe('getPendingAiDecisionsEscalationsCount', () => { | ||
| it('returns pending AI escalation count when query succeeds', async () => { | ||
| prismaMock.$queryRaw.mockResolvedValueOnce([{ count: '3' }]); |
There was a problem hiding this comment.
[correctness]
The test case relies on the count field being a string, which is then compared to a number in the assertion expect(result).toBe(3);. Consider converting the count to a number before the assertion to ensure type consistency.
| }); | ||
|
|
||
| it('logs error and rethrows when query fails', async () => { | ||
| prismaMock.$queryRaw.mockRejectedValueOnce(new Error('query failed')); |
There was a problem hiding this comment.
[💡 maintainability]
The error handling test case uses a generic error message 'query failed'. Consider using a more specific error message or error type to ensure that the test accurately reflects the expected behavior in different failure scenarios.
| challengeId: string, | ||
| allowedSubmissionIds: string[], | ||
| ): Promise<number> { | ||
| const trimmedPhaseId = phaseId?.trim(); |
There was a problem hiding this comment.
[correctness]
The phaseId parameter is trimmed and checked for truthiness, but challengeId is only checked for truthiness. Consider trimming challengeId as well to ensure consistent input validation.
| async getPendingAiDecisionsEscalationsCount( | ||
| challengeId: string, | ||
| ): Promise<number> { | ||
| const query = Prisma.sql` |
There was a problem hiding this comment.
[💡 correctness]
The SQL query uses COUNT(*)::int AS count, which is cast to an integer. Ensure that the database column types and casting are consistent across the application to avoid unexpected type issues.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔴 PhaseReviewService test constructor missing marathonMatchReviewService parameter, shifting all subsequent dependencies
The PhaseReviewService constructor was updated to accept marathonMatchReviewService as the 5th parameter (src/autopilot/services/phase-review.service.ts:45), but the test at phase-review.service.spec.ts:182-190 was not updated. The test passes 7 arguments instead of 8, causing all dependencies after position 4 to be shifted: challengeCompletionService mock → marathonMatchReviewService slot, reviewSummationApiService mock → challengeCompletionService slot, dbLogger mock → reviewSummationApiService slot, and dbLogger field becomes undefined. Any test path that triggers void this.dbLogger.logAction(...) will throw a TypeError: Cannot read properties of undefined because this.dbLogger is undefined.
(Refers to lines 182-190)
Prompt for agents
In src/autopilot/services/phase-review.service.spec.ts, the PhaseReviewService constructor at line 182-190 is missing the marathonMatchReviewService mock as the 5th parameter. The constructor signature in phase-review.service.ts:40-49 is: (challengeApiService, reviewService, resourcesService, configService, marathonMatchReviewService, challengeCompletionService, reviewSummationApiService, dbLogger). Add a marathonMatchReviewService mock before challengeCompletionService. You need to: 1) Create a marathonMatchReviewService mock in the beforeEach block like: `const marathonMatchReviewServiceMock = { handleReviewPhaseOpened: jest.fn().mockResolvedValue(undefined) } as unknown as jest.Mocked<MarathonMatchReviewService>;` 2) Pass it as the 5th argument in the PhaseReviewService constructor call.
Was this helpful? React with 👍 or 👎 to provide feedback.
| @@ -169,6 +173,8 @@ describe('PhaseReviewService', () => { | |||
| ] as any); | |||
| reviewService.createPendingReview.mockResolvedValue(true); | |||
There was a problem hiding this comment.
🔴 createPendingReview mock returns boolean instead of { created, reviewId } object, causing destructuring to yield undefined
The createPendingReview return type was changed from Promise<boolean> to Promise<{ created: boolean; reviewId: string | null }> in this PR (src/review/review.service.ts:1341). Production callers were updated to destructure: const { created } = await this.reviewService.createPendingReview(...). However, the test mock at line 174 still returns true (a primitive boolean). When JavaScript destructures { created } from true, created is undefined (falsy), so the if (created) { createdCount++ } logic in production code is never exercised in tests, silently breaking test coverage of the review creation counting path.
| reviewService.createPendingReview.mockResolvedValue(true); | |
| reviewService.createPendingReview.mockResolvedValue({ created: true, reviewId: 'review-1' }); |
Was this helpful? React with 👍 or 👎 to provide feedback.
| void this.triggerStatsRefreshForWinners( | ||
| challengeId, | ||
| placementWinners, | ||
| challenge, | ||
| ); |
There was a problem hiding this comment.
🟡 finalizeChallenge fresh completion path only refreshes member stats for placement winners, omitting passed-review winners
In challenge-completion.service.ts:753-757, the fresh completion path of finalizeChallenge passes only placementWinners to triggerStatsRefreshForWinners, meaning members who passed review but didn't win a prize placement never get their stats refreshed. This is inconsistent with completeChallengeWithWinners at line 785 which passes all winners (including PASSED_REVIEW), and the already-COMPLETED replay path at line 622-626 which also passes challenge.winners (all winner types). The result is that members with PASSED_REVIEW status only get stats refresh during replays or explicit winner completion, but not during the primary finalizeChallenge flow.
| void this.triggerStatsRefreshForWinners( | |
| challengeId, | |
| placementWinners, | |
| challenge, | |
| ); | |
| void this.triggerStatsRefreshForWinners( | |
| challengeId, | |
| winners, | |
| challenge, | |
| ); |
Was this helpful? React with 👍 or 👎 to provide feedback.
Uh oh!
There was an error while loading. Please reload this page.