Skip to content

Demo pr ia cursor#176

Open
Mteheran wants to merge 7 commits into
mainfrom
DEMO-PR-IA-CURSOR
Open

Demo pr ia cursor#176
Mteheran wants to merge 7 commits into
mainfrom
DEMO-PR-IA-CURSOR

Conversation

@Mteheran

Copy link
Copy Markdown
Owner

No description provided.

@Mteheran Mteheran requested a review from Rinaplata as a code owner March 11, 2026 17:38

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

This PR introduces a Cursor CLI code-review step into the CI workflow and modifies RegionRoutes.cs. The workflow additions look structurally sound, but RegionRoutes.cs contains several critical bugs — hardcoded IDs and broken error-handling logic — that would break the API in production. These must be fixed before merging.

Issues found: 3 🚨 Critical, 2 ⚠️ Logic

return Results.BadRequest();
}

id = 1; //testing with item 1

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 Critical: This hardcodes the region ID to 1, completely ignoring the id parameter from the request. Every call to this endpoint will return Region 1 regardless of what the caller asks for. Remove this line.

return Results.NotFound();
return

Results.NotFound();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✨ Improvement: Splitting return Results.NotFound() across three lines with a blank line in the middle hurts readability for no benefit. Keep it as a single-line return statement.

var region = await db.Regions.Include(p => p.Departments)
.SingleOrDefaultAsync(p => p.Id == id);
.SingleOrDefaultAsync(p => p.Id == 0);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 Critical: The query is hardcoded to p.Id == 0, so this will always return null and the endpoint will always 404 — the actual id parameter is completely ignored. This should be p.Id == id.

{
return Results.BadRequest(RequestMessages.BadRequest);
if (region is not null)
{

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Logic: These nested null-checks around Results.BadRequest are unnecessary and introduce a silent failure path. If region were null (it can't be — it was checked above) or db were null, the invalid sort would be silently ignored and the request would proceed. Revert to the original direct return Results.BadRequest(...).

- name: Build with dotnet
run: dotnet build --configuration Release --no-restore

- name: Checkout repository

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Logic: This is a second actions/checkout that overwrites the working tree after dotnet build. The subsequent dotnet test --no-build relies on existing build artifacts, which this checkout may invalidate. Move this checkout + code-review step either before the build or into a separate job.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant