From 9f75cdbf351bdff2697aaeadbe51d24fc2032d43 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 01:22:20 -0400 Subject: [PATCH 1/2] feat(review): return a hold only for the cost, trust-gate and critical-security classes The review lanes' structured output gained an optional hold field. Both review skills now fill it only for the owner's hold classes: cost (code lane), trust-gate, and critical-security (CRITICAL only; 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. Co-Authored-By: Claude Opus 5.5 --- .../feat-review-hold-criteria-fce127db.md | 7 ++++++ plugins/review/skills/code-review/SKILL.md | 25 ++++++++++++++++++- .../review/skills/security-review/SKILL.md | 18 +++++++++++++ 3 files changed, 49 insertions(+), 1 deletion(-) create mode 100644 .changes/review/feat-review-hold-criteria-fce127db.md 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..fb9b3596e6 --- /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. 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..a129c600c4 100644 --- a/plugins/review/skills/security-review/SKILL.md +++ b/plugins/review/skills/security-review/SKILL.md @@ -222,3 +222,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. From a0a06fde3b0786aad5831a273aa18250cf55b323 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 01:41:24 -0400 Subject: [PATCH 2/2] fix(review): keep a trust-gate change out of the security lane's no-surface skip Co-Authored-By: Claude Opus 5.5 --- .changes/review/feat-review-hold-criteria-fce127db.md | 2 +- plugins/review/skills/security-review/SKILL.md | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/.changes/review/feat-review-hold-criteria-fce127db.md b/.changes/review/feat-review-hold-criteria-fce127db.md index fb9b3596e6..ae6d114da6 100644 --- a/.changes/review/feat-review-hold-criteria-fce127db.md +++ b/.changes/review/feat-review-hold-criteria-fce127db.md @@ -4,4 +4,4 @@ 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. +- `/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/security-review/SKILL.md b/plugins/review/skills/security-review/SKILL.md index a129c600c4..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