Skip to content

Commit faf1a87

Browse files
authored
Merge branch 'getsentry:master' into patch-2
2 parents d9d7282 + 59e97e7 commit faf1a87

File tree

345 files changed

+53287
-11910
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

345 files changed

+53287
-11910
lines changed
Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
---
2+
name: code-review
3+
description: Perform code reviews following Sentry engineering practices. Use when reviewing pull requests, examining code changes, or providing feedback on code quality. Covers security, performance, testing, and design review.
4+
---
5+
6+
# Sentry Code Review
7+
8+
Follow these guidelines when reviewing code for Sentry projects.
9+
10+
## Review Checklist
11+
12+
### Identifying Problems
13+
14+
Look for these issues in code changes:
15+
16+
- **Runtime errors**: Potential exceptions, null pointer issues, out-of-bounds access
17+
- **Performance**: Unbounded O(n²) operations, N+1 queries, unnecessary allocations
18+
- **Side effects**: Unintended behavioral changes affecting other components
19+
- **Backwards compatibility**: Breaking API changes without migration path
20+
- **ORM queries**: Complex Django ORM with unexpected query performance
21+
- **Security vulnerabilities**: Injection, XSS, access control gaps, secrets exposure
22+
23+
### Design Assessment
24+
25+
- Do component interactions make logical sense?
26+
- Does the change align with existing project architecture?
27+
- Are there conflicts with current requirements or goals?
28+
29+
### Test Coverage
30+
31+
Every PR should have appropriate test coverage:
32+
33+
- Functional tests for business logic
34+
- Integration tests for component interactions
35+
- End-to-end tests for critical user paths
36+
37+
Verify tests cover actual requirements and edge cases. Avoid excessive branching or looping in test code.
38+
39+
### Long-Term Impact
40+
41+
Flag for senior engineer review when changes involve:
42+
43+
- Database schema modifications
44+
- API contract changes
45+
- New framework or library adoption
46+
- Performance-critical code paths
47+
- Security-sensitive functionality
48+
49+
## Feedback Guidelines
50+
51+
### Tone
52+
53+
- Be polite and empathetic
54+
- Provide actionable suggestions, not vague criticism
55+
- Phrase as questions when uncertain: "Have you considered...?"
56+
57+
### Approval
58+
59+
- Approve when only minor issues remain
60+
- Don't block PRs for stylistic preferences
61+
- Remember: the goal is risk reduction, not perfect code
62+
63+
## Common Patterns to Flag
64+
65+
### Python/Django
66+
67+
```python
68+
# Bad: N+1 query
69+
for user in users:
70+
print(user.profile.name) # Separate query per user
71+
72+
# Good: Prefetch related
73+
users = User.objects.prefetch_related('profile')
74+
```
75+
76+
### TypeScript/React
77+
78+
```typescript
79+
// Bad: Missing dependency in useEffect
80+
useEffect(() => {
81+
fetchData(userId);
82+
}, []); // userId not in deps
83+
84+
// Good: Include all dependencies
85+
useEffect(() => {
86+
fetchData(userId);
87+
}, [userId]);
88+
```
89+
90+
### Security
91+
92+
```python
93+
# Bad: SQL injection risk
94+
cursor.execute(f"SELECT * FROM users WHERE id = {user_id}")
95+
96+
# Good: Parameterized query
97+
cursor.execute("SELECT * FROM users WHERE id = %s", [user_id])
98+
```
99+
100+
## References
101+
102+
- [Sentry Code Review Guidelines](https://develop.sentry.dev/engineering-practices/code-review/)

.agents/skills/find-bugs/SKILL.md

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
---
2+
name: find-bugs
3+
description: Find bugs, security vulnerabilities, and code quality issues in local branch changes. Use when asked to review changes, find bugs, security review, or audit code on the current branch.
4+
---
5+
6+
# Find Bugs
7+
8+
Review changes on this branch for bugs, security vulnerabilities, and code quality issues.
9+
10+
## Phase 1: Complete Input Gathering
11+
12+
1. Get the FULL diff: `git diff $(gh repo view --json defaultBranchRef --jq '.defaultBranchRef.name')...HEAD`
13+
2. If output is truncated, read each changed file individually until you have seen every changed line
14+
3. List all files modified in this branch before proceeding
15+
16+
## Phase 2: Attack Surface Mapping
17+
18+
For each changed file, identify and list:
19+
20+
* All user inputs (request params, headers, body, URL components)
21+
* All database queries
22+
* All authentication/authorization checks
23+
* All session/state operations
24+
* All external calls
25+
* All cryptographic operations
26+
27+
## Phase 3: Security Checklist (check EVERY item for EVERY file)
28+
29+
* [ ] **Injection**: SQL, command, template, header injection
30+
* [ ] **XSS**: All outputs in templates properly escaped?
31+
* [ ] **Authentication**: Auth checks on all protected operations?
32+
* [ ] **Authorization/IDOR**: Access control verified, not just auth?
33+
* [ ] **CSRF**: State-changing operations protected?
34+
* [ ] **Race conditions**: TOCTOU in any read-then-write patterns?
35+
* [ ] **Session**: Fixation, expiration, secure flags?
36+
* [ ] **Cryptography**: Secure random, proper algorithms, no secrets in logs?
37+
* [ ] **Information disclosure**: Error messages, logs, timing attacks?
38+
* [ ] **DoS**: Unbounded operations, missing rate limits, resource exhaustion?
39+
* [ ] **Business logic**: Edge cases, state machine violations, numeric overflow?
40+
41+
## Phase 4: Verification
42+
43+
For each potential issue:
44+
45+
* Check if it's already handled elsewhere in the changed code
46+
* Search for existing tests covering the scenario
47+
* Read surrounding context to verify the issue is real
48+
49+
## Phase 5: Pre-Conclusion Audit
50+
51+
Before finalizing, you MUST:
52+
53+
1. List every file you reviewed and confirm you read it completely
54+
2. List every checklist item and note whether you found issues or confirmed it's clean
55+
3. List any areas you could NOT fully verify and why
56+
4. Only then provide your final findings
57+
58+
## Output Format
59+
60+
**Prioritize**: security vulnerabilities > bugs > code quality
61+
62+
**Skip**: stylistic/formatting issues
63+
64+
For each issue:
65+
66+
* **File:Line** - Brief description
67+
* **Severity**: Critical/High/Medium/Low
68+
* **Problem**: What's wrong
69+
* **Evidence**: Why this is real (not already fixed, no existing test, etc.)
70+
* **Fix**: Concrete suggestion
71+
* **References**: OWASP, RFCs, or other standards if applicable
72+
73+
If you find nothing significant, say so - don't invent issues.
74+
75+
Do not make changes - just report findings. I'll decide what to address.
Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
The reference material in this skill is derived from the OWASP Cheat Sheet Series.
2+
3+
Source: https://cheatsheetseries.owasp.org/
4+
OWASP Foundation: https://owasp.org/
5+
6+
Original content is licensed under:
7+
8+
Creative Commons Attribution-ShareAlike 4.0 International (CC BY-SA 4.0)
9+
https://creativecommons.org/licenses/by-sa/4.0/
10+
11+
You are free to:
12+
- Share — copy and redistribute the material in any medium or format
13+
- Adapt — remix, transform, and build upon the material for any purpose,
14+
even commercially
15+
16+
Under the following terms:
17+
- Attribution — You must give appropriate credit, provide a link to the
18+
license, and indicate if changes were made.
19+
- ShareAlike — If you remix, transform, or build upon the material, you
20+
must distribute your contributions under the same license as the original.
21+
22+
Full license text: https://creativecommons.org/licenses/by-sa/4.0/legalcode

0 commit comments

Comments
 (0)