feat(api): add GET /bounties/:id bounty detail endpoint#56
feat(api): add GET /bounties/:id bounty detail endpoint#56genesis-ai-labs-star wants to merge 3 commits intodevasignhq:mainfrom
Conversation
🟠 AI Code Review ResultsStatus: Changes Needed 🟠 Merge Score: 50/100🔴 Recommendation: ❌ This PR needs significant improvements before it should be merged. The PR successfully implements the new bounty detail endpoint and introduces a good dependency injection pattern for testability. However, it has a critical security gap due to missing input validation on the bounty ID. The tests are also incomplete, missing key edge cases. I'm requesting changes to address the validation and improve test coverage before this can be merged. 💡 Code Suggestions (5)🔴 High Priority (1)
💭 Reasoning: Validating input at the edge of the application is a fundamental security and robustness principle. This change ensures that only validly formatted IDs reach the database layer, providing clearer error messages to clients (400 vs. 500) and preventing unexpected database errors. Suggested Code: app.get('/bounties/:id', async (c) => {
const bountyId = c.req.param('id');
const uuidRegex = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i;
if (!uuidRegex.test(bountyId)) {
return c.json({ error: 'Invalid bounty ID format' }, 400);
}
const db = deps?.db;🟡 Medium Priority (3)
💭 Reasoning: Adding these tests ensures the endpoint is robust. The invalid ID test verifies the input validation, while the unassigned bounty test confirms that the conditional logic for the Suggested Code: it('should return 400 for invalid bounty ID format', async () => {
const res = await app.request('/bounties/not-a-valid-uuid');
expect(res.status).toBe(400);
});
it('should return bounty details with null assignee if unassigned', async () => {
const unassignedDb: DbLike = {
execute: async () => ({
rows: [
{
id: '11111111-1111-1111-1111-111111111111',
title: 'Test bounty',
// ... other fields
creator_id: '22222222-2222-2222-2222-222222222222',
assignee_id: null,
creator_username: 'creator',
creator_avatar_url: 'https://example.com/c.png',
assignee_username: null,
assignee_avatar_url: null,
application_count: 3,
},
],
}),
};
const appWithUnassigned = createApp({ db: unassignedDb });
const res = await appWithUnassigned.request('/bounties/11111111-1111-1111-1111-111111111111');
expect(res.status).toBe(200);
const body = await res.json();
expect(body.id).toBe('11111111-1111-1111-1111-111111111111');
expect(body.assignee).toBeNull();
});
💭 Reasoning: Following the principle of least privilege, APIs should not expose more information than necessary. Internal identifiers like UUIDs should generally not be part of a public contract unless there is a specific reason for it, as it increases the surface area for information gathering by potential attackers.
💭 Reasoning: Using the ORM's query builder provides better type safety, makes the code easier to refactor, and prevents entire classes of errors related to typos in column or table names. It aligns the implementation with the choice to add Drizzle as a dependency. Suggested Code: // This is a conceptual example assuming schemas are defined in './db/schema.ts'
// import { bounties, users, applications } from './db/schema';
// import { eq, count } from 'drizzle-orm';
// const subquery = db.select({ value: count() }).from(applications).where(eq(applications.bountyId, bounties.id)).as('application_count');
// const result = await db.select({
// ...bounties,
// creatorUsername: users.username,
// creatorAvatarUrl: users.avatarUrl,
// assigneeUsername: sql<string>`assignee.username`,
// assigneeAvatarUrl: sql<string>`assignee.avatar_url`,
// applicationCount: sql<number>`${subquery}`
// })
// .from(bounties)
// .innerJoin(users, eq(bounties.creatorId, users.id))
// .leftJoin(users, eq(bounties.assigneeId, users.id))
// .where(eq(bounties.id, bountyId))
// .limit(1);🔵 Low Priority (1)
💭 Reasoning: Spying on the mock dependency is a standard and efficient testing pattern. Using Suggested Code: it('should return bounty details when found', async () => {
const mockExecute = vi.spyOn(mockDb, 'execute').mockResolvedValueOnce({
rows: [
{
id: '11111111-1111-1111-1111-111111111111',
github_issue_id: 123,
repo_owner: 'acme',
repo_name: 'repo',
title: 'Test bounty',
description: 'Desc',
amount_usdc: '100',
tech_tags: ['ts'],
difficulty: 'beginner',
status: 'open',
deadline: null,
creator_id: '22222222-2222-2222-2222-222222222222',
assignee_id: '33333333-3333-3333-3333-333333333333',
created_at: '2026-02-19T00:00:00.000Z',
updated_at: '2026-02-19T00:00:00.000Z',
creator_username: 'creator',
creator_avatar_url: 'https://example.com/c.png',
assignee_username: 'assignee',
assignee_avatar_url: 'https://example.com/a.png',
application_count: 7,
},
],
});
const res = await app.request('/bounties/11111111-1111-1111-1111-111111111111');
expect(res.status).toBe(200);
const body = await res.json();
expect(body).toMatchObject({
id: '11111111-1111-1111-1111-111111111111',
repoOwner: 'acme',
repoName: 'repo',
title: 'Test bounty',
amountUsdc: '100',
applicationCount: 7,
creator: {
id: '22222222-2222-2222-2222-222222222222',
username: 'creator',
},
assignee: {
id: '33333333-3333-3333-3333-333333333333',
username: 'assignee',
},
});
mockExecute.mockRestore();
});📊 Review Metadata
|
|
Update: addressed review feedback.
All api tests pass: . |
🔄 Follow-Up AI Code ReviewStatus: Ready to Merge 🟢 Updated Merge Score: 95/100🟢 Recommendation: ✅ This PR looks great and is ready for merge! 📋 Previous Review SummaryThe PR successfully implements the new bounty detail endpoint and introduces a good dependency injection pattern for testability. However, it has a critical security gap due to missing input validation on the bounty ID. The tests are also incomplete, missing key edge cases. I'm requesting changes to address the validation and improve test coverage before this can be merged. Excellent work. The author has fully addressed all concerns from the previous review and gone above and beyond. Resolved Concerns:
Additional Improvements:
The PR is now in excellent shape and I recommend merging it. 💡 Code Suggestions✨ Great job! No specific suggestions at this time. 📊 Review Metadata
|
|
Follow-up: CI/tests are green and the bot review now marks this PR as Ready to Merge (95/100).\n\nIf this looks good, could you please merge? Happy to adjust any style or response shape preferences. |
|
@bishopBethel @truemperor quick ping — bot follow-up review marks this PR as Ready to Merge (95/100) and the branch is clean/mergeable. If the response shape looks good to you, could you please merge? (Recap: added UUID validation for :id (400 on invalid), improved tests incl. edge cases, and removed internal user UUIDs from public response.) |
|
@genesis-ai-labs-star I'm following the issue sequence, hence I can't merge and close #21 when there are other open issues before it |
2 participants
Implements GET /bounties/:id to return full bounty detail including creator info, assignee info (if assigned), application count, and status.\n\n- Adds endpoint in packages/api (Hono)\n- Injects db dependency into createApp for testability\n- Adds tests for the new endpoint\n\nFixes #21