-
Notifications
You must be signed in to change notification settings - Fork 0
Devin Review Example PR #30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: devin/1762461057-sonar-ui-trigger
Are you sure you want to change the base?
Devin Review Example PR #30
Conversation
- Reduced SYSTEM_PROMPT from ~40 lines to ~7 lines - Reduced USER_PROMPT from ~100 lines to ~20 lines - Removed verbose Phase 1-4 instructions - Removed detailed security/bugs/code smells guidance - Kept essential information: issue summary, failed conditions, resources, and basic instructions This significantly reduces noise in Devin sessions while maintaining all necessary functionality. Co-Authored-By: Jake Cosme <jake@cognition.ai>
…rkflow Simplify SonarCloud workflow prompts to reduce noise
- Query hotspots API separately (api/hotspots/search) for accurate counts - Detect when failures are ONLY coverage/hotspot-review metrics (not code issues) - Skip Devin trigger for coverage-only failures - Post clear PR comment explaining action required for coverage/hotspot failures - Include hotspots details in issue summary with security category This prevents triggering Devin sessions when there are no actual code issues to fix, only coverage metrics or security hotspots requiring manual review. Co-Authored-By: Jake Cosme <jake@cognition.ai>
…erage-handling Fix hotspots and coverage-only quality gate failure handling
Co-Authored-By: Jake Cosme <jake@cognition.ai>
- Remove quality gate check step (no more red X failures) - Add wait-for-analysis step to ensure fresh Sonar results - Add Devin commit detection (checks author and [devin-remediation] tag) - Add issue collection step that always runs after scan - Trigger Devin only when: PR + human commit + code issues found - Infinite loop prevention: skips triggering if commit has [devin-remediation] tag Co-Authored-By: Jake Cosme <jake@cognition.ai>
This test file contains: - SQL injection vulnerability (string concatenation in query) - Bug (comparing two random values) - Code smell (deeply nested if statements) - Empty exception handler Purpose: Test that the SonarCloud workflow triggers Devin session when issues are found Co-Authored-By: Jake Cosme <jake@cognition.ai>
Added a comment to indicate the purpose of the code.
Co-Authored-By: Jake Cosme <jake@cognition.ai>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| String query = "SELECT * FROM users WHERE name = '" + userInput + "'"; | ||
| stmt.executeQuery(query); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 SQL Injection vulnerability via string concatenation
The vulnerableMethod constructs a SQL query by directly concatenating user input into the query string:
String query = "SELECT * FROM users WHERE name = '" + userInput + "'";
stmt.executeQuery(query);This is a classic SQL injection vulnerability. An attacker can pass malicious input like ' OR '1'='1 or '; DROP TABLE users; -- to bypass authentication, extract sensitive data, or destroy the database.
Actual behavior: User input is directly interpolated into the SQL string without any sanitization or parameterization.
Expected behavior: Use PreparedStatement with parameterized queries to prevent SQL injection:
PreparedStatement pstmt = conn.prepareStatement("SELECT * FROM users WHERE name = ?");
pstmt.setString(1, userInput);
pstmt.executeQuery();Impact: Critical security vulnerability that could lead to data breach, data loss, or unauthorized access.
Recommendation: Replace Statement with PreparedStatement and use parameterized queries: PreparedStatement pstmt = conn.prepareStatement("SELECT * FROM users WHERE name = ?"); pstmt.setString(1, userInput);
Was this helpful? React with 👍 or 👎 to provide feedback.
| } | ||
|
|
||
| public int buggyMethod() { | ||
| if (secureRandom.nextInt() == secureRandom.nextInt()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 Comparing two different random values will almost never be equal
The buggyMethod compares two separate calls to secureRandom.nextInt():
if (secureRandom.nextInt() == secureRandom.nextInt()) {
return 1;
}Each call to nextInt() generates a new random integer. The probability of two random integers being equal is approximately 1 in 2^32 (about 1 in 4.3 billion). This means the method will almost always return 0, making the return 1 branch effectively dead code.
Actual behavior: The condition is almost never true because each nextInt() call returns a different random value.
Expected behavior: If the intent was to compare a random value against something, the value should be stored first:
int randomValue = secureRandom.nextInt();
if (randomValue == someExpectedValue) {
return 1;
}Impact: Logic error that makes the code behave unexpectedly - the method will almost always return 0 regardless of intent.
Recommendation: Store the random value in a variable before comparison, or clarify the intended logic of this comparison.
Was this helpful? React with 👍 or 👎 to provide feedback.
Uh oh!
There was an error while loading. Please reload this page.