diff --git a/.changes/review/feat-review-hold-criteria-fce127db.md b/.changes/review/feat-review-hold-criteria-fce127db.md new file mode 100644 index 0000000000..ae6d114da6 --- /dev/null +++ b/.changes/review/feat-review-hold-criteria-fce127db.md @@ -0,0 +1,7 @@ +--- +bump: minor +--- + +### Added + +- `/review:code-review` and `/review:security-review` return the review's optional `hold` field, when the wrapper's output schema has one, only for the hold classes: `cost` (a change that adds or raises model or API spend, code lane), `trust-gate` (a change to the trusted-actors list, the trusted-trigger action, a `CODEOWNERS` or a `.gitmodules` file) and `critical-security` (a CRITICAL security finding; the code lane only where no security lane runs). A hold adds nothing to the findings count, and every other flag stays a normal finding. A change in a hold class is no longer eligible for the code lane's trivial-change skip, nor a `trust-gate` change for the security lane's no-security-surface skip. diff --git a/plugins/review/skills/code-review/SKILL.md b/plugins/review/skills/code-review/SKILL.md index 8441307d92..2aa078c4f0 100644 --- a/plugins/review/skills/code-review/SKILL.md +++ b/plugins/review/skills/code-review/SKILL.md @@ -80,7 +80,8 @@ post nothing else): 1. PR is closed or not open 2. PR is a draft -3. Change is trivial/automated with no meaningful review surface +3. Change is trivial/automated with no meaningful review surface; a change in a + hold class ("Reporting") never is 4. This head already has a successful review from this lane that still applies ## Criteria @@ -214,3 +215,25 @@ Follow the reporting mechanics the workflow wrapper appended to the prompt (inline MCP tool, `gh`, or the output schema it supplies). Keep the summary comment for overview and cross-file / whole-PR findings; use commit-blob permalinks with the supplied HEAD SHA for locations no changed line can carry. + +**Hold for a person.** When the wrapper's output schema has a `hold` field, fill it as that +schema defines it, only for these classes, each named by its criterion, with a reason naming the +files and what they change: + +- `cost`: the change adds or raises model or API spend: a new model call or agent run, a raised + model, effort, turn or timeout limit on one, a new schedule or trigger that starts one, a runner + that is not free, or a resource added or resized in infrastructure as code (Pulumi, Terraform, + Bicep). It is the class `/source-control:babysit-prs` rates `cost-review-required`. +- `trust-gate`: the change touches `.github/standards/trusted-actors/**`, + `.github/actions/check-trusted-trigger/**`, a `CODEOWNERS` file or a `.gitmodules` file. +- `critical-security`: a security finding of CRITICAL severity, only where no security lane runs + ("Gotchas"). + +A hold is not a finding. `cost` and `trust-gate` belong to the change, so return them with zero +findings too, and no hold adds to the findings count. Every other flag, a `manifest : flag` +line and every blocking or advisory finding included, is a normal finding and never sets `hold`. +With no `hold` field in the schema, return none. Pointer: the field's shape is the output schema +in the compose step of +[ci-workflows `pr-review-cli.yml`](https://github.com/melodic-software/ci-workflows/blob/v0.64.0/.github/workflows/pr-review-cli.yml#L861-L884). +As of: 2026-10-23. Recheck trigger: a ci-workflows release changes the `hold` schema or its +criteria. diff --git a/plugins/review/skills/security-review/SKILL.md b/plugins/review/skills/security-review/SKILL.md index 88be266050..5df9425811 100644 --- a/plugins/review/skills/security-review/SKILL.md +++ b/plugins/review/skills/security-review/SKILL.md @@ -107,7 +107,8 @@ nothing else): 1. PR is closed or not open 2. Change has no security-relevant surface after reading the diff. A diff that deletes, narrows, or softens a standing instruction is not eligible for this skip until it has been read under - the instruction-surface lens below: it reads as prose-only while it can remove a control + the instruction-surface lens below: it reads as prose-only while it can remove a control. A + change to a `trust-gate` path ("Reporting") is never eligible for it either 3. This head already has a successful security review that still applies ## Criteria @@ -222,3 +223,21 @@ The merge gate reads severity from two plain-text lines, never HTML comments: it reports that have no inline comment (0 when none). A comment or summary without its one readable line counts every finding it holds as CRITICAL. + +**Hold for a person.** When the wrapper's output schema has a `hold` field, fill it as that +schema defines it, only for these classes, each named by its criterion, with a reason naming the +findings or files: + +- `critical-security`: one or more CRITICAL findings. Each keeps its `severity: CRITICAL` line and + its place in `critical-findings: `; the hold adds to neither count. +- `trust-gate`: the change touches `.github/standards/trusted-actors/**`, + `.github/actions/check-trusted-trigger/**`, a `CODEOWNERS` file or a `.gitmodules` file, with or + without a finding. + +An IMPORTANT or SUGGESTION finding, a `manifest : flag` line short of CRITICAL, and every +other finding is a normal finding and never sets `hold`. With no `hold` field in the schema, +seat-run mode included, return none. Pointer: the field's shape is the output schema in the +compose step of +[ci-workflows `pr-review-cli.yml`](https://github.com/melodic-software/ci-workflows/blob/v0.64.0/.github/workflows/pr-review-cli.yml#L861-L884). +As of: 2026-10-23. Recheck trigger: a ci-workflows release changes the `hold` schema or its +criteria.