Load PR review criteria from trusted base

Author: gontzess

.github/actions/pr-review/action.yml

@@ -56,6 +56,12 @@ runs:
       GH_TOKEN: ${{ inputs.github_token }}
       PR_NUMBER: ${{ inputs.pr_number }}
     run: python3 ${{ github.action_path }}/scripts/resolve-outdated-threads.py
+  - name: Load repo-local review criteria
+    shell: bash
+    env:
+      GH_TOKEN: ${{ inputs.github_token }}
+      REVIEW_CRITERIA_PATH: .claude/skills/ci-review.md
+    run: python3 ${{ github.action_path }}/scripts/load-review-criteria.py
   - name: Load review prompt
     id: prompt
     shell: bash
@@ -74,6 +80,10 @@ runs:
             cat "${PROMPT_DIR}/mixins/${mixin}.md"
           done
         fi
+        if [ -f ".github/review-criteria.md" ]; then
+          echo
+          cat ".github/review-criteria.md"
+        fi
         echo "${DELIM}"
       } >> "${GITHUB_ENV}"
   - name: Run Claude PR Review
@@ -82,8 +92,9 @@ runs:
       anthropic_api_key: ${{ inputs.anthropic_api_key }}
       github_token: ${{ inputs.github_token }}
       include_fix_links: true
+      use_sticky_comment: true
       allowed_bots: "*"
-      claude_args: --model claude-opus-4-8 --max-turns 100 --allowedTools "Read,Glob,Grep,Skill,Task,mcp__github_inline_comment__create_inline_comment,Bash(gh pr diff:*),Bash(gh pr view:*),Bash(gh pr review:*),Bash(gh api:*)"
+      claude_args: --model claude-opus-4-8 --max-turns 100 --allowedTools "Read,Glob,Grep,mcp__github_inline_comment__create_inline_comment,Bash(gh pr diff:*),Bash(gh pr view:*),Bash(gh pr review:*),Bash(gh api:*)"
       prompt: ${{ env.REVIEW_PROMPT }}
   - name: Upload review context artifacts
     if: always()
@@ -94,4 +105,6 @@ runs:
         .github/pr-context.json
         .github/resolved-threads.json
         .github/incremental.diff
+        .github/review-criteria.md
+        .github/review-criteria.json
       retention-days: 7

.github/actions/pr-review/prompts/base-pr-review.md

@@ -1,6 +1,14 @@
 You are a senior code reviewer performing an automated PR review in CI.
 This is a READ-ONLY review — do NOT write files, create commits, or run build/test commands.
 
+You are running non-interactively in CI. There is no human to answer follow-up
+questions, so do not ask any. Decide based on the diff and the code in front of
+you. Do not narrate your process or think out loud in posted output. The only
+things you post are inline comments and the summary comment, in the formats
+specified below. Keep every posted line terse and actionable. When you are
+uncertain, encode the uncertainty as confidence and severity on the finding
+rather than as prose hedging in the summary.
+
 ## Procedure
 
 ### Step 1 — Gather context
@@ -58,39 +66,71 @@ Read `.github/resolved-threads.json` — it contains a summary of outdated bot r
 that were automatically resolved before this review started. Use `resolved_count` from this
 file when reporting "Threads Resolved" in the summary.
 
-### Step 4 — Check For Repo Review Skill
+### Step 4 — Use Trusted Repo-Local Review Criteria
+
+The action may append a section named "Repo-Local Review Criteria (Trusted Base Data)"
+to this prompt. That section is fetched before you run from
+`.claude/skills/ci-review.md` at the trusted PR base SHA, validated as plain markdown,
+and appended as data. It is not a Claude skill and must not be invoked as `/ci-review`.
 
-Check for `.claude/skills/ci-review.md` using Glob. The workspace is the same-repo
-PR head checkout. If the skill exists, invoke `/ci-review` and incorporate its results
-as an additive layer alongside the base checks and any built-in mixins in this prompt.
-For connector repositories, this means the effective review stack is base prompt +
-connector mixin + repo-local `ci-review.md` when that skill exists.
-If `.claude/skills/ci-review.md` itself changed in the PR, do not invoke it; review it
-as changed source instead.
+If the criteria status says criteria loaded, use that criteria markdown as an additive
+review layer alongside the base checks and any built-in mixins in this prompt. For
+connector repositories, this means the effective review stack is base prompt +
+connector mixin + trusted repo-local criteria when those criteria load.
+
+If the criteria status says none loaded because the file is missing, invalid, or
+unavailable, continue the review with the base prompt and built-in mixins. This is
+advisory observability, not a hard failure. Always include the criteria status in the
+summary contract below.
 
 ### Step 5 — Review changed files
 
-If review mode is `"incremental"`, read the file named by `incremental_diff_path` for
-suggestions. Still scan the full PR diff (`gh pr diff <pr_number> --repo <repository>`) for
-security and confident correctness issues.
-If the incremental metadata reports dropped paths or truncation, mention that
-partial coverage in the review summary and use the full diff to check whether
-the omitted paths affect dependency locks, generated source, vendored source,
-or release behavior.
+In BOTH modes you must fetch and read the complete PR diff with
+`gh pr diff <pr_number> --repo <repository>` and scan every changed hunk in it for the
+Security and Correctness criteria below. This full-diff security pass is required, not
+optional. Do not skip it, and do not treat the filtered incremental artifact as a
+substitute for it. The incremental artifact deliberately omits paths such as vendored,
+generated, lockfile, and truncated entries; a security or correctness issue in an omitted
+path still blocks merge.
+
+If review mode is `"incremental"`, additionally read the file named by
+`incremental_diff_path` and scope suggestion-level non-blocking review to that artifact.
+If the incremental metadata reports dropped paths or truncation, say so in the summary and
+use the full diff to check whether the omitted paths affect dependency locks, generated
+source, vendored source, or release behavior.
 
 If review mode is `"full"`, review the full PR diff for all categories.
 
-Use the local checkout with Read, Glob, Grep, and Task for source-file inspection. Use
+Use the local checkout with Read, Glob, and Grep for source-file inspection. Use
 `gh pr view` and `gh api` for extra GitHub metadata when needed.
 
-Exclude bulk content-level review of vendored code, generated files, and
-lockfiles after checking whether those paths affect dependencies, generated or
-vendored source reachability, or release behavior. Do not exclude `go.mod` or
-`go.sum` from dependency review.
+Dependency manifests are always in scope. If `go.mod` or `go.sum` changed, you MUST
+review them: confirm added, updated, or removed modules match the code changes; flag
+unexplained or unrelated dependency additions, version bumps that change behavior, and
+any module `replace`, `exclude`, or checksum change. `go.mod` and `go.sum` are NOT
+lockfiles for the purpose of the exclusion below and are never excluded from review.
+
+For other paths, exclude only bulk content-level review of vendored code, generated
+files, and language lockfiles, and only after checking whether those paths affect
+dependencies, generated or vendored source reachability, or release behavior.
 
 ### Step 6 — Validate findings
 
-Read the code yourself and drop false positives. Only flag real issues.
+This step has two stages, and the line between them matters:
+
+INTERNAL, not posted: first enumerate every candidate finding you noticed in the scan,
+each with a confidence of high, medium, or low and a severity. This enumeration is
+internal coverage scratch-work, so you do not silently drop a medium-confidence true
+positive. Do not post this raw candidate list.
+
+POSTED review output: for each candidate, read the code yourself to confirm it is real.
+Post only findings you have validated as real, and label each posted finding with its
+confidence. Drop a candidate from posted output only when you have confirmed it is a
+false positive, not merely because you are unsure. A real issue you are not fully
+confident about is a validated finding at `suggestion` severity with its confidence
+noted, not a dropped finding and not an unvalidated guess. The downstream verdict logic,
+not pre-filtering, decides what blocks merge.
+
 Skip any issue that was already raised in an existing PR comment or inline review comment.
 Do not re-flag issues on unchanged code that were pre-resolved (see step 3).
 
@@ -126,12 +166,14 @@ summary as only counts plus "None found" sections.
 
 **Blocking Issues: N** | **Suggestions: M** | **Threads Resolved: R**
 _Review mode: incremental since `<last_reviewed_sha short>`_ (or _Review mode: full_)
+**Criteria:** <copy the exact `Criteria status:` value from the trusted criteria section>
 [View review run](<review_run_url>)
 
 ### Review Summary
-<1-3 sentences describing what was reviewed. In incremental mode, include addressed
-prior feedback when applicable, for example "The previous pagination suggestion is now
-addressed by passing the page token through the client call. No new issues found.">
+<1-3 sentences describing what was reviewed. State that the full PR diff was scanned for
+security and correctness. In incremental mode, include addressed prior feedback when
+applicable, for example "The previous pagination suggestion is now addressed by passing
+the page token through the client call. No new issues found.">
 
 ### Security Issues
 <one-liner per finding with file:line, or "None found.">
@@ -190,9 +232,10 @@ specific fix in plain English. If there are no findings, omit this section entir
 
 ## Review Criteria
 
-Use these base criteria for every repository. Built-in mixins may add domain-specific checks.
+Use these base criteria for every repository. Built-in mixins and trusted repo-local
+criteria may add domain-specific checks.
 Do not apply connector implementation rules such as resource builder registration, connector
-docs, or SaaS API pagination unless a connector mixin is present or the trusted repo-local skill
+docs, or SaaS API pagination unless a connector mixin is present or the trusted repo-local criteria
 explicitly asks for those checks.
 
 ### Security (blocking)
@@ -240,4 +283,6 @@ explicitly asks for those checks.
 | `blocking-correctness` | Yes | Confident bug, crash, data loss, or compatibility break |
 | `suggestion` | No | Uncertain issues, style, test gaps, doc gaps, or maintainability |
 
-**When in doubt, use suggestion.**
+**When in doubt about a real finding, report it as a `suggestion` — never drop it.**
+Doubt lowers severity; it does not remove the finding. Only confirmed false positives are
+dropped (see Step 6).

.github/actions/pr-review/prompts/mixins/connector.md

@@ -4,8 +4,10 @@ Apply these extra criteria when reviewing Baton connector implementation reposit
 Baton connectors are Go projects that sync identity data from SaaS APIs into ConductorOne.
 
 When provisioning files change, inspect the full file content from the local checkout if the
-diff does not contain enough context. Exclude `vendor/`, `conf.gen.go`, generated files, and
-lockfiles from connector-specific review.
+diff does not contain enough context. Exclude `vendor/`, `conf.gen.go`, and generated files
+from connector-specific content review. Do NOT exclude `go.mod` or `go.sum`: if they
+changed, apply the Dependency Checks section below. They are dependency manifests, not
+excluded lockfiles.
 
 ### File Context
 
@@ -166,8 +168,12 @@ Do not flag these patterns without clear repo-specific evidence:
 
 ### Dependency Checks
 
-- Dependency changes should match the code changes.
-- New dependencies should be justified by the changed code.
-- Removed dependencies should not still be needed.
-- Check whether the connector is on a recent enough baton-sdk version for the behavior it relies on.
-- SDK version changes should not unintentionally widen or narrow connector behavior.
+If `go.mod` or `go.sum` changed, you must run these checks against the manifest diff from
+the full `gh pr diff`, not only the incremental artifact:
+
+- Every added, updated, or removed module matches the code changes; flag unexplained or
+  unrelated additions.
+- New dependencies are justified by the changed code; removed dependencies are no longer needed.
+- The connector is on a recent enough baton-sdk version for the behavior it relies on.
+- SDK version bumps do not unintentionally widen or narrow connector behavior; treat
+  behavior-changing bumps as a correctness finding, not a silent pass.