diff --git a/scripts/summary-comment.mjs b/scripts/summary-comment.mjs index 5c0e4ba..f8faff6 100644 --- a/scripts/summary-comment.mjs +++ b/scripts/summary-comment.mjs @@ -3,7 +3,7 @@ // // node summary-comment.mjs --tier cheap|strong --push // --gate pass|blocked [--prev ] -// [--passes ] [--quiet] [--block-on P0,P1] [--held] [--fix-first P0,P1] +// [--passes ] [--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), @@ -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"; @@ -54,14 +60,13 @@ const STATE_RE = //; const usage = () => { console.error( "usage: node summary-comment.mjs --tier cheap|strong --push " + - "--gate pass|blocked [--prev ] [--passes ] [--quiet] [--block-on P0,P1] " + - "[--held] [--fix-first P0,P1]", + "--gate pass|blocked [--prev ] [--passes ] [--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]; @@ -70,20 +75,21 @@ 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) || @@ -91,8 +97,7 @@ if ( 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(); } @@ -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( diff --git a/scripts/summary-comment.test.mjs b/scripts/summary-comment.test.mjs index 7cac1d6..305940e 100644 --- a/scripts/summary-comment.test.mjs +++ b/scripts/summary-comment.test.mjs @@ -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}`); diff --git a/skills/orca-review-action/references/inputs.md b/skills/orca-review-action/references/inputs.md index 8463cb4..fb52b59 100644 --- a/skills/orca-review-action/references/inputs.md +++ b/skills/orca-review-action/references/inputs.md @@ -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 |