Conversation
…emplateId and attach default ai config when creating a challenge
…v6 into PM-4062_ai-config
[DO NOT MERGE] PM-4062 default ai config
PM-4062 fix migration file
…ing phase after the submission phase
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| process.env.SUBMISSIONS_API_URL || "https://api.topcoder-dev.com/v5/submissions", | ||
| MEMBERS_API_URL: process.env.MEMBERS_API_URL || "https://api.topcoder-dev.com/v6/members", | ||
| REVIEW_SUMMATIONS_API_URL: process.env.REVIEW_SUMMATIONS_API_URL || "https://api.topcoder-dev.com/v6/reviewSummations", | ||
| REVIEWS_API_URL: process.env.REVIEWS_API_URL || "https://api.topcoder-dev.com", |
There was a problem hiding this comment.
[maintainability]
The REVIEWS_API_URL is set to https://api.topcoder-dev.com, which is inconsistent with the other API URLs that specify a version (e.g., /v5 or /v6). Consider adding a version to ensure consistency and avoid potential issues with future API changes.
| @@ -4,5 +4,6 @@ AUTH0_URL=https://topcoder-dev.auth0.com/oauth/token | |||
| AUTH0_AUDIENCE=https://m2m.topcoder-dev.com/ | |||
| DATABASE_URL="postgresql://topcoder:NUDvFEZzrey2x2og2!zn_kBzcaJ.fu_njAJcD*z2q@topcoder-services.ci8xwsszszsw.us-east-1.rds.amazonaws.com:5432/topcoder-services?schema=challenges" | |||
There was a problem hiding this comment.
[❗❗ security]
The DATABASE_URL contains hardcoded credentials, which is a security risk. Consider using environment variables or a secure vault to manage sensitive information.
PM-3926 ai screening phase
| '2025-03-10T13:08:02.378Z', | ||
| 'topcoder user' | ||
| ) | ||
| ON CONFLICT DO NOTHING; |
There was a problem hiding this comment.
[correctness]
Using ON CONFLICT DO NOTHING can silently ignore conflicts without logging or handling them, which might lead to data inconsistencies or missed updates. Consider specifying a conflict target and handling the conflict appropriately to ensure data integrity.
| * @param {Function} logDebugMessage optional logging function | ||
| */ | ||
| async addAIScreeningPhaseForChallengeCreation(challenge, prisma, logDebugMessage = () => {}) { | ||
| if (!challenge || !challenge.phases || !Array.isArray(challenge.reviewers)) { |
There was a problem hiding this comment.
[maintainability]
The function addAIScreeningPhaseForChallengeCreation mutates the challenge object in-place. Consider returning a new object or using a more functional approach to avoid potential side effects, which can improve maintainability.
| ); | ||
|
|
||
| if (!submissionPhaseName) { | ||
| throw new errors.BadRequestError( |
There was a problem hiding this comment.
[💡 maintainability]
Throwing an error when no submission phase is found is correct, but consider logging this event before throwing to aid in debugging and tracing issues in production.
| ([_, phase]) => phase.name === "AI Screening" | ||
| ); | ||
|
|
||
| if (!aiScreeningPhaseDefEntry) { |
There was a problem hiding this comment.
[💡 maintainability]
The error message 'AI Screening phase definition not found in the system' could be more informative by including the context or identifier of the challenge to help with debugging.
|
|
||
| // Recalculate phase dates to keep timeline in sync | ||
| if (submissionPhase.scheduledEndDate) { | ||
| aiScreeningPhase.scheduledStartDate = submissionPhase.scheduledEndDate; |
There was a problem hiding this comment.
[💡 performance]
The use of moment for date manipulation is fine, but consider using native JavaScript Date methods or a lighter library like date-fns if moment's full functionality is not needed, to reduce bundle size.
| @@ -98,9 +100,27 @@ class ChallengePhaseHelper { | |||
| // to incorrectly push earlier phases forward. Sorting by template order prevents that. | |||
| const orderIndex = new Map(); | |||
There was a problem hiding this comment.
[💡 performance]
The variable orderIndex is created using new Map() but is populated using _.each. Consider using native JavaScript methods like forEach for consistency and potentially better performance.
| const submissionPhase = submissionPhaseName | ||
| ? _.find(challengePhases, (phase) => phase.name === submissionPhaseName) | ||
| : null; | ||
| const submissionOrderIndex = _.isNil(submissionPhase) |
There was a problem hiding this comment.
[💡 readability]
The use of _.isNil to check for null or undefined is correct, but consider using submissionPhase === null for clarity since submissionPhase is explicitly set to null when not found.
| return updatedPhase; | ||
| }); | ||
|
|
||
| const aiScreeningPhase = _.find(updatedPhases, (phase) => phase.name === "AI Screening"); |
There was a problem hiding this comment.
[correctness]
The logic for updating the predecessor of aiScreeningPhase and review phases assumes that updateSubmissionPhase is not null. Ensure that this logic is covered by tests to prevent potential issues if updateSubmissionPhase is unexpectedly null.
PM-3926 - create the ai screening phase when challenge is launched
.github/workflows/trivy.yaml
Outdated
|
|
||
| - name: Run Trivy scanner in repo mode | ||
| uses: aquasecurity/trivy-action@0.33.1 | ||
| uses: aquasecurity/trivy-action@latest |
There was a problem hiding this comment.
[maintainability]
Using @latest for the Trivy action can lead to unexpected behavior if a breaking change is introduced in future versions. Consider specifying a specific version to ensure consistent behavior.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| // Recalculate phase dates to keep timeline in sync | ||
| if (submissionPhase.scheduledEndDate) { | ||
| aiScreeningPhase.scheduledStartDate = submissionPhase.scheduledEndDate; | ||
| aiScreeningPhase.scheduledEndDate = moment(aiScreeningPhase.scheduledStartDate) | ||
| .add(aiScreeningPhase.duration, "seconds") | ||
| .toDate() | ||
| .toISOString(); | ||
|
|
||
| logDebugMessage( | ||
| `AI screening phase dates calculated (start=${aiScreeningPhase.scheduledStartDate}, end=${aiScreeningPhase.scheduledEndDate})` | ||
| ); | ||
|
|
||
| // Update dates for review phases that now depend on AI Screening | ||
| reviewPhases.forEach((phase) => { | ||
| if (_.isNil(phase.actualStartDate)) { | ||
| phase.scheduledStartDate = aiScreeningPhase.scheduledEndDate; | ||
| if (phase.duration) { | ||
| phase.scheduledEndDate = moment(phase.scheduledStartDate) | ||
| .add(phase.duration, "seconds") | ||
| .toDate() | ||
| .toISOString(); | ||
|
|
||
| logDebugMessage( | ||
| `Updated ${phase.name} phase dates (start=${phase.scheduledStartDate}, end=${phase.scheduledEndDate})` | ||
| ); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
🔴 Incomplete phase date cascade after AI Screening insertion causes stale downstream dates
When addAIScreeningPhaseForChallengeCreation inserts the AI Screening phase during challenge activation (src/services/ChallengeService.js:2725-2737), it only recalculates dates for the AI Screening phase itself and its immediate successor review phases (lines 624-638). Downstream phases whose predecessor chain passes through a review phase (e.g., Appeals → depends on Review, Appeals Response → depends on Appeals, Post-Mortem, etc.) retain their stale scheduledStartDate/scheduledEndDate values that were computed by populatePhasesForChallengeUpdate before the AI Screening was inserted. This means those downstream phases will have dates that overlap with or precede the shifted review phase, producing an incorrect timeline that is persisted via syncChallengePhases (src/services/ChallengeService.js:3031-3038).
Example of the stale date problem
Before AI Screening insertion (dates from populatePhasesForChallengeUpdate):
- Submission: ends T+3d
- Review: starts T+3d, ends T+6d
- Appeals: starts T+6d, ends T+7d
After AI Screening insertion (only Review shifted):
- Submission: ends T+3d
- AI Screening: starts T+3d, ends T+3d+4h
- Review: starts T+3d+4h, ends T+6d+4h ← updated
- Appeals: starts T+6d, ends T+7d ← NOT updated, overlaps with Review
Prompt for agents
In src/common/challenge-helper.js, the addAIScreeningPhaseForChallengeCreation method (around lines 611-638) only updates dates for the immediate review phases that were re-linked as successors of AI Screening. It needs to cascade date recalculation through the entire downstream predecessor chain. After updating review phase dates, iterate through ALL remaining phases in challenge.phases and for each phase whose predecessor matches any already-updated phase's phaseId (and whose actualStartDate is nil), set its scheduledStartDate to the predecessor's scheduledEndDate and recompute scheduledEndDate. Continue this cascade until no more phases are updated. Alternatively, refactor to reuse the same predecessor-chain date propagation loop used in populatePhasesForChallengeUpdate (src/common/phase-helper.js lines 203-230) after inserting the AI Screening phase.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (Array.isArray(challenge.reviewers) && challenge.reviewers.length > 0) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🟡 Duplicate early-return guard check in applyDefaultMemberReviewersForChallengeCreation
The exact same guard check if (Array.isArray(challenge.reviewers) && challenge.reviewers.length > 0) { return; } appears twice in succession at lines 237-239 and 241-243. The second check is dead code that can never be reached (if the first check passes, the function already returned). This is harmless but suggests a copy-paste error.
| if (Array.isArray(challenge.reviewers) && challenge.reviewers.length > 0) { | |
| return; | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
3 participants
Uh oh!
There was an error while loading. Please reload this page.