Skip to content

Commit 6cc4092

Browse files
author
baton-admin[bot]
committed
chore: update baton-admin doc files
1 parent d5c3e66 commit 6cc4092

File tree

2 files changed

+324
-0
lines changed

2 files changed

+324
-0
lines changed
Lines changed: 276 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,276 @@
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.

.coderabbit.yaml

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json
2+
reviews:
3+
request_changes_workflow: true
4+
path_filters:
5+
- "!vendor/**"
6+
path_instructions:
7+
- path: "**/*.go"
8+
instructions: |
9+
## Secret Configuration Rule (Blocking)
10+
11+
All configuration fields that handle sensitive data MUST include `field.WithIsSecret(true)`.
12+
This prevents plaintext exposure of credentials and secrets.
13+
14+
Flag as a blocking issue any `field.StringField` (or similar field definition) where:
15+
1. The field name suggests sensitive data (e.g., contains "key", "secret", "token",
16+
"password", "credential", "private", "auth"), AND
17+
2. The field options do NOT include `field.WithIsSecret(true)`
18+
19+
Common secret field patterns that MUST have `field.WithIsSecret(true)`:
20+
- API keys: "api-key", "api_key", "apikey"
21+
- Passwords: "password", "passwd"
22+
- Secrets: "secret", "client-secret", "client_secret", "app-secret"
23+
- Tokens: "token", "access-token", "auth-token", "refresh-token"
24+
- Private keys: "private-key", "private_key", "pem"
25+
- Credentials: "credential", "credentials"
26+
27+
CORRECT example:
28+
```go
29+
APIKeyField = field.StringField(
30+
"api-key",
31+
field.WithDescription("API key for authentication"),
32+
field.WithRequired(true),
33+
field.WithIsSecret(true),
34+
)
35+
```
36+
37+
INCORRECT example (MUST request changes):
38+
```go
39+
APIKeyField = field.StringField(
40+
"api-key",
41+
field.WithDescription("API key for authentication"),
42+
field.WithRequired(true),
43+
// MISSING: field.WithIsSecret(true) - secrets will be exposed in plaintext
44+
)
45+
```
46+
47+
Failure to mark secrets with `field.WithIsSecret(true)` has caused production incidents
48+
where private keys and credentials were exposed in plaintext.

0 commit comments

Comments
 (0)