|
| 1 | +--- |
| 2 | +name: review-connector |
| 3 | +description: Review a baton connector PR. Use when asked to review a connector, review a PR, review connector code, or run a connector code review. Trigger phrases include "review connector", "review PR", "connector review", "code review connector". |
| 4 | +--- |
| 5 | + |
| 6 | +# Review Baton Connector PR |
| 7 | + |
| 8 | +Perform a structured code review of a baton connector PR. Uses at most 3 focused agents with embedded criteria to minimize token usage. |
| 9 | + |
| 10 | +## Arguments |
| 11 | + |
| 12 | +Usage: `/review-connector [connector-name] [--base branch] [--fresh] [--pr <number|url>]` |
| 13 | + |
| 14 | +- `--fresh` — Force full review, ignore existing report. |
| 15 | +- `--pr <number|url>` — Specify PR number or URL. Auto-detects from branch if omitted. |
| 16 | + |
| 17 | +## Step 1: Determine Context |
| 18 | + |
| 19 | +1. **Project root:** If connector name given, use `~/projects/<name>`. Otherwise use CWD (verify `pkg/connector/connector.go` exists). |
| 20 | +2. **Base branch:** Use `--base` if given, else `git -C <root> symbolic-ref refs/remotes/origin/HEAD 2>/dev/null | sed 's@^refs/remotes/origin/@@'`, fallback `main`. |
| 21 | +3. **Current branch:** `git -C <root> branch --show-current` → store as `BRANCH_NAME`. |
| 22 | +4. **Changed files:** `git -C <root> diff --name-only <base>...HEAD`. Exclude `vendor/`, `conf.gen.go`, non-`.go` files (keep `go.mod`/`go.sum`). Stop if empty. |
| 23 | + |
| 24 | +## Step 1.1: Fetch PR Context |
| 25 | + |
| 26 | +1. Find PR: if `--pr` given, use `gh pr view <number|url> --repo <owner/repo> --json number,title,body,reviews,url,state`. Otherwise: `gh pr list --head <BRANCH_NAME> --repo <owner/repo> --json number,title,body,reviews,url,state --limit 1`. Derive repo from `git remote get-url origin`. |
| 27 | +2. If found, fetch inline review comments: `gh api repos/<owner>/<repo>/pulls/<number>/comments --jq '.[] | {path, line, body, user: .user.login}'` |
| 28 | +3. Store PR description and comments as `PR_CONTEXT`. Extract actionable change requests grouped by file as `PR_REQUESTED_CHANGES`. |
| 29 | +4. If no PR found, continue without PR context. |
| 30 | + |
| 31 | +## Step 1.5: Resume Detection |
| 32 | + |
| 33 | +Skip if `--fresh`. Check for `<root>/REVIEW_<BRANCH_NAME with / replaced by _>.md`. |
| 34 | + |
| 35 | +If exists: parse previous findings and files reviewed. Compare against current diff. Carry forward findings for unchanged files. Set `FILES_TO_REVIEW` to only new/modified files. If nothing changed, inform user and stop. |
| 36 | + |
| 37 | +If not exists: full review of all changed files. |
| 38 | + |
| 39 | +## Step 2: Gather Diffs |
| 40 | + |
| 41 | +For each category of files in `FILES_TO_REVIEW`, gather the git diff: |
| 42 | +``` |
| 43 | +git -C <root> diff <base>...HEAD -- <file-paths> |
| 44 | +``` |
| 45 | + |
| 46 | +The orchestrator reads diffs and passes them to agents. Agents do NOT read reference docs — all criteria are embedded in their prompts. |
| 47 | + |
| 48 | +## Step 3: Spawn Review Agents |
| 49 | + |
| 50 | +Classify `FILES_TO_REVIEW` and spawn **at most 3 agents** in parallel using the Task tool. |
| 51 | + |
| 52 | +### File Classification |
| 53 | + |
| 54 | +| File Pattern | Category | Agent | |
| 55 | +|---|---|---| |
| 56 | +| `pkg/connector/client*.go`, `pkg/client/*.go` | Client | sync-reviewer | |
| 57 | +| `pkg/connector/connector.go` | Connector Core | sync-reviewer | |
| 58 | +| `pkg/connector/resource_types.go` | Resource Types | sync-reviewer | |
| 59 | +| `pkg/connector/<resource>.go` (users.go, groups.go, etc.) | Resource Builder | sync-reviewer | |
| 60 | +| `pkg/connector/*_actions.go`, `pkg/connector/actions.go` | Provisioning | provisioning-reviewer | |
| 61 | +| `pkg/config/config.go` | Config | lightweight-reviewer | |
| 62 | +| `go.mod`, `go.sum` | Dependencies | lightweight-reviewer | |
| 63 | + |
| 64 | +### Agent 1: sync-reviewer (sonnet) |
| 65 | + |
| 66 | +Spawn with `subagent_type: "general-purpose"`. Reviews ALL non-provisioning Go files including breaking change detection. This is the main review agent. |
| 67 | + |
| 68 | +**Prompt template:** |
| 69 | + |
| 70 | +``` |
| 71 | +You are a code reviewer for a Baton connector (Go project syncing identity data from SaaS APIs into ConductorOne). |
| 72 | +
|
| 73 | +Review the diffs below against these criteria. For each finding provide JSON: |
| 74 | +{"id": "<code>", "severity": "critical|warning|suggestion", "file": "<path>", "lines": "<range>", "description": "<issue>", "recommendation": "<fix>", "confidence": 0-100} |
| 75 | +
|
| 76 | +Return a JSON array. Empty array if no issues. Only findings with confidence >= 80. |
| 77 | +
|
| 78 | +## CLIENT CRITERIA (C1-C7) |
| 79 | +- C1: API endpoints documented at top of client.go (endpoints, docs links, required scopes) |
| 80 | +- C2: Must use uhttp.BaseHttpClient, not raw http.Client |
| 81 | +- C3: Rate limits: return annotations with v2.RateLimitDescription from response headers |
| 82 | +- C4: All list functions must paginate |
| 83 | +- C5: DRY: central doRequest function; WithQueryParam patterns |
| 84 | +- C6: URL construction via url.JoinPath or url.Parse, never string concat |
| 85 | +- C7: Endpoint paths as constants, not inline strings |
| 86 | +
|
| 87 | +## RESOURCE CRITERIA (R1-R11) |
| 88 | +- R1: List methods return []*Type (pointer slices) |
| 89 | +- R2: No unused function parameters |
| 90 | +- R3: Clear variable names (groupMember not gm) |
| 91 | +- R4: Errors use %w (not %v), include baton-{service}: prefix, use uhttp.WrapErrors |
| 92 | +- R5: Use StaticEntitlements for uniform entitlements |
| 93 | +- R6: Use Skip annotations (SkipEntitlementsAndGrants, etc.) appropriately |
| 94 | +- R7: Missing API permissions = degrade gracefully, don't fail sync |
| 95 | +- R8: Pagination via SDK pagination.Bag (Push/Next/Marshal). Return "" when done. NEVER hardcode tokens. NEVER buffer all pages. |
| 96 | +- R9: User resources include: status, email, profile, login |
| 97 | +- R10: Resource IDs = stable immutable API IDs, never emails or mutable fields |
| 98 | +- R11: All API calls receive ctx; long loops check ctx.Done() |
| 99 | +
|
| 100 | +## CONNECTOR CRITERIA (N1-N4) |
| 101 | +- N1: ResourceSyncers() returns all implemented builders |
| 102 | +- N2: Metadata() has accurate DisplayName/Description |
| 103 | +- N3: Validate() exercises API credentials (not just return nil) |
| 104 | +- N4: New() accepts config, creates client properly |
| 105 | +
|
| 106 | +## HTTP SAFETY (H1-H5) |
| 107 | +- H1: defer resp.Body.Close() AFTER err check (panic if resp nil) |
| 108 | +- H2: No resp.StatusCode/resp.Body access when resp might be nil |
| 109 | +- H3: Type assertions use two-value form: x, ok := val.(Type) |
| 110 | +- H4: No error swallowing (log.Println + continue = silent data loss) |
| 111 | +- H5: No secrets in logs (apiKey, password, token values) |
| 112 | +
|
| 113 | +## BREAKING CHANGES (B1-B8) — Check in diffs: |
| 114 | +- B1: Resource type Id: field changes = CRITICAL (grants orphaned) |
| 115 | +- B2: Entitlement slug changes in NewAssignmentEntitlement/NewPermissionEntitlement = CRITICAL |
| 116 | +- B3: Resource ID derivation changes (user.ID→user.Email) = CRITICAL |
| 117 | +- B4: Parent hierarchy changes (org→workspace) = HIGH |
| 118 | +- B5: Removed resource types/entitlements = HIGH |
| 119 | +- B6: Trait type changes (NewUserResource→NewAppResource) = MEDIUM |
| 120 | +- B7: New required OAuth scopes = breaking |
| 121 | +- B8: SAFE: display name changes, adding new types, adding trait options, adding pagination |
| 122 | +
|
| 123 | +## TOP BUG DETECTION PATTERNS |
| 124 | +1. Pagination: `return resources, "", nil, nil` without conditional = stops after page 1 |
| 125 | +2. Pagination: `return resources, "next", nil, nil` hardcoded = infinite loop |
| 126 | +3. HTTP: defer resp.Body.Close() BEFORE if err != nil = panic |
| 127 | +4. HTTP: resp.StatusCode in error path without resp != nil check = panic |
| 128 | +5. Type assertion: .(Type) without , ok := = panic |
| 129 | +6. Error: log.Print(err) without return = silent data loss |
| 130 | +7. Error: fmt.Errorf("...%v", err) should be %w |
| 131 | +8. IDs: .Email as 3rd arg to NewUserResource = unstable ID |
| 132 | +9. ParentResourceId.Resource without nil check = panic |
| 133 | +
|
| 134 | +Read the FULL file content (using Read tool) ONLY when the diff suggests a potential issue that requires full-file context (e.g., pagination flow, resource builder structure). For simple pattern issues visible in the diff, the diff alone is sufficient. |
| 135 | +
|
| 136 | +<IF PR_CONTEXT: include PR description and filtered PR_REQUESTED_CHANGES here> |
| 137 | +
|
| 138 | +FILES AND DIFFS: |
| 139 | +<paste diffs here, grouped by file> |
| 140 | +``` |
| 141 | + |
| 142 | +### Agent 2: provisioning-reviewer (sonnet) |
| 143 | + |
| 144 | +Only spawn if `FILES_TO_REVIEW` contains `*_actions.go` or `actions.go` files. This agent MUST read the full provisioning files (not just diffs) because entity source correctness requires understanding the complete Grant/Revoke flow. |
| 145 | + |
| 146 | +**Prompt template:** |
| 147 | + |
| 148 | +``` |
| 149 | +You are reviewing provisioning (Grant/Revoke) code for a Baton connector. |
| 150 | +
|
| 151 | +CRITICAL CONTEXT — Entity Source Rules (caused 3 production reverts): |
| 152 | +- WHO (user/account ID): principal.Id.Resource |
| 153 | +- WHAT (group/role): entitlement.Resource.Id.Resource |
| 154 | +- WHERE (workspace/org): principal.ParentResourceId.Resource |
| 155 | +- NEVER get context from entitlement.Resource.ParentResourceId |
| 156 | +
|
| 157 | +In Revoke: |
| 158 | +- Principal: grant.Principal.Id.Resource |
| 159 | +- Entitlement: grant.Entitlement.Resource.Id.Resource |
| 160 | +- Context: grant.Principal.ParentResourceId.Resource |
| 161 | +
|
| 162 | +Review criteria (P1-P6, H1-H5): |
| 163 | +- P1: CRITICAL — entity source correctness per rules above |
| 164 | +- P2: Revoke uses grant.Principal and grant.Entitlement correctly |
| 165 | +- P3: Grant handles "already exists" as success; Revoke handles "not found" as success |
| 166 | +- P4: Validate params before API calls; wrap errors with gRPC status codes |
| 167 | +- P5: API argument order — multiple string params are easy to swap (verify against function signature) |
| 168 | +- P6: ParentResourceId nil check before access |
| 169 | +- H1-H5: (same HTTP safety rules as sync-reviewer) |
| 170 | +
|
| 171 | +Read the full provisioning files using the Read tool, then check the diff for what changed. |
| 172 | +
|
| 173 | +Return JSON array of findings (same format as above). Confidence >= 80 only. |
| 174 | +
|
| 175 | +<IF PR_CONTEXT: include filtered PR_REQUESTED_CHANGES> |
| 176 | +
|
| 177 | +FILES TO READ: <list full paths> |
| 178 | +DIFFS: <paste diffs> |
| 179 | +``` |
| 180 | + |
| 181 | +### Agent 3: lightweight-reviewer (haiku) |
| 182 | + |
| 183 | +Only spawn if `FILES_TO_REVIEW` contains config or dependency files. Use `model: "haiku"` for efficiency. |
| 184 | + |
| 185 | +**Prompt template:** |
| 186 | + |
| 187 | +``` |
| 188 | +Review these connector config/dependency changes: |
| 189 | +
|
| 190 | +Config criteria (G1-G4): |
| 191 | +- G1: conf.gen.go must NEVER be manually edited |
| 192 | +- G2: Fields use field.StringField/BoolField from SDK |
| 193 | +- G3: Required fields: WithRequired(true); secrets: WithIsSecret(true) |
| 194 | +- G4: No hardcoded credentials/URLs; base URL configurable |
| 195 | +
|
| 196 | +Dependency checks: |
| 197 | +- Is baton-sdk at a recent version? |
| 198 | +- Any unexpected new dependencies? |
| 199 | +- Any removed deps still needed? |
| 200 | +
|
| 201 | +Return JSON array of findings. Confidence >= 80 only. |
| 202 | +
|
| 203 | +DIFFS: |
| 204 | +<paste diffs> |
| 205 | +``` |
| 206 | + |
| 207 | +### Agent Spawning Rules |
| 208 | + |
| 209 | +- If no provisioning files changed → skip Agent 2 |
| 210 | +- If no config/dep files changed → skip Agent 3 |
| 211 | +- If only config/dep files changed → skip Agent 1, only spawn Agent 3 |
| 212 | +- Always spawn at least one agent |
| 213 | + |
| 214 | +## Step 4: Validate and Aggregate |
| 215 | + |
| 216 | +1. Merge `PREVIOUS_FINDINGS` with new agent results. |
| 217 | +2. Parse JSON arrays from each agent. Filter confidence < 80. |
| 218 | +3. Deduplicate: same file + line range → keep highest confidence. |
| 219 | +4. **Cross-validate entity sources** (if provisioning changed): Read the Grant/Revoke code yourself to verify P1/P2 findings. This is the #1 bug. |
| 220 | +5. **Cross-validate PR feedback**: Check `PR_REQUESTED_CHANGES` against findings. Add missing unaddressed items as PR-N warnings. |
| 221 | +6. Downgrade breaking changes gated behind config flags from critical → suggestion. |
| 222 | + |
| 223 | +## Step 5: Build and Test |
| 224 | + |
| 225 | +1. `cd <root> && make` — capture pass/fail. |
| 226 | +2. `cd <root> && go test ./...` — capture pass/fail. |
| 227 | + |
| 228 | +## Step 6: Write Report |
| 229 | + |
| 230 | +Write to `<root>/REVIEW_<sanitized_branch>.md`: |
| 231 | + |
| 232 | +```markdown |
| 233 | +# Connector Code Review: <name> |
| 234 | + |
| 235 | +**Branch:** `<branch>` | **Base:** `<base>` | **Date:** <date> |
| 236 | +**PR:** [#<n> — <title>](<url>) / No PR found |
| 237 | +**Review type:** Full / Resumed (from <prev date>) | **Build:** PASS/FAIL | **Tests:** PASS/FAIL |
| 238 | + |
| 239 | +## Summary |
| 240 | + |
| 241 | +| Severity | Count | |
| 242 | +|----------|-------| |
| 243 | +| Critical | X | |
| 244 | +| Warning | Y | |
| 245 | +| Suggestion | Z | |
| 246 | + |
| 247 | +## Breaking Changes |
| 248 | + |
| 249 | +<findings or "None detected."> |
| 250 | + |
| 251 | +## Unaddressed PR Feedback |
| 252 | + |
| 253 | +<findings or "None."> |
| 254 | + |
| 255 | +## Critical Issues |
| 256 | + |
| 257 | +<grouped by file> |
| 258 | + |
| 259 | +## Warnings |
| 260 | + |
| 261 | +<grouped by file> |
| 262 | + |
| 263 | +## Suggestions |
| 264 | + |
| 265 | +<grouped by file> |
| 266 | + |
| 267 | +## Files Reviewed |
| 268 | + |
| 269 | +| File | Category | Findings | Status | |
| 270 | +|------|----------|----------|--------| |
| 271 | +| `<path>` | <cat> | <n> | Reviewed / Carried forward | |
| 272 | +``` |
| 273 | + |
| 274 | +## Step 7: Present Results |
| 275 | + |
| 276 | +Print concise summary: severity counts, breaking changes detected (y/n), build/test status, critical findings with file:line, path to report. |
0 commit comments