From e4662dbc555ab8514795bbdd1d73e3d588e4575b Mon Sep 17 00:00:00 2001 From: Russell Haering Date: Thu, 26 Feb 2026 10:40:58 -0800 Subject: [PATCH 1/5] feat: make docs staleness critical, streamline review output - Doc staleness findings are now critical severity (release blocker) - Tighter summary template: severity counts on one line, no verbose sections (Files Reviewed, Verdict, Breaking Changes) - Inline comments kept to 2-3 sentences - Summary doesn't repeat inline comment details --- skills/pr-review.md | 31 +++++++++++++------------------ 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/skills/pr-review.md b/skills/pr-review.md index 0c5c359..32e10b2 100644 --- a/skills/pr-review.md +++ b/skills/pr-review.md @@ -220,10 +220,10 @@ Procedure: - D3: Credential requirements — Required API scopes or permissions changed (new OAuth scopes, different permission levels, new authentication methods). - D4: Configuration fields — Config fields added, removed, or renamed in pkg/config/config.go. -If any of D1-D4 apply, read docs/connector.mdx to confirm the specific section that would need updating. +If any of D1-D4 apply, read docs/connector.mdx to confirm the specific section that would need updating. Documentation staleness is a critical blocker — the docs must be updated before merge. Return a JSON object: -{"status": "stale", "findings": [{"id": "D1", "section": "
", "reason": ""}]} +{"status": "stale", "findings": [{"id": "D1", "severity": "critical", "section": "
", "reason": ""}]} Or if none apply: {"status": "up_to_date"} @@ -246,7 +246,7 @@ DIFFS: 3. **Cross-validate entity sources** (if provisioning changed): Read the Grant/Revoke code yourself to verify P1/P2 findings. This is the #1 bug. 4. **Cross-validate PR feedback**: Check PR review comments against findings. Add missing unaddressed items as warnings. 5. Downgrade breaking changes gated behind config flags from critical → suggestion. -6. Parse the docs-reviewer (Agent 4) result. If status is "stale", convert each finding to a warning. +6. Parse the docs-reviewer (Agent 4) result. If status is "stale", convert each finding to **critical** severity — stale docs are a release blocker. **Deliverable:** A merged list of findings (code + docs) with duplicates removed. Print the count of findings by severity. @@ -256,28 +256,23 @@ DIFFS: Post findings directly as PR comments: -1. **Inline comments** on specific lines where issues are found, with the finding ID, severity, description, and recommendation. +1. **Inline comments** on specific lines where issues are found. Keep each comment to 2-3 sentences: what's wrong, why it matters, and how to fix it. -2. **Summary comment** with ALL of the following sections (do not omit any): +2. **Summary comment** — be concise. No more than a few sentences per finding. Use the following template (do not omit any section): ``` ### PR Review: -### Findings -| Severity | Count | -|----------|-------| -| Critical | N | -| Warning | N | -| Suggestion | N | +**Critical: N | Warning: N | Suggestion: N** -### Breaking Changes - +### Critical Issues + ### Documentation - + -### Files Reviewed -| File | Category | -|------|----------| -| ... | ... | +### Other Findings + ``` + +Do NOT include a "Files Reviewed" section, a "Verdict" section, or a "Breaking Changes" section. Do NOT repeat findings that were already posted as inline comments — just reference them briefly in the summary. Keep the entire summary comment short. From d3adfbd54c89d1e0cd75931081080fb09584b709 Mon Sep 17 00:00:00 2001 From: Russell Haering Date: Thu, 26 Feb 2026 10:43:07 -0800 Subject: [PATCH 2/5] fix: enable inline PR comments by requesting the MCP tool In agent mode, the inline comment MCP server is only installed if its tools are explicitly listed in --allowedTools. Without this, Claude can only post issue comments, not line-level review comments. --- .github/workflows/pr-review.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/pr-review.yaml b/.github/workflows/pr-review.yaml index f83c5d7..54961bb 100644 --- a/.github/workflows/pr-review.yaml +++ b/.github/workflows/pr-review.yaml @@ -34,6 +34,6 @@ jobs: use_sticky_comment: true track_progress: true include_fix_links: true - claude_args: --model claude-opus-4-6 --max-turns 100 + claude_args: --model claude-opus-4-6 --max-turns 100 --allowedTools "mcp__github_inline_comment__create_inline_comment" prompt: | Use the /pr-review skill to review this PR. From 9ba664170f5bef4327177ac7936e7590644e4807 Mon Sep 17 00:00:00 2001 From: Russell Haering Date: Thu, 26 Feb 2026 11:01:00 -0800 Subject: [PATCH 3/5] feat: enable inline comments and gh CLI tools for PR review Add recommended PR review tools from claude-code-action docs: - Inline code comments via MCP server - gh pr comment/diff/view for CLI access to PR context --- .github/workflows/pr-review.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/pr-review.yaml b/.github/workflows/pr-review.yaml index 54961bb..cc85b72 100644 --- a/.github/workflows/pr-review.yaml +++ b/.github/workflows/pr-review.yaml @@ -34,6 +34,6 @@ jobs: use_sticky_comment: true track_progress: true include_fix_links: true - claude_args: --model claude-opus-4-6 --max-turns 100 --allowedTools "mcp__github_inline_comment__create_inline_comment" + claude_args: --model claude-opus-4-6 --max-turns 100 --allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr comment:*),Bash(gh pr diff:*),Bash(gh pr view:*)" prompt: | Use the /pr-review skill to review this PR. From 3ea3a6555d3df4d2231de10ac885c1f31729ebdf Mon Sep 17 00:00:00 2001 From: Russell Haering Date: Thu, 26 Feb 2026 11:07:29 -0800 Subject: [PATCH 4/5] feat: simplify review to single severity level MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove critical/warning/suggestion tiers — every finding is an issue that needs to be addressed. Raise the bar: only flag things you'd reject in a human code review. Drop confidence thresholds and downgrade logic. --- skills/pr-review.md | 59 +++++++++++++++++++++++---------------------- 1 file changed, 30 insertions(+), 29 deletions(-) diff --git a/skills/pr-review.md b/skills/pr-review.md index 32e10b2..088caba 100644 --- a/skills/pr-review.md +++ b/skills/pr-review.md @@ -66,10 +66,12 @@ Spawn with `subagent_type: "general-purpose"`. Reviews ALL non-provisioning Go f ``` You are a code reviewer for a Baton connector (Go project syncing identity data from SaaS APIs into ConductorOne). -Review the diffs below against these criteria. For each finding provide JSON: -{"id": "", "severity": "critical|warning|suggestion", "file": "", "lines": "", "description": "", "recommendation": "", "confidence": 0-100} +Review the diffs below against these criteria. Only flag issues you are confident are real problems — things you would reject in a human code review. Do not flag style preferences or hypothetical concerns. -Return a JSON array. Empty array if no issues. Only findings with confidence >= 80. +For each finding provide JSON: +{"id": "", "file": "", "lines": "", "description": "", "recommendation": ""} + +Return a JSON array. Empty array if no issues. ## CLIENT CRITERIA (C1-C7) - C1: API endpoints documented at top of client.go (endpoints, docs links, required scopes) @@ -107,14 +109,14 @@ Return a JSON array. Empty array if no issues. Only findings with confidence >= - H5: No secrets in logs (apiKey, password, token values) ## BREAKING CHANGES (B1-B8) — Check in diffs: -- B1: Resource type Id: field changes = CRITICAL (grants orphaned) -- B2: Entitlement slug changes in NewAssignmentEntitlement/NewPermissionEntitlement = CRITICAL -- B3: Resource ID derivation changes (user.ID→user.Email) = CRITICAL -- B4: Parent hierarchy changes (org→workspace) = HIGH -- B5: Removed resource types/entitlements = HIGH -- B6: Trait type changes (NewUserResource→NewAppResource) = MEDIUM -- B7: New required OAuth scopes = breaking -- B8: SAFE: display name changes, adding new types, adding trait options, adding pagination +- B1: Resource type Id: field changes (grants orphaned) +- B2: Entitlement slug changes in NewAssignmentEntitlement/NewPermissionEntitlement +- B3: Resource ID derivation changes (user.ID→user.Email) +- B4: Parent hierarchy changes (org→workspace) +- B5: Removed resource types/entitlements +- B6: Trait type changes (NewUserResource→NewAppResource) +- B7: New required OAuth scopes +- B8: SAFE (do not flag): display name changes, adding new types, adding trait options, adding pagination ## TOP BUG DETECTION PATTERNS 1. Pagination: `return resources, "", nil, nil` without conditional = stops after page 1 @@ -142,6 +144,8 @@ Only spawn if changed files contain `*_actions.go` or `actions.go` files. This a ``` You are reviewing provisioning (Grant/Revoke) code for a Baton connector. +Only flag issues you are confident are real problems — things you would reject in a human code review. Do not flag style preferences or hypothetical concerns. + CRITICAL CONTEXT — Entity Source Rules (caused 3 production reverts): - WHO (user/account ID): principal.Id.Resource - WHAT (group/role): entitlement.Resource.Id.Resource @@ -154,7 +158,7 @@ In Revoke: - Context: grant.Principal.ParentResourceId.Resource Review criteria (P1-P6, H1-H5): -- P1: CRITICAL — entity source correctness per rules above +- P1: Entity source correctness per rules above - P2: Revoke uses grant.Principal and grant.Entitlement correctly - P3: Grant handles "already exists" as success; Revoke handles "not found" as success - P4: Validate params before API calls; wrap errors with gRPC status codes @@ -164,7 +168,7 @@ Review criteria (P1-P6, H1-H5): Read the full provisioning files using the Read tool, then check the diff for what changed. -Return JSON array of findings (same format as above). Confidence >= 80 only. +Return JSON array of findings (same format as sync-reviewer). Empty array if no issues. FILES TO READ: DIFFS: @@ -177,7 +181,7 @@ Only spawn if changed files contain config or dependency files. Use `model: "hai **Prompt template:** ``` -Review these connector config/dependency changes: +Review these connector config/dependency changes. Only flag issues you are confident are real problems. Config criteria (G1-G4): - G1: conf.gen.go must NEVER be manually edited @@ -190,7 +194,7 @@ Dependency checks: - Any unexpected new dependencies? - Any removed deps still needed? -Return JSON array of findings. Confidence >= 80 only. +Return JSON array of findings (same format as sync-reviewer). Empty array if no issues. DIFFS: @@ -220,10 +224,10 @@ Procedure: - D3: Credential requirements — Required API scopes or permissions changed (new OAuth scopes, different permission levels, new authentication methods). - D4: Configuration fields — Config fields added, removed, or renamed in pkg/config/config.go. -If any of D1-D4 apply, read docs/connector.mdx to confirm the specific section that would need updating. Documentation staleness is a critical blocker — the docs must be updated before merge. +If any of D1-D4 apply, read docs/connector.mdx to confirm the specific section that would need updating. Documentation staleness is a blocker — the docs must be updated before merge. Return a JSON object: -{"status": "stale", "findings": [{"id": "D1", "severity": "critical", "section": "
", "reason": ""}]} +{"status": "stale", "findings": [{"id": "D1", "section": "
", "reason": ""}]} Or if none apply: {"status": "up_to_date"} @@ -241,14 +245,14 @@ DIFFS: ## Step 3: Validate and Aggregate -1. Parse JSON arrays from code review agents (Agents 1-3). Filter confidence < 80. -2. Deduplicate: same file + line range → keep highest confidence. +1. Parse JSON arrays from code review agents (Agents 1-3). +2. Deduplicate: same file + line range → keep one. 3. **Cross-validate entity sources** (if provisioning changed): Read the Grant/Revoke code yourself to verify P1/P2 findings. This is the #1 bug. -4. **Cross-validate PR feedback**: Check PR review comments against findings. Add missing unaddressed items as warnings. -5. Downgrade breaking changes gated behind config flags from critical → suggestion. -6. Parse the docs-reviewer (Agent 4) result. If status is "stale", convert each finding to **critical** severity — stale docs are a release blocker. +4. **Cross-validate PR feedback**: Check PR review comments against findings. Add any unaddressed items from human reviewers. +5. Drop findings that are fully mitigated by a config flag or feature gate. +6. Parse the docs-reviewer (Agent 4) result. If status is "stale", include each finding — stale docs are a release blocker. -**Deliverable:** A merged list of findings (code + docs) with duplicates removed. Print the count of findings by severity. +**Deliverable:** A merged list of all findings with duplicates removed. Print the count. --- @@ -263,16 +267,13 @@ Post findings directly as PR comments: ``` ### PR Review: -**Critical: N | Warning: N | Suggestion: N** +**Issues: N** -### Critical Issues - +### Issues + ### Documentation - -### Other Findings - ``` Do NOT include a "Files Reviewed" section, a "Verdict" section, or a "Breaking Changes" section. Do NOT repeat findings that were already posted as inline comments — just reference them briefly in the summary. Keep the entire summary comment short. From 33d7892ed1fcfbac60143a6a0d6915f9aadb4795 Mon Sep 17 00:00:00 2001 From: Russell Haering Date: Thu, 26 Feb 2026 11:16:09 -0800 Subject: [PATCH 5/5] feat: aggressive review, orchestrator validation, breaking changes section MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove hedging language — flag every criterion violation - Orchestrator validates sub-agent findings and drops incorrect ones - Add dedicated Breaking Changes section to summary template - Remove config-flag escape hatch from aggregation --- skills/pr-review.md | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/skills/pr-review.md b/skills/pr-review.md index 088caba..989028c 100644 --- a/skills/pr-review.md +++ b/skills/pr-review.md @@ -66,7 +66,7 @@ Spawn with `subagent_type: "general-purpose"`. Reviews ALL non-provisioning Go f ``` You are a code reviewer for a Baton connector (Go project syncing identity data from SaaS APIs into ConductorOne). -Review the diffs below against these criteria. Only flag issues you are confident are real problems — things you would reject in a human code review. Do not flag style preferences or hypothetical concerns. +Review the diffs below against every criterion listed. Flag every violation — do not give the benefit of the doubt. If the code doesn't meet a criterion, flag it. The PR author can always push back. For each finding provide JSON: {"id": "", "file": "", "lines": "", "description": "", "recommendation": ""} @@ -144,7 +144,7 @@ Only spawn if changed files contain `*_actions.go` or `actions.go` files. This a ``` You are reviewing provisioning (Grant/Revoke) code for a Baton connector. -Only flag issues you are confident are real problems — things you would reject in a human code review. Do not flag style preferences or hypothetical concerns. +Flag every violation — do not give the benefit of the doubt. If the code doesn't meet a criterion, flag it. The PR author can always push back. CRITICAL CONTEXT — Entity Source Rules (caused 3 production reverts): - WHO (user/account ID): principal.Id.Resource @@ -181,7 +181,7 @@ Only spawn if changed files contain config or dependency files. Use `model: "hai **Prompt template:** ``` -Review these connector config/dependency changes. Only flag issues you are confident are real problems. +Review these connector config/dependency changes. Flag every violation — do not give the benefit of the doubt. Config criteria (G1-G4): - G1: conf.gen.go must NEVER be manually edited @@ -247,9 +247,9 @@ DIFFS: 1. Parse JSON arrays from code review agents (Agents 1-3). 2. Deduplicate: same file + line range → keep one. -3. **Cross-validate entity sources** (if provisioning changed): Read the Grant/Revoke code yourself to verify P1/P2 findings. This is the #1 bug. -4. **Cross-validate PR feedback**: Check PR review comments against findings. Add any unaddressed items from human reviewers. -5. Drop findings that are fully mitigated by a config flag or feature gate. +3. **Validate each finding yourself.** You are the final judge — sub-agents can be wrong. Read the relevant code and drop any finding that is incorrect or not a real issue. +4. **Cross-validate entity sources** (if provisioning changed): Read the Grant/Revoke code yourself to verify P1/P2 findings. This is the #1 bug. +5. **Cross-validate PR feedback**: Check PR review comments against findings. Add any unaddressed items from human reviewers. 6. Parse the docs-reviewer (Agent 4) result. If status is "stale", include each finding — stale docs are a release blocker. **Deliverable:** A merged list of all findings with duplicates removed. Print the count. @@ -269,6 +269,9 @@ Post findings directly as PR comments: **Issues: N** +### Breaking Changes + + ### Issues