Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 18 additions & 13 deletions scripts/summary-comment.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
//
// node summary-comment.mjs <result.json> --tier cheap|strong --push <n>
// --gate pass|blocked [--prev <file with the previous comment body>]
// [--passes <n>] [--quiet] [--block-on P0,P1] [--held] [--fix-first P0,P1]
// [--passes <n>] [--quiet] [--block-on P0,P1]
//
// Prints the summary MARKDOWN to stdout. The driver (action.yml) writes it into
// a marker-delimited region of the PR DESCRIPTION body (scripts/inject-summary.mjs),
Expand Down Expand Up @@ -41,9 +41,15 @@
// a cheap pass withholding the strong review over a fix-first finding — counted
// over the fix-first set instead, because block-on need not contain those
// severities and "❌ 0 findings block merge" beside a "held" tier line
// contradicts itself. There is no held run and no tier line now, so the count
// contradicts itself. The cascade went, and the tier line with it, so the count
// and the gate read the same set, which is the property that matters: the
// summary cannot claim something the merge gate does not enforce.
//
// --held and --fix-first went with it here, having been parsed and then read by
// nothing for as long as the exception has been gone. fix-first REMAINS an action
// input — it stops the exhaustive loop early, see action.yml — and must not be
// wired back into this count on the strength of that name: the two answer
// different questions, which is what the paragraph above is about.

import fs from "node:fs";
import { SEVERITIES, countSeverities } from "./severity.mjs";
Expand All @@ -54,14 +60,13 @@ const STATE_RE = /<!-- orca-cr-state: (\{.*?\}) -->/;
const usage = () => {
console.error(
"usage: node summary-comment.mjs <result.json> --tier cheap|strong --push <n> " +
"--gate pass|blocked [--prev <file>] [--passes <n>] [--quiet] [--block-on P0,P1] " +
"[--held] [--fix-first P0,P1]",
"--gate pass|blocked [--prev <file>] [--passes <n>] [--quiet] [--block-on P0,P1]",
);
process.exit(2);
};

const [file, ...rest] = process.argv.slice(2);
const opts = { passes: "1", blockOn: "P0,P1", fixFirst: "P0,P1" };
const opts = { passes: "1", blockOn: "P0,P1" };
for (let i = 0; i < rest.length; i += 1) {
if (rest[i] === "--tier") opts.tier = rest[++i];
else if (rest[i] === "--push") opts.push = rest[++i];
Expand All @@ -70,29 +75,29 @@ for (let i = 0; i < rest.length; i += 1) {
else if (rest[i] === "--passes") opts.passes = rest[++i];
else if (rest[i] === "--quiet") opts.quiet = true;
else if (rest[i] === "--block-on") opts.blockOn = rest[++i];
else if (rest[i] === "--held") opts.held = true;
else if (rest[i] === "--fix-first") opts.fixFirst = rest[++i];
// NO else: an unrecognised flag, and the value that follows it, are skipped
// one token at a time. That is what makes removing --fix-first safe to ship
// ahead of the caller that still passes it — the pair falls through without
// shifting the flags after it.
}
const push = Number(opts.push);
const passes = Number(opts.passes);
// Severity-set normalization shared by --block-on and --fix-first: trim +
// uppercase, empty = the empty set (valid — "block on nothing" / no fix-first).
// Severity-set normalization for --block-on: trim + uppercase, empty = the
// empty set (valid — a deliberate "block on nothing").
const parseSet = (v) =>
String(v ?? "")
.split(",")
.map((s) => s.trim().toUpperCase())
.filter(Boolean);
const blockOn = parseSet(opts.blockOn);
const fixFirst = parseSet(opts.fixFirst);
if (
!file ||
!["pass", "blocked"].includes(opts.gate) ||
!Number.isInteger(push) ||
push < 1 ||
!Number.isInteger(passes) ||
passes < 1 ||
blockOn.some((s) => !SEVERITIES.includes(s)) ||
fixFirst.some((s) => !SEVERITIES.includes(s))
blockOn.some((s) => !SEVERITIES.includes(s))
) {
usage();
}
Expand Down Expand Up @@ -151,7 +156,7 @@ lines.push("");
// Counted over block-on, which is what the gate enforces. The --held variant
// (count over fix-first instead) went with the cascade: it existed so a withheld
// escalation would not render "❌ 0 findings block merge" beside a held tier line,
// and there is neither withholding nor a tier line any more.
// and there is neither withholding nor a tier line any more. See the header.
const blockingSet = blockOn;
const blocking = blockingSet.reduce((n, s) => n + counts[s], 0);
lines.push(
Expand Down
9 changes: 6 additions & 3 deletions scripts/summary-comment.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -173,12 +173,15 @@ describe("the ❌ count follows block-on", () => {
// the summary read "❌ 0 findings block merge" beside a held tier line. There is
// no withholding any more, so what survives is the case that was never about
// held runs at all.
test("the count follows --block-on, not --fix-first", () => {
test("the count follows --block-on, and a removed flag cannot steer it", () => {
// --fix-first is deliberately a DIFFERENT set here: it used to steer this
// count on a held run, and now it must not steer it at all.
// count on a held run, and now it must not steer it at all. It is no longer
// parsed, and it sits in the MIDDLE of the argv on purpose — the flag AND
// its value have to be skipped without shifting the --block-on that follows,
// which is what lets this script ship ahead of a caller still passing it.
const out = run(
["[P0] a", "[P2] b"],
["--push", "1", "--gate", "blocked", "--block-on", "P2", "--fix-first", "P0"],
["--push", "1", "--fix-first", "P0", "--gate", "blocked", "--block-on", "P2"],
);
assert.ok(out.includes("❌ 1 finding blocks merge"), `got:
${out}`);
Expand Down
6 changes: 6 additions & 0 deletions skills/orca-review-action/references/inputs.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,12 @@
Every input of `Continuum-AI-Corp/orca-code-review@v1`. Read the row before
answering a question about behavior — do not guess a default.

**These rows describe this action.** The hosted OrcaCode Review GitHub App runs a
different review engine, so a setting of the same name can behave differently
there — `fix-first` and `exhaustive` in particular. For the App, the console at
**OrcaRouter → Apps → OrcaCode Review** is the authority; do not answer a question
about the App from this file.

## Required

| Input | Default | What it does |
Expand Down
Loading