MiguelPecegueiro PR Review
Goal
Do a real review (read diff, compare repo patterns, check commits/CI). Deliver a short comment for GitHub — not an essay.
Workflow
- Fetch PR:
gh pr view,gh pr diff, commits; compare with similar code in the repo. - Decide: Approve, Approve with nits, or Request changes (only for real blockers).
- Output in two layers (see below). Default to Layer 1 only.
Output layers
Layer 1 — GitHub comment (default)
2–4 lines max. What the reviewer will actually paste.
Template:
**[Approved | Request changes].** [One sentence: what the PR does + verdict.]
[Optional: one concrete nit or pre-merge check, only if it matters.]
Approve example:
**Approved.** Matches the other wallboards; schema fix in the last commit looks good.
Optional: note hub URL and what triggers `POST /update` in the PR description.
Request changes example:
**Request changes.** Service queries `Foo` but the view is `schema.Foo` — will 500 in prod.
Fix the SQL schema prefix, then good to merge.
Layer 2 — Detailed notes (only if asked)
Use when the user says "full review", "detailed", or "write-up". Structure: Summary → What looks good → Blockers/nits → Pre-merge checklist → Verdict. Still skip filler; no tables unless they add clarity.
Security lens (Layer 3)
Activate when the user asks for a security review, vulnerability scan, or bad practice check. Run this in addition to the standard review, not instead of it.
Check the diff for:
Critical — block the PR:
- Credentials, secrets, connection strings, API keys, tokens committed in any file
- SQL built by string concatenation (SQL injection)
- Auth bypass or missing auth on any new endpoint or route
- PII logged to console, file, or Elastic without masking
- New external HTTP calls without input validation (SSRF surface)
- Hardcoded internal IPs, hostnames, or environment-specific values
High — flag and require fix:
- Missing input validation on any user-controlled value reaching the DB or filesystem
- New dependency added without a security review note
- Exception swallowed silently (empty catch block) on a security-relevant operation
- CORS misconfiguration on new API endpoints
- JWT or token validation weakened or skipped
Medium — flag, judgment call:
- Overly broad exception handlers masking real errors
- Missing audit log entry for a state-changing operation (ISO 27001 relevance)
- Config values that should be in Key Vault but are in appsettings.json
- Dead code left in that contains credential patterns (even if commented out)
Out of scope for this review:
- Architectural decisions that were locked in a grill-me session
- Issues already tracked in Asana or Confluence as known debt
- Style preferences not covered by the global Cursor rules
For each finding: file name, line number, severity, one-line explanation, one-line fix. Do not reproduce credential values — reference by file and line only.
GoConnection context
- ISO 27001 active — security findings are not optional suggestions
- All secrets must be in Azure Key Vault (
goconnection-keyvault, RBAC mode) - SQL Server schemas:
Clientes.*,GoConnection.*,OCGoConnection.*via linked server - Deploy target: SRVGOB (IIS) — no container or cloud-native patterns expected
- Multi-tenant risk: changes to GC_OneAgentScripts affect all clients — flag blast radius
Rules
- Never approve a PR with a Critical security finding
- Never approve a PR where CI is failing unless the failure is demonstrably unrelated
- If a 2MB+ binary is in the diff, flag it — it probably should not be in source control
- Do not write long essays by default — Layer 1 is the default output
- Do not flag things that are clearly intentional and consistent with existing patterns