feat: implement POST /submissions/:id/dispute endpoint#106
Conversation
- Validate submission exists and belongs to authenticated user - Ensure only rejected submissions can be disputed - Prevent duplicate disputes (409 if already exists) - Create dispute record with 'open' status - Update submission status to 'disputed' in a transaction - Add comprehensive tests for all validation paths Closes devasignhq#29
Merge Score: 85/100🟢 The PR successfully implements the dispute endpoint with comprehensive test coverage and good validation. However, there is a critical TOCTOU (Time-of-Check to Time-of-Use) race condition where concurrent requests could create multiple disputes for the same submission. Moving the validation queries inside the database transaction with a row lock will resolve this issue. Code Suggestions (2)High Priority (1)
Reasoning: Currently, the submission status and existing dispute checks are performed outside the transaction. Concurrent requests could bypass these checks and create multiple disputes for the same submission. Moving these checks inside the transaction and using Suggested Code: const result = await db.transaction(async (tx) => {
// Fetch the submission and verify ownership + status with a row lock
const [submission] = await tx
.select()
.from(submissions)
.where(and(eq(submissions.id, id), eq(submissions.developerId, user.id)))
.for('update');
if (!submission) {
return { error: 'Submission not found', status: 404 as const };
}
if (submission.status !== 'rejected') {
return { error: 'Only rejected submissions can be disputed', status: 400 as const };
}
// Check if a dispute already exists for this submission
const [existingDispute] = await tx
.select()
.from(disputes)
.where(eq(disputes.submissionId, id));
if (existingDispute) {
return { error: 'A dispute already exists for this submission', status: 409 as const };
}
// Create the dispute and update submission status
const [newDispute] = await tx
.insert(disputes)
.values({
submissionId: id,
reason,
evidenceLinks: evidence_links,
status: 'open',
})
.returning();
await tx
.update(submissions)
.set({ status: 'disputed' })
.where(eq(submissions.id, id));
return { data: newDispute, status: 201 as const };
});
if ('error' in result) {
return c.json({ error: result.error }, result.status);
}
return c.json({ data: result.data }, result.status);Medium Priority (1)
Reasoning: After moving the read queries into the transaction to fix the race condition, the tests will need to be updated to mock the Suggested Code: const mockTransaction = vi.fn().mockImplementation(async (callback: any) => {
return callback({
select: vi.fn().mockImplementation(() => {
selectCallCount++;
if (selectCallCount === 1) return { from: mockFrom1 } as any;
return { from: mockFrom2 } as any;
}),
insert: mockInsert,
update: mockUpdate,
});
});📊 Review Metadata
|
There was a problem hiding this comment.
All criteria are met. The endpoint enforces JWT auth, validates UUID and reason, accepts optional evidence_links, verifies ownership, checks submission status, prevents duplicate disputes, and creates the dispute with status transition in a transaction. Tests cover all required paths.
There was a problem hiding this comment.
End goal
Provide a POST /submissions/:id/dispute endpoint that lets a developer dispute a rejected submission they own by creating an 'open' dispute record with reason and evidence.
✅ Acceptance criteria met (all 8 met)
- C1 — POST /api/submissions/:id/dispute requires a valid JWT and returns 401 when the request is unauthorized.
- C2 — The endpoint validates the submission id is a valid UUID and returns 400 when it is invalid.
- C3 — The endpoint requires a
reasonin the request body and returns 400 when it is missing. - C4 — The endpoint returns 400 when the submission's status is not
rejected. - C5 — The endpoint verifies the submission belongs to the authenticated user before allowing a dispute.
- C6 — The endpoint returns 409 when a dispute already exists for the submission.
- C7 — On success, a dispute record is created with
openstatus (capturing reason and evidence_links) and the submission transitions todisputed, performed atomically in a transaction, returning 201. - C8 — Tests cover all paths (auth, validation, duplicate prevention, and success).
The dispute endpoint implements all eight criteria: JWT auth check (401), UUID param validation via idSchema (400), reason validation via disputeSchema (400), status-not-rejected guard (400), ownership check in the WHERE clause, duplicate-dispute 409, and atomic transaction creating an 'open' dispute + transitioning the submission to 'disputed' returning 201. Tests cover all listed paths. No concrete defects found.
Summary
Implements
POST /api/submissions/:id/disputeendpoint as described in #29.Changes
Behavior
reasonrejectedopenstatus and transitions submission todisputedin a transaction, returns 201Request Body
{ "reason": "The code compiles fine on CI", "evidence_links": ["https://github.com/example/runs/123"] }Closes #29