diff --git a/.changeset/hold-for-human-dead-core-lenses.md b/.changeset/hold-for-human-dead-core-lenses.md new file mode 100644 index 00000000..b3f059d1 --- /dev/null +++ b/.changeset/hold-for-human-dead-core-lenses.md @@ -0,0 +1,19 @@ +--- +"review": minor +--- + +A run whose core review pass (`correctness-reviewer` or `skill-auditor`) +produced no output no longer auto-approves: the plan CLI now feeds the +dispatcher's real `skippedDimensions` into `computeVerdict`, whose +HOLD_FOR_HUMAN gate was previously unreachable (the dimensions were hardcoded +"assessed"). A hold submits no review event; the plan's body posts as one +standalone PR comment explaining the hold and how to get unstuck, the +conformance gate blocks any other shape (a queued review event, inline +comments, thread resolutions, or a withheld hold comment), and the cache +writer leaves the prior fingerprints standing so the next run reviews in +full (it drops only `risksPatternsKey`, because posting the hold comment +collapses the standing guidance comment). Blocking findings still win: with +a validated blocking claim the verdict stays REQUEST_CHANGES and the dead +lens is disclosed in a note line. The production shape this closes: +Khan/actions#328's re-run, where every core lens died on an API auth error +and the bot still submitted "Approved" over seven "not assessed" notes. diff --git a/workflows/review/lib/cache-record.test.ts b/workflows/review/lib/cache-record.test.ts index d71ebccc..16fb6573 100644 --- a/workflows/review/lib/cache-record.test.ts +++ b/workflows/review/lib/cache-record.test.ts @@ -404,3 +404,90 @@ describe("the in-run safe-output queue (GH_AW_SAFE_OUTPUTS)", () => { expect(record.risksPatternsKey).toBe("prior-key"); }); }); + +describe("the hold record (HOLD_FOR_HUMAN plans)", () => { + const holdStaged = ( + over: Record = {}, + ): Record => ({ + ...staged(), + [`${REVIEW}/submission-plan.json`]: JSON.stringify({ + event: "HOLD_FOR_HUMAN", + comments: [], + }), + [QUEUE]: JSON.stringify({ + items: [{type: "add_comment", body: "Holding for human review."}], + }), + ...over, + }); + + const priorRecord = JSON.stringify({ + timestamp: "2026-07-01T00:00:00.000Z", + verdict: "APPROVE", + diffFingerprint: {"a.ts": "prior-sha"}, + reviewedHunks: {"a.ts": ["prior-hunk"]}, + risksPatternsKey: "risk:a.ts:t1", + requestedTeams: ["t1"], + }); + + it("never records fingerprints for a hold (the next run reviews in full)", () => { + const fs = makeFakeFs(holdStaged()); + const result = runCacheRecordCli(fs, NOW); + // No prior record: nothing to update, nothing written \u2014 the staged + // (current) fingerprints must NOT land, or the next run would scope + // its re-review against hunks nobody reviewed. + expect(result.written).toBe(false); + expect(fs.files[`${CACHE}/pr-41.json`]).toBeUndefined(); + }); + + it("drops risksPatternsKey from the prior record when the hold comment queued", () => { + const fs = makeFakeFs( + holdStaged({[`${CACHE}/pr-41.json`]: priorRecord}), + ); + const result = runCacheRecordCli(fs, NOW); + expect(result.written).toBe(true); + const record = JSON.parse(fs.files[`${CACHE}/pr-41.json`] as string); + // The key is gone (the hold comment collapsed the standing guidance + // comment, so the next approving run must repost it) \u2026 + expect(record.risksPatternsKey).toBeUndefined(); + // \u2026 and everything else carries verbatim: fingerprints, verdict, + // and supplements stay the prior run's, never this run's staged + // facts. + expect(record.verdict).toBe("APPROVE"); + expect(record.diffFingerprint).toEqual({"a.ts": "prior-sha"}); + expect(record.reviewedHunks).toEqual({"a.ts": ["prior-hunk"]}); + expect(record.requestedTeams).toEqual(["t1"]); + }); + + it("leaves the prior record untouched when no hold comment queued", () => { + const fs = makeFakeFs( + holdStaged({ + [`${CACHE}/pr-41.json`]: priorRecord, + [QUEUE]: JSON.stringify({items: []}), + }), + ); + const result = runCacheRecordCli(fs, NOW); + expect(result.written).toBe(false); + expect(fs.files[`${CACHE}/pr-41.json`]).toBe(priorRecord); + }); + + it("refuses loudly on an unreadable queue (nothing corroborates the hold comment)", () => { + const files = holdStaged({[`${CACHE}/pr-41.json`]: priorRecord}); + delete files[QUEUE]; + const fs = makeFakeFs(files); + const result = runCacheRecordCli(fs, NOW); + expect(result.written).toBe(false); + expect(result.warn).toBe(true); + expect(fs.files[`${CACHE}/pr-41.json`]).toBe(priorRecord); + }); + + it("skips when the prior record carries no risksPatternsKey", () => { + const prior = JSON.stringify({ + verdict: "APPROVE", + diffFingerprint: {"a.ts": "prior-sha"}, + }); + const fs = makeFakeFs(holdStaged({[`${CACHE}/pr-41.json`]: prior})); + const result = runCacheRecordCli(fs, NOW); + expect(result.written).toBe(false); + expect(fs.files[`${CACHE}/pr-41.json`]).toBe(prior); + }); +}); diff --git a/workflows/review/lib/cache-record.ts b/workflows/review/lib/cache-record.ts index 881e50c6..a05a96a8 100644 --- a/workflows/review/lib/cache-record.ts +++ b/workflows/review/lib/cache-record.ts @@ -273,6 +273,58 @@ export const runCacheRecordCli = ( "the dispatch-conformance gate blocked this run: nothing posted, the prior record stands", ); } + if (plan.event === "HOLD_FOR_HUMAN") { + // A hold reviewed nothing (its core lenses produced no output), so + // the fingerprints/verdict of the prior record stand untouched and + // the next run reviews in full. ONE field does change: posting the + // hold comment made the engine's hide-older-comments collapse the + // standing risks/patterns guidance comment, and the next approving + // run would read the unchanged `risksPatternsKey` as "guidance + // already posted" and never restore it. Dropping the key here makes + // that run repost the guidance. Everything else carries verbatim. + const {items: holdItems, readable: holdReadable} = readQueue( + fs, + queuePath, + ); + if (!holdReadable) { + // Mirror the review-event path's corroboration refusal: an + // unreadable queue means nothing proves the hold comment + // posted, and a silent skip here would leave a stale + // risksPatternsKey failure invisible. + return refuse( + "hold plan but no readable safe-output queue, so nothing corroborates the hold comment: the prior record stands", + ); + } + const holdCommentQueued = holdItems.some( + (item) => item["type"] === "add_comment", + ); + if (!holdCommentQueued) { + return skip( + "hold plan with no queued hold comment: the prior record stands untouched", + ); + } + const holdPr = readJson(fs, `${REVIEW_DIR}/pr-context.json`) as + | {number?: unknown} + | undefined; + if (typeof holdPr?.number !== "number") { + return skip("hold plan and no pr-context: nothing to update"); + } + const holdRecordPath = `${CACHE_MEMORY_DIR}/pr-${holdPr.number}.json`; + const holdPrior = readJson(fs, holdRecordPath); + if (!isRecord(holdPrior) || !("risksPatternsKey" in holdPrior)) { + return skip( + "hold plan: no prior record or no risksPatternsKey to drop; the prior record stands", + ); + } + const {risksPatternsKey: _, ...holdKept} = holdPrior; + fs.mkdirSync(CACHE_MEMORY_DIR, {recursive: true}); + fs.writeFileSync(holdRecordPath, JSON.stringify(holdKept, null, 2)); + return { + written: true, + reason: `hold: dropped risksPatternsKey from ${holdRecordPath} (the hold comment collapsed the standing guidance comment); fingerprints untouched`, + record: holdKept, + }; + } if (plan.event !== "APPROVE" && plan.event !== "REQUEST_CHANGES") { return refuse("the staged plan carries no submittable event"); } diff --git a/workflows/review/lib/dispatch-gate-plan.ts b/workflows/review/lib/dispatch-gate-plan.ts new file mode 100644 index 00000000..2b41ad38 --- /dev/null +++ b/workflows/review/lib/dispatch-gate-plan.ts @@ -0,0 +1,255 @@ +/** + * Rule 7 of the dispatch-conformance gate (dispatch-gate.ts): when a + * submission plan is staged (`submission-plan.json`, scripted mode, slice + * 4), the queued safe outputs must match it. Split out of dispatch-gate.ts + * when the HOLD_FOR_HUMAN shape took that file over the max-lines cap; the + * rules are unchanged and dispatch-gate.ts is the only production caller. + * + * The shapes enforced: + * + * - A review-event plan (APPROVE / REQUEST_CHANGES): the queued event, + * body, and inline comments must match the plan under a + * sanitizer-tolerant normalization (`normalizeBody`, + * sanitizer-normalize.ts, which documents every absorbed transform); + * anything beyond that is a splice (#244) and blocks. The rule also + * owns the NO-submission shapes: queued comments with no submit would + * land as an ungated COMMENT review, and a silently-dropped plan would + * withhold a REQUEST_CHANGES verdict (or a disclosure), so only an + * APPROVE plan with no comments and a bare approve body may + * legitimately queue nothing (the Step 6 redundant-approval skip). + * - A HOLD_FOR_HUMAN plan (submission.ts's hold path: a core review pass + * produced no output on a would-be approval) is the inverse shape: no + * review submission, no inline comments, no thread resolutions, and + * exactly the plan's body queued as one standalone PR comment. A + * queued review submission, inline comment, or thread resolution, or + * a withheld hold comment, blocks (other safe-output kinds are outside + * this rule; their own frontmatter caps and rules govern them) — the + * production failure the hold exists for + * (Khan/actions#328's re-run) was precisely an APPROVE submitted over + * dead core lenses, so the gate must make "hold plan but approval + * posted anyway" a red run. + * + * Determinism boundary: a pure function of the queued items and the staged + * plan; no model call, no clock, no prose about the code under review. + */ + +import type {DispatchGateViolation, SafeOutputItem} from "./dispatch-gate"; +import {renderReviewBody} from "./render-comment"; +import {normalizeBody} from "./sanitizer-normalize"; + +const COMMENT_TYPE = "create_pull_request_review_comment"; +const RESOLVE_TYPE = "resolve_pull_request_review_thread"; +const ADD_COMMENT_TYPE = "add_comment"; + +export type SubmissionPlanViolationsInput = { + /** The validated safe-output queue (`agent_output.json` `items`). */ + items: SafeOutputItem[]; + /** Parsed `submission-plan.json` (unknown: agent-writable staging). */ + submissionPlan: unknown; + /** The queued review submission item, when one exists. */ + submit: SafeOutputItem | undefined; + /** The queued verdict event; null when no review submission is queued. */ + verdictEvent: string | null; + /** The queued review body ("" when no submission is queued). */ + body: string; + /** Queued inline review comment count. */ + commentCount: number; +}; + +/** Evaluate rule 7; returns [] when no plan is staged or nothing deviates. */ +export const submissionPlanViolations = ( + input: SubmissionPlanViolationsInput, +): DispatchGateViolation[] => { + const violations: DispatchGateViolation[] = []; + const {items, submit, verdictEvent, body, commentCount} = input; + + // Defensive over agent-writable staged input, like every sibling parse + // in the gate. `readJsonIfPresent` passes `JSON.parse("null")` straight + // through, so a `submission-plan.json` containing literal `null` reaches + // here as `null` — which a bare `!== undefined` guard admits, and the + // property reads below then throw. That throw escapes to the CLI entry + // catch, which exits 0 with the queue untouched: not a rule-7 failure but + // a fail-open of ALL SEVEN rules. The sibling `priorReviews` parse was + // hardened against exactly this shape; this one has to match it. + const planStaged = + typeof input.submissionPlan === "object" && + input.submissionPlan !== null + ? (input.submissionPlan as { + event?: unknown; + body?: unknown; + comments?: unknown; + skipSubmission?: unknown; + }) + : undefined; + const planIsHold = + planStaged !== undefined && planStaged.event === "HOLD_FOR_HUMAN"; + if (planStaged !== undefined && planIsHold) { + // The hold shape (submission.ts's HOLD_FOR_HUMAN): the plan's body + // posts as ONE standalone PR comment and nothing else queues. Each + // deviation is its own violation so the report names everything + // wrong at once. + const planBody = + typeof planStaged.body === "string" ? planStaged.body : ""; + if (submit !== undefined) { + violations.push({ + code: "submission-plan-mismatch", + dimension: "verdict", + detail: + `a ${ + verdictEvent || "(no event)" + } review submission is queued but the ` + + "staged plan is HOLD_FOR_HUMAN (a hold submits no review)", + }); + } + if (commentCount > 0) { + violations.push({ + code: "submission-plan-mismatch", + dimension: "inline comments", + detail: `${commentCount} inline comment(s) queued but the staged HOLD_FOR_HUMAN plan posts none`, + }); + } + const queuedResolves = items.filter( + (item) => item.type === RESOLVE_TYPE, + ).length; + if (queuedResolves > 0) { + violations.push({ + code: "submission-plan-mismatch", + dimension: "thread resolutions", + detail: `${queuedResolves} thread resolution(s) queued but a HOLD_FOR_HUMAN plan withholds resolutions (a partial run leaves existing threads standing)`, + }); + } + const holdCommentQueued = items.some( + (item) => + item.type === ADD_COMMENT_TYPE && + normalizeBody( + typeof item.body === "string" ? item.body : "", + ) === normalizeBody(planBody), + ); + if (!holdCommentQueued) { + violations.push({ + code: "submission-plan-mismatch", + dimension: "hold comment", + detail: + "the staged plan is HOLD_FOR_HUMAN but no queued PR comment matches its body " + + "(normalized comparison); the hold disclosure would be withheld", + }); + } + } + if (planStaged !== undefined && !planIsHold && submit === undefined) { + const planComments = Array.isArray(planStaged.comments) + ? planStaged.comments + : []; + if (commentCount > 0) { + violations.push({ + code: "submission-plan-mismatch", + dimension: "verdict", + detail: `${commentCount} inline comment(s) queued with no review submission (they would land as an ungated COMMENT review); the staged plan requires a ${String( + planStaged.event, + )} submission`, + }); + } else { + // review.md's redundant-approval skip is narrower than "APPROVE + // with no comments": the body must ALSO carry no `Note:` lines + // and no accountability section, i.e. it is exactly the bare + // comment-less-approve line (the fingerprint stamp is an HTML + // comment, which normalizeBody already drops). Without the body + // check, an APPROVE that shed a lens (carrying a mandatory + // "Note: not assessed this run" disclosure) could be + // dropped on the floor and still pass the gate green, silently + // withholding both the disclosure and the approval. + const bareApprove = normalizeBody( + renderReviewBody({event: "APPROVE", hasInlineComments: false}), + ); + const planBody = + typeof planStaged.body === "string" ? planStaged.body : ""; + // The plan CLI owns this predicate (`skipSubmission`) so the + // prompt and this gate cannot describe the skip differently — + // they diverged once, over the collapsed low-confidence section + // riding the body. Fall back to deriving it only for a plan + // staged before the field existed. + const planSkips = + typeof planStaged.skipSubmission === "boolean" + ? planStaged.skipSubmission + : planStaged.event === "APPROVE" && + planComments.length === 0 && + normalizeBody(planBody) === bareApprove; + if (!planSkips) { + violations.push({ + code: "submission-plan-mismatch", + dimension: "verdict", + detail: `nothing queued but the staged plan is ${String( + planStaged.event, + )} with ${ + planComments.length + } comment(s); only an APPROVE plan with no comments and a bare "${renderReviewBody( + {event: "APPROVE", hasInlineComments: false}, + )}" body may skip the submission`, + }); + } + } + } + if (planStaged !== undefined && !planIsHold && submit !== undefined) { + if ( + typeof planStaged.event === "string" && + verdictEvent !== planStaged.event + ) { + violations.push({ + code: "submission-plan-mismatch", + dimension: "verdict", + detail: `queued event ${ + verdictEvent || "(none)" + } does not match the staged submission plan's ${ + planStaged.event + }`, + }); + } + if ( + typeof planStaged.body === "string" && + normalizeBody(body) !== normalizeBody(planStaged.body) + ) { + violations.push({ + code: "submission-plan-mismatch", + dimension: "review body", + detail: "queued review body does not match the staged submission plan (normalized comparison)", + }); + } + if (Array.isArray(planStaged.comments)) { + const planned = planStaged.comments + .filter( + ( + comment, + ): comment is {path: string; line: number; body: string} => + typeof (comment as {path?: unknown}).path === + "string" && + typeof (comment as {body?: unknown}).body === "string", + ) + .map( + (comment) => + `${comment.path}:${comment.line}:${normalizeBody( + comment.body, + )}`, + ) + .sort(); + const queued = items + .filter((item) => item.type === COMMENT_TYPE) + .map( + (item) => + `${ + typeof item["path"] === "string" ? item["path"] : "" + }:${String(item["line"] ?? "")}:${normalizeBody( + typeof item.body === "string" ? item.body : "", + )}`, + ) + .sort(); + if (JSON.stringify(planned) !== JSON.stringify(queued)) { + violations.push({ + code: "submission-plan-mismatch", + dimension: "inline comments", + detail: `queued inline comments (${queued.length}) do not match the staged submission plan (${planned.length})`, + }); + } + } + } + + return violations; +}; diff --git a/workflows/review/lib/dispatch-gate.ts b/workflows/review/lib/dispatch-gate.ts index a7e15b1e..87330c9c 100644 --- a/workflows/review/lib/dispatch-gate.ts +++ b/workflows/review/lib/dispatch-gate.ts @@ -54,6 +54,9 @@ * mode, slice 4), the queued event, body, and inline comments must * match it under a sanitizer-tolerant normalization; any splice or * omission blocks (the #244 accountability-splice check, as code). + * Owned by dispatch-gate-plan.ts (split out at the max-lines cap), + * including the HOLD_FOR_HUMAN shape: no review submission and the + * plan's body queued as one standalone PR comment. * * Violation behavior: strip every posting/mutating item from the queue * (keeping the diagnostics and the `out/` artifact upload so the evidence @@ -76,10 +79,10 @@ import {extractJsonValue} from "./agent-json"; import {forwardedRunWarnings} from "./forwarded-warnings"; -import {isBlockingLabel, renderReviewBody} from "./render-comment"; +import {submissionPlanViolations} from "./dispatch-gate-plan"; +import {isBlockingLabel} from "./render-comment"; import {parseLeadingLabel} from "./rereview"; import {findLatestStamp, stampFromCacheMemory} from "./rereview-mode"; -import {normalizeBody} from "./sanitizer-normalize"; /* -------------------------------------------------------------------------- */ /* Types */ @@ -517,150 +520,21 @@ export const evaluateDispatchConformance = ( } } - // Rule 7 (scripted mode, slice 4): when a submission plan is staged, the - // queued outputs must match it. gh-aw's ingest sanitizer rewrites what - // the agent queued before the gate sees it, so bodies are compared under - // `normalizeBody` (sanitizer-normalize.ts, which documents every absorbed - // transform); anything beyond that is a splice (#244) and blocks. The - // rule also owns the NO-submission shapes: queued comments with no submit - // would land as a COMMENT review, and a silently-dropped plan would - // withhold a REQUEST_CHANGES verdict (or a disclosure), so only - // an APPROVE plan with no comments and a bare approve body may - // legitimately queue nothing (the Step 6 redundant-approval skip, whose - // shape the skip branch below checks in full). - // Defensive over agent-writable staged input, like every sibling parse in - // this file. `readJsonIfPresent` passes `JSON.parse("null")` straight - // through, so a `submission-plan.json` containing literal `null` reaches - // here as `null` — which a bare `!== undefined` guard admits, and the - // property reads below then throw. That throw escapes to the CLI entry - // catch, which exits 0 with the queue untouched: not a rule-7 failure but - // a fail-open of ALL SEVEN rules. The sibling `priorReviews` parse was - // hardened against exactly this shape; this one has to match it. - const planStaged = - typeof input.submissionPlan === "object" && - input.submissionPlan !== null - ? (input.submissionPlan as { - event?: unknown; - body?: unknown; - comments?: unknown; - skipSubmission?: unknown; - }) - : undefined; - if (planStaged !== undefined && submit === undefined) { - const planComments = Array.isArray(planStaged.comments) - ? planStaged.comments - : []; - if (commentCount > 0) { - violations.push({ - code: "submission-plan-mismatch", - dimension: "verdict", - detail: `${commentCount} inline comment(s) queued with no review submission (they would land as an ungated COMMENT review); the staged plan requires a ${String( - planStaged.event, - )} submission`, - }); - } else { - // review.md's redundant-approval skip is narrower than "APPROVE - // with no comments": the body must ALSO carry no `Note:` lines - // and no accountability section, i.e. it is exactly the bare - // comment-less-approve line (the fingerprint stamp is an HTML - // comment, which normalizeBody already drops). Without the body - // check, an APPROVE that shed a lens (carrying a mandatory - // "Note: not assessed this run" disclosure) could be - // dropped on the floor and still pass the gate green, silently - // withholding both the disclosure and the approval. - const bareApprove = normalizeBody( - renderReviewBody({event: "APPROVE", hasInlineComments: false}), - ); - const planBody = - typeof planStaged.body === "string" ? planStaged.body : ""; - // The plan CLI owns this predicate (`skipSubmission`) so the - // prompt and this gate cannot describe the skip differently — - // they diverged once, over the collapsed low-confidence section - // riding the body. Fall back to deriving it only for a plan - // staged before the field existed. - const planSkips = - typeof planStaged.skipSubmission === "boolean" - ? planStaged.skipSubmission - : planStaged.event === "APPROVE" && - planComments.length === 0 && - normalizeBody(planBody) === bareApprove; - if (!planSkips) { - violations.push({ - code: "submission-plan-mismatch", - dimension: "verdict", - detail: `nothing queued but the staged plan is ${String( - planStaged.event, - )} with ${ - planComments.length - } comment(s); only an APPROVE plan with no comments and a bare "${renderReviewBody( - {event: "APPROVE", hasInlineComments: false}, - )}" body may skip the submission`, - }); - } - } - } - if (planStaged !== undefined && submit !== undefined) { - if ( - typeof planStaged.event === "string" && - verdictEvent !== planStaged.event - ) { - violations.push({ - code: "submission-plan-mismatch", - dimension: "verdict", - detail: `queued event ${ - verdictEvent || "(none)" - } does not match the staged submission plan's ${ - planStaged.event - }`, - }); - } - if ( - typeof planStaged.body === "string" && - normalizeBody(body) !== normalizeBody(planStaged.body) - ) { - violations.push({ - code: "submission-plan-mismatch", - dimension: "review body", - detail: "queued review body does not match the staged submission plan (normalized comparison)", - }); - } - if (Array.isArray(planStaged.comments)) { - const planned = planStaged.comments - .filter( - ( - comment, - ): comment is {path: string; line: number; body: string} => - typeof (comment as {path?: unknown}).path === - "string" && - typeof (comment as {body?: unknown}).body === "string", - ) - .map( - (comment) => - `${comment.path}:${comment.line}:${normalizeBody( - comment.body, - )}`, - ) - .sort(); - const queued = input.items - .filter((item) => item.type === COMMENT_TYPE) - .map( - (item) => - `${ - typeof item["path"] === "string" ? item["path"] : "" - }:${String(item["line"] ?? "")}:${normalizeBody( - typeof item.body === "string" ? item.body : "", - )}`, - ) - .sort(); - if (JSON.stringify(planned) !== JSON.stringify(queued)) { - violations.push({ - code: "submission-plan-mismatch", - dimension: "inline comments", - detail: `queued inline comments (${queued.length}) do not match the staged submission plan (${planned.length})`, - }); - } - } - } + // Rule 7 (scripted mode, slice 4): the staged submission plan must match + // the queued outputs — including the HOLD_FOR_HUMAN shape, which queues a + // standalone PR comment and no review. Split into its own module + // (dispatch-gate-plan.ts) when the hold shape took this file over the + // max-lines cap; the doc and every branch live there. + violations.push( + ...submissionPlanViolations({ + items: input.items, + submissionPlan: input.submissionPlan, + submit, + verdictEvent, + body, + commentCount, + }), + ); return { conformant: violations.length === 0, diff --git a/workflows/review/lib/dispatch-roster.ts b/workflows/review/lib/dispatch-roster.ts index 4854fa82..90fb428e 100644 --- a/workflows/review/lib/dispatch-roster.ts +++ b/workflows/review/lib/dispatch-roster.ts @@ -23,6 +23,15 @@ export const DEFAULT_FINDERS = [ "skill-auditor", ] as const; +/** + * The skipped-dimension name the triage pass records (distinct from the + * `pattern-triage` agent name: note lines read "pattern triage not + * assessed"). Lives here with DEFAULT_FINDERS so the dispatcher (which + * writes the name) and the plan CLI's dimension mapping (which reads it, + * submission.ts) share one definition. + */ +export const TRIAGE_DIMENSION = "pattern triage"; + /** * The Step 3 dispatch/shed ranking, first-shed first. Fill order under the * invocation cap is this list reversed after the defaults and matched diff --git a/workflows/review/lib/dispatch.ts b/workflows/review/lib/dispatch.ts index 0d70494b..a53d4eff 100644 --- a/workflows/review/lib/dispatch.ts +++ b/workflows/review/lib/dispatch.ts @@ -70,7 +70,8 @@ import { } from "./dispatch-contracts"; import {loadAgents, type DispatchFs} from "./dispatch-agents"; -import {computeRoster, type RosterShed} from "./dispatch-roster"; +import {computeRoster, TRIAGE_DIMENSION} from "./dispatch-roster"; +import type {RosterShed} from "./dispatch-roster"; import {refusalFallbackFor} from "./refusal-fallback"; import { applyProvenanceGate, @@ -581,7 +582,7 @@ export const runDispatch = async ( // Triage unavailable: review everything (fail toward more // review), and say so. skippedDimensions.push({ - dimension: "pattern triage", + dimension: TRIAGE_DIMENSION, cause: "unavailable", }); fs.writeFileSync(`${REVIEW_DIR}/pr.diff`, diffText); @@ -903,7 +904,7 @@ export const runDispatch = async ( ? VALIDATOR : skip.dimension === "thread reconciliation" ? RECONCILER - : skip.dimension === "pattern triage" + : skip.dimension === TRIAGE_DIMENSION ? TRIAGE : skip.dimension, ), diff --git a/workflows/review/lib/render-comment.ts b/workflows/review/lib/render-comment.ts index b1ea0bea..53ec4788 100644 --- a/workflows/review/lib/render-comment.ts +++ b/workflows/review/lib/render-comment.ts @@ -243,12 +243,22 @@ export type ReviewBodyInput = { rereviewSection?: string; }; +/** + * The HOLD_FOR_HUMAN verdict head. Exported (with {@link HOLD_UNSTUCK_LINES}) + * for the submission-plan CLI, which composes the hold's standalone PR comment + * from the same fixed template text this renderer uses, so the two surfaces + * cannot drift. + */ +export const HOLD_HEAD = + "Holding for human review — the automated review could not " + + "complete safely this run."; + /** * How the author of a held PR gets unstuck. Fixed template text (code-owned, * like the skipped-dimension note): a hold must never strand the author with a * verdict and no next action. */ -const HOLD_UNSTUCK_LINES = [ +export const HOLD_UNSTUCK_LINES = [ "To get unstuck: push a new commit (or re-run the review workflow from the " + "Actions tab) to retry the failed pass, or ask a human to review this " + "PR manually. A hold means the automated review declined to approve on " + @@ -301,9 +311,7 @@ export const renderReviewBody = (input: ReviewBodyInput): string => { head = "Changes requested — see inline comments."; break; case "HOLD_FOR_HUMAN": - head = - "Holding for human review — the automated review could not " + - "complete safely this run."; + head = HOLD_HEAD; break; default: { // Exhaustiveness guard: a new VerdictEvent must add a body branch. diff --git a/workflows/review/lib/submission-hold.test.ts b/workflows/review/lib/submission-hold.test.ts new file mode 100644 index 00000000..e9e6c038 --- /dev/null +++ b/workflows/review/lib/submission-hold.test.ts @@ -0,0 +1,344 @@ +import {describe, it, expect} from "vitest"; + +import {evaluateDispatchConformance} from "./dispatch-gate"; +import {runSubmissionCli, type SubmissionFs} from "./submission"; + +/** + * The hold path (HOLD_FOR_HUMAN) tests, split from submission.test.ts at the + * max-lines cap: the plan CLI's core-dimension gate (a run whose correctness + * or skill/severity pass produced no output must not auto-approve) and the + * dispatch-conformance gate's hold shape (the hold posts as one standalone + * PR comment and nothing else). The production regression pinned here: + * Khan/actions#328's re-run (31124365377 attempt 2), where every core lens + * died on an API auth error and the run still submitted "Approved \u2014 no + * blocking issues found" over seven "not assessed" note lines. + */ + +const REVIEW = "/tmp/gh-aw/review"; + +const makeFakeFs = ( + files: Record = {}, +): SubmissionFs & {files: Record} => { + const state = {...files}; + return { + files: state, + readFileSync: (p: string) => { + if (!(p in state)) { + throw new Error(`ENOENT: ${p}`); + } + return state[p]; + }, + writeFileSync: (p: string, data: string) => { + state[p] = data; + }, + existsSync: (p: string) => + p in state || Object.keys(state).some((f) => f.startsWith(`${p}/`)), + mkdirSync: () => {}, + }; +}; + +const claim = (overrides: Record = {}) => ({ + id: "c1", + source: "correctness-reviewer", + path: "a.ts", + line: 2, + label: "issue (blocking)", + subject: "s", + discussion: "The guard was removed.", + failure_scenario: "f", + confidence: 0.9, + ...overrides, +}); + +const staged = ( + dispatchResult: Record, + extra: Record = {}, +): Record => ({ + [`${REVIEW}/dispatch-result.json`]: JSON.stringify(dispatchResult), + [`${REVIEW}/rereview-plan.json`]: JSON.stringify({ + depth: dispatchResult["depth"] ?? "full", + mode: "full", + stampAnchorDraft: false, + stampHunks: {}, + }), + ...extra, +}); + +describe("the hold path (core dimension unavailable)", () => { + /** + * The production regression this pins: Khan/actions#328's re-run + * (31124365377 attempt 2), where every core lens died on an API auth + * error ("403 Maximum consecutive cache misses exceeded"), the + * dispatcher recorded them all in `skippedDimensions`, and the plan + * still resolved to "Approved \u2014 no blocking issues found" over seven + * "not assessed" note lines. + */ + const coreDead = [ + {dimension: "correctness-reviewer", cause: "unavailable"}, + {dimension: "skill-auditor", cause: "unavailable"}, + ]; + + it("holds instead of approving when the core passes produced no output (Khan/actions#328)", () => { + const fs = makeFakeFs( + staged({ + depth: "full", + claims: [], + skippedDimensions: coreDead, + noteLines: [ + "Note: correctness-reviewer not assessed this run (correctness-reviewer output unavailable).", + "Note: skill-auditor not assessed this run (skill-auditor output unavailable).", + ], + reconciliation: {resolve: ["PRRT_x"], skipLines: []}, + }), + ); + const plan = runSubmissionCli(fs); + expect(plan.event).toBe("HOLD_FOR_HUMAN"); + expect(plan.body).toContain("Holding for human review"); + expect(plan.body).toContain( + "correctness-reviewer not assessed this run", + ); + expect(plan.body).toContain("To get unstuck"); + // A hold never approves, stamps, posts inline, or resolves threads. + expect(plan.body).not.toContain("Approved"); + expect(plan.body).not.toContain("pr-reviewer:rereview"); + expect(plan.comments).toEqual([]); + expect(plan.resolve).toEqual([]); + expect(plan.skipSubmission).toBe(false); + expect(plan.reasons).toContainEqual({ + code: "core-dimension-unavailable", + dimension: "correctness", + }); + expect(plan.reasons).toContainEqual({ + code: "core-dimension-unavailable", + dimension: "skill-severity", + }); + }); + + it("stays REQUEST_CHANGES when a blocking claim posts despite the dead core pass", () => { + const fs = makeFakeFs( + staged({ + depth: "full", + claims: [claim()], + skippedDimensions: [coreDead[1]], + }), + ); + const plan = runSubmissionCli(fs); + // The hold only ever replaces a would-be auto-approval; a blocking + // finding is actionable on its own and wins. + expect(plan.event).toBe("REQUEST_CHANGES"); + expect(plan.comments).toHaveLength(1); + expect(plan.reasons).toContainEqual({ + code: "core-dimension-unavailable", + dimension: "skill-severity", + }); + }); + + it("folds surviving non-blocking claims into the hold comment as lines", () => { + const fs = makeFakeFs( + staged({ + depth: "full", + claims: [claim({label: "suggestion (non-blocking)"})], + skippedDimensions: coreDead, + }), + ); + const plan = runSubmissionCli(fs); + expect(plan.event).toBe("HOLD_FOR_HUMAN"); + expect(plan.comments).toEqual([]); + expect(plan.body).toContain("- `a.ts:2` suggestion (non-blocking): s"); + expect( + plan.notes.some((note) => + note.includes("folded into the hold comment"), + ), + ).toBe(true); + }); + + it("carries blocking-only collapsed pr-level claims into the hold body", () => { + // The #329 blocking-only modifier diverts non-blocking pr-level + // claims into a collapsed bucket that only the normal path renders; + // a hold must fold them too, not drop them. + const fs = makeFakeFs( + staged( + { + depth: "scoped", + claims: [ + claim({ + id: "pr1", + path: undefined, + line: undefined, + label: "suggestion (non-blocking)", + subject: "spanning concern", + }), + ], + skippedDimensions: coreDead, + }, + { + [`${REVIEW}/rereview-plan.json`]: JSON.stringify({ + depth: "scoped", + mode: "scoped", + stampAnchorDraft: false, + stampHunks: {}, + }), + [`${REVIEW}/routing.json`]: JSON.stringify({ + reReviewBlockingOnly: true, + }), + }, + ), + ); + const plan = runSubmissionCli(fs); + expect(plan.event).toBe("HOLD_FOR_HUMAN"); + expect(plan.body).toContain( + "- suggestion (non-blocking): spanning concern", + ); + }); + + it("does not hold for a non-core lens or pattern triage (note-and-continue)", () => { + const fs = makeFakeFs( + staged({ + depth: "full", + claims: [], + skippedDimensions: [ + {dimension: "holistic", cause: "unavailable"}, + {dimension: "pattern triage", cause: "unavailable"}, + ], + }), + ); + const plan = runSubmissionCli(fs); + expect(plan.event).toBe("APPROVE"); + expect(plan.reasons).toContainEqual({ + code: "pattern-triage-unavailable", + }); + }); + + it("reads a dispatch-result without skippedDimensions as all-assessed (older dispatcher)", () => { + const fs = makeFakeFs(staged({depth: "full", claims: []})); + expect(runSubmissionCli(fs).event).toBe("APPROVE"); + }); +}); + +describe("the gate's hold shape", () => { + const holdFs = () => + makeFakeFs( + staged({ + depth: "full", + claims: [], + skippedDimensions: [ + {dimension: "correctness-reviewer", cause: "unavailable"}, + {dimension: "skill-auditor", cause: "unavailable"}, + ], + noteLines: [ + "Note: correctness-reviewer not assessed this run (correctness-reviewer output unavailable).", + ], + }), + ); + + it("passes when exactly the hold comment queues", () => { + const plan = runSubmissionCli(holdFs()); + const result = evaluateDispatchConformance({ + items: [{type: "add_comment", body: plan.body}], + plan: {depth: "full"}, + routing: {enabledReviewers: [], lensesToSpawn: []}, + outFiles: {}, + submissionPlan: plan, + }); + expect(result.violations).toEqual([]); + }); + + it("blocks a review submission queued over a hold plan (the auto-approve shape)", () => { + const plan = runSubmissionCli(holdFs()); + const result = evaluateDispatchConformance({ + items: [ + {type: "add_comment", body: plan.body}, + { + type: "submit_pull_request_review", + event: "APPROVE", + body: "Approved \u2014 no blocking issues found.", + }, + ], + plan: {depth: "full"}, + routing: {enabledReviewers: [], lensesToSpawn: []}, + outFiles: {}, + submissionPlan: plan, + }); + expect( + result.violations.some( + (v) => + v.code === "submission-plan-mismatch" && + v.dimension === "verdict", + ), + ).toBe(true); + }); + + it("blocks a dropped hold comment (the disclosure must post)", () => { + const plan = runSubmissionCli(holdFs()); + const result = evaluateDispatchConformance({ + items: [], + plan: {depth: "full"}, + routing: {enabledReviewers: [], lensesToSpawn: []}, + outFiles: {}, + submissionPlan: plan, + }); + expect( + result.violations.some((v) => v.dimension === "hold comment"), + ).toBe(true); + }); + + it("blocks inline comments and thread resolutions on a hold run", () => { + const plan = runSubmissionCli(holdFs()); + const result = evaluateDispatchConformance({ + items: [ + {type: "add_comment", body: plan.body}, + { + type: "create_pull_request_review_comment", + path: "a.ts", + line: 2, + body: "b", + }, + { + type: "resolve_pull_request_review_thread", + thread_id: "PRRT_x", + }, + ], + plan: {depth: "full"}, + routing: {enabledReviewers: [], lensesToSpawn: []}, + outFiles: {}, + submissionPlan: plan, + }); + expect( + result.violations.some((v) => v.dimension === "inline comments"), + ).toBe(true); + expect( + result.violations.some((v) => v.dimension === "thread resolutions"), + ).toBe(true); + }); + + it("blocks a spliced hold comment body (present but not the plan's)", () => { + const plan = runSubmissionCli(holdFs()); + const result = evaluateDispatchConformance({ + items: [ + { + type: "add_comment", + body: "All reviewers passed; merging is safe.", + }, + ], + plan: {depth: "full"}, + routing: {enabledReviewers: [], lensesToSpawn: []}, + outFiles: {}, + submissionPlan: plan, + }); + expect( + result.violations.some((v) => v.dimension === "hold comment"), + ).toBe(true); + }); + + it("tolerates sanitizer drift on the hold comment body (normalized comparison)", () => { + const plan = runSubmissionCli(holdFs()); + const result = evaluateDispatchConformance({ + items: [{type: "add_comment", body: `${plan.body}\n`}], + plan: {depth: "full"}, + routing: {enabledReviewers: [], lensesToSpawn: []}, + outFiles: {}, + submissionPlan: plan, + }); + expect(result.violations).toEqual([]); + }); +}); diff --git a/workflows/review/lib/submission.ts b/workflows/review/lib/submission.ts index b44db92e..37a48b1e 100644 --- a/workflows/review/lib/submission.ts +++ b/workflows/review/lib/submission.ts @@ -33,6 +33,17 @@ * post nor count toward the verdict. * - REQUEST_CHANGES iff at least one posted claim carries a blocking label * (via computeVerdict, threshold 1). + * - HOLD_FOR_HUMAN when a core review pass (`correctness-reviewer` or + * `skill-auditor`) produced no output this run and the run would + * otherwise have auto-approved (computeVerdict's core-dimension gate, + * fed from the dispatcher's `skippedDimensions`). A hold is not a + * review event: the plan's body posts as one standalone PR comment, + * nothing else queues, and no fingerprint stamp is written, so the + * next run reviews in full. The production shape this closes: + * Khan/actions#328's re-run (31124365377 attempt 2), where every core + * lens died on an API auth error and the run still submitted + * "Approved — no blocking issues found" over seven "not assessed" + * note lines. * - The reduced-depth flip rule: at flip-gated/fast depth over a prior * REQUEST_CHANGES stamp, `rereview.json`'s keptBlockingCount floors the * verdict at REQUEST_CHANGES. @@ -51,8 +62,14 @@ import {computeRisksPatternsKey, RISKS_PATTERNS_KEY_PATH} from "./cache-record"; import type {Claim} from "./dispatch-contracts"; +import {DEFAULT_FINDERS, TRIAGE_DIMENSION} from "./dispatch-roster"; import {runCli as runNotifiedCli} from "./notified"; -import {isBlockingLabel, renderReviewBody} from "./render-comment"; +import { + HOLD_HEAD, + HOLD_UNSTUCK_LINES, + isBlockingLabel, + renderReviewBody, +} from "./render-comment"; import {runRereviewCli, type RereviewCliFs} from "./rereview"; import {normalizeBody} from "./sanitizer-normalize"; import { @@ -62,6 +79,7 @@ import { type PriorReview, } from "./rereview-mode"; import {computeVerdict} from "./verdict"; +import type {DimensionStatus, VerdictReason} from "./verdict"; /* -------------------------------------------------------------------------- */ /* Types and paths */ @@ -73,9 +91,16 @@ const CACHE_MEMORY_DIR = "/tmp/gh-aw/cache-memory"; export type PlannedComment = {path: string; line: number; body: string}; export type SubmissionPlan = { - /** The event to submit (Step 4's two-state rule; never HOLD here). */ - event: "APPROVE" | "REQUEST_CHANGES"; - /** The full review body, stamp included; submit verbatim. */ + /** + * The outcome: a review event to submit (Step 4's mechanical rule), or + * HOLD_FOR_HUMAN, which submits NO review — the orchestrator posts + * `body` as one standalone PR comment instead (Step 6's hold branch). + */ + event: "APPROVE" | "REQUEST_CHANGES" | "HOLD_FOR_HUMAN"; + /** + * The full text to post verbatim: the review body (stamp included) for + * a review event, or the hold comment (never stamped) for a hold. + */ body: string; /** * Whether the orchestrator may emit NO submission at all (the @@ -91,7 +116,7 @@ export type SubmissionPlan = { /** Thread ids to resolve (the reconciler's decision, passed through). */ resolve: string[]; /** Why the event is what it is (fixed-format, for the artifact). */ - reasons: string[]; + reasons: VerdictReason[]; /** Non-blocking composition observations. */ notes: string[]; }; @@ -117,6 +142,15 @@ const parseSkipLines = (raw: unknown): Set => { return keys; }; +/** Write the staged plan (`submission-plan.json`) and hand it back. */ +const stagePlan = (fs: SubmissionFs, plan: SubmissionPlan): SubmissionPlan => { + fs.writeFileSync( + `${REVIEW_DIR}/submission-plan.json`, + JSON.stringify(plan, null, 2), + ); + return plan; +}; + const readJson = (fs: SubmissionFs, path: string): unknown => { if (!fs.existsSync(path)) { return undefined; @@ -297,6 +331,7 @@ export const runSubmissionCli = ( riskFiles?: unknown; patterns?: unknown; excludedFiles?: unknown; + skippedDimensions?: unknown; } | undefined; if (dispatch === undefined) { @@ -449,6 +484,132 @@ export const runSubmissionCli = ( } } + // A blocking candidate the dispatcher suppressed as a duplicate of a + // still-open BLOCKING bot thread (trial suggestion g) blocks like a + // fresh one: the reviewer re-confirmed the defect, and the open thread + // is the actionable feedback. Without this floor, suppression could + // flip the verdict to APPROVE over an unfixed blocking objection. Both + // sides must be blocking: suppression happens before validation, so the + // candidate's own label is unvalidated; the matched thread's opener + // label is the severity that DID survive a prior run's validation. A + // blocking candidate matching a non-blocking open thread therefore + // never floors (it would force REQUEST_CHANGES with no validation and + // no visible blocking comment). (A thread the reduced-depth floor above + // already counted may add one more here; the verdict is the same either + // way, only the reason count differs.) + const suppressedBlocking = ( + Array.isArray(dispatch.threadSuppressions) + ? dispatch.threadSuppressions + : [] + ).filter( + (entry) => + typeof (entry as {label?: unknown}).label === "string" && + isBlockingLabel((entry as {label: string}).label) && + (entry as {threadBlocking?: unknown}).threadBlocking === true, + ).length; + + // Real dimension availability (the hold rule's input): a core lens + // recorded in the dispatcher's `skippedDimensions` (either cause) + // produced no usable output this run and must not be reported + // "assessed", or a crashed run auto-approves. The dimension names are + // imported from the modules that write them (`DEFAULT_FINDERS`, + // `TRIAGE_DIMENSION`), so a rename cannot silently decouple the hold + // from the dispatcher. The production dispatcher always writes + // `skippedDimensions` (an empty array on a clean run); an absent field + // (hand-staged eval fixtures) reads as all-assessed rather than + // guessing at a hold. + const skippedDimensionNames = new Set( + (Array.isArray(dispatch.skippedDimensions) + ? dispatch.skippedDimensions + : [] + ) + .map((entry) => (entry as {dimension?: unknown}).dimension) + .filter((name): name is string => typeof name === "string"), + ); + const dimensionStatus = (name: string): DimensionStatus => + skippedDimensionNames.has(name) ? "unavailable" : "assessed"; + + const verdict = computeVerdict({ + postedLabels: claims.map((claim) => claim.label), + dimensions: { + correctness: dimensionStatus(DEFAULT_FINDERS[0]), + skillSeverity: dimensionStatus(DEFAULT_FINDERS[1]), + patternTriage: dimensionStatus(TRIAGE_DIMENSION), + }, + keptBlockingCount: keptBlockingFloor + suppressedBlocking, + }); + + // The depth note (Step 3), when the run reduced. + const plan = readJson(fs, `${REVIEW_DIR}/rereview-plan.json`) as + | {mode?: unknown; tripwireRearmed?: unknown; divergence?: unknown} + | undefined; + const depthNotes: string[] = []; + if (plan !== undefined && depth !== "full") { + const mode = typeof plan.mode === "string" ? plan.mode : "full"; + depthNotes.push( + `Note: re-review ran at ${depth} depth (re-review mode ${mode}${ + blockingOnly ? ", blocking-only" : "" + }).`, + ); + } + if (plan?.tripwireRearmed === true) { + const share = ( + plan.divergence as {unreviewedShare?: unknown} | undefined + )?.unreviewedShare; + depthNotes.push( + `Note: divergence tripwire re-armed a full review (unreviewed share ${ + typeof share === "number" ? share.toFixed(2) : "unknown" + }).`, + ); + } + + // The hold path (computeVerdict's core-dimension gate): a run whose + // correctness or skill/severity pass produced no output must not resolve + // to an approval the automation cannot stand behind. A hold is not a + // review event: the orchestrator posts this body as ONE standalone PR + // comment (the add-comment safe output) and submits no review, so the + // PR shows neither an approval nor a block. Claims that survived + // validation fold into the comment as one line each (blocking claims + // cannot exist here: they force REQUEST_CHANGES over the hold), the + // reconciler's resolutions are withheld (a partial run leaves existing + // threads standing), and no fingerprint stamp is written, so the cache + // writer refuses the record and the next run reviews in full. + if (verdict.event === "HOLD_FOR_HUMAN") { + // Both post-fold buckets ride the hold comment: anchored claims (no + // inline comments post on a hold) and the blocking-only collapsed + // pr-level claims (their collapsed section renders only on the + // normal path). + const heldClaimLines = [...anchored, ...prLevelCollapsed].map( + (claim) => { + notes.push( + `claim ${claim.id} folded into the hold comment (a hold posts no inline comments)`, + ); + return claim.path !== undefined && claim.line !== undefined + ? `- \`${claim.path}:${claim.line}\` ${claim.label}: ${claim.subject}` + : `- ${claim.label}: ${claim.subject}`; + }, + ); + return stagePlan(fs, { + event: "HOLD_FOR_HUMAN", + body: [ + HOLD_HEAD, + rereview.section, + ...prLevelLines, + ...heldClaimLines, + ...noteLines, + ...depthNotes, + ...HOLD_UNSTUCK_LINES, + ] + .filter((line) => line !== "") + .join("\n"), + skipSubmission: false, + comments: [], + resolve: [], + reasons: verdict.reasons, + notes, + }); + } + // The posting bar (the Step 5 ranked bar, as code): rank // blocking before non-blocking, then confidence descending (the sort is // stable, so dispatch order breaks ties). A claim below medium @@ -537,76 +698,9 @@ export const runSubmissionCli = ( ); } - // A blocking candidate the dispatcher suppressed as a duplicate of a - // still-open BLOCKING bot thread (trial suggestion g) blocks like a - // fresh one: the reviewer re-confirmed the defect, and the open thread - // is the actionable feedback. Without this floor, suppression could - // flip the verdict to APPROVE over an unfixed blocking objection. Both - // sides must be blocking: suppression happens before validation, so the - // candidate's own label is unvalidated; the matched thread's opener - // label is the severity that DID survive a prior run's validation. A - // blocking candidate matching a non-blocking open thread therefore - // never floors (it would force REQUEST_CHANGES with no validation and - // no visible blocking comment). (A thread the reduced-depth floor above - // already counted may add one more here; the verdict is the same either - // way, only the reason count differs.) - const suppressedBlocking = ( - Array.isArray(dispatch.threadSuppressions) - ? dispatch.threadSuppressions - : [] - ).filter( - (entry) => - typeof (entry as {label?: unknown}).label === "string" && - isBlockingLabel((entry as {label: string}).label) && - (entry as {threadBlocking?: unknown}).threadBlocking === true, - ).length; - - const verdict = computeVerdict({ - postedLabels: claims.map((claim) => claim.label), - dimensions: { - correctness: "assessed", - skillSeverity: "assessed", - patternTriage: "assessed", - }, - keptBlockingCount: keptBlockingFloor + suppressedBlocking, - }); - // With every dimension reported assessed (the dispatcher's unavailable - // dimensions surface as note lines instead), the two-state Step 4 rule - // is what remains: HOLD_FOR_HUMAN is unreachable here, and the guard - // makes a future edit that feeds real dimension availability into - // computeVerdict fail loudly instead of auto-approving a crashed run. - if (verdict.event === "HOLD_FOR_HUMAN") { - throw new Error( - "HOLD_FOR_HUMAN reached the submission plan: dimension availability must not feed this CLI without a hold path", - ); - } const event = verdict.event === "REQUEST_CHANGES" ? "REQUEST_CHANGES" : "APPROVE"; - // The depth note (Step 3), when the run reduced. - const plan = readJson(fs, `${REVIEW_DIR}/rereview-plan.json`) as - | {mode?: unknown; tripwireRearmed?: unknown; divergence?: unknown} - | undefined; - const depthNotes: string[] = []; - if (plan !== undefined && depth !== "full") { - const mode = typeof plan.mode === "string" ? plan.mode : "full"; - depthNotes.push( - `Note: re-review ran at ${depth} depth (re-review mode ${mode}${ - blockingOnly ? ", blocking-only" : "" - }).`, - ); - } - if (plan?.tripwireRearmed === true) { - const share = ( - plan.divergence as {unreviewedShare?: unknown} | undefined - )?.unreviewedShare; - depthNotes.push( - `Note: divergence tripwire re-armed a full review (unreviewed share ${ - typeof share === "number" ? share.toFixed(2) : "unknown" - }).`, - ); - } - const head = renderReviewBody({ event, hasInlineComments: inline.length > 0, @@ -639,7 +733,7 @@ export const runSubmissionCli = ( priorStamp !== null && priorStamp.verdict === "APPROVE"; - const submission: SubmissionPlan = { + return stagePlan(fs, { event, body, skipSubmission, @@ -651,12 +745,7 @@ export const runSubmissionCli = ( : [], reasons: verdict.reasons, notes, - }; - fs.writeFileSync( - `${REVIEW_DIR}/submission-plan.json`, - JSON.stringify(submission, null, 2), - ); - return submission; + }); }; // Run only when executed directly (review.md Steps 4-6, scripted dispatch diff --git a/workflows/review/review.md b/workflows/review/review.md index 655ead44..dc96d62d 100644 --- a/workflows/review/review.md +++ b/workflows/review/review.md @@ -639,7 +639,15 @@ cd gh-aw-review-lib && npx -y tsx workflows/review/lib/submission.ts `skipSubmission` is `true` (the plan CLI sets it for an APPROVE with zero `comments` whose body is the bare approve line, on a PR whose last stamped verdict was already APPROVE; the gate reads the same field, so the two can - never disagree). When it is `false`, always submit. The dispatch-conformance gate + never disagree). When it is `false`, always submit. One exception outranks + both: when the plan's `event` is `HOLD_FOR_HUMAN` (a core review pass + produced no output on a run that would otherwise auto-approve), emit **no** + review submission, **no** inline comments, and **no** thread resolutions + (a hold plan stages none of them) — instead post the plan's `body` verbatim + as one standalone PR comment with the `add-comment` safe output, then skip + Steps 7 and 8 (they are APPROVE-only) and continue at Step 9, where the + cache CLI handles the hold on its own (it leaves the prior fingerprints + standing so the next run reviews in full). The dispatch-conformance gate compares what you queued against the staged plan and blocks the submission on any deviation, so a mis-typed or "improved" body is a red run, never a posted one. `dispatch-result.json`'s `riskFiles`, @@ -658,7 +666,13 @@ cd gh-aw-review-lib && npx -y tsx workflows/review/lib/submission.ts The verdict is computed by the plan CLI (Step 3), never by you: REQUEST_CHANGES iff a validated posted claim carries a blocking label, plus the reduced-depth flip floor over kept blocking threads and the open-thread suppression floor — -all `lib/verdict.ts` / `lib/submission.ts` rules. The plan's `event` IS the +all `lib/verdict.ts` / `lib/submission.ts` rules. A third outcome exists: +HOLD_FOR_HUMAN, when a core review pass (`correctness-reviewer` or +`skill-auditor`) produced no usable output this run and the run would +otherwise have auto-approved — the automation never approves a change its +core passes did not look at (a blocking finding still wins: it is actionable +on its own, so the verdict stays REQUEST_CHANGES and the gap is disclosed in +a note line). The plan's `event` IS the verdict; never recompute, second-guess, or override it. (The blocking-label vocabulary and the concrete-failing-scenario bar live in the sub-agent definitions and the shared lib.) @@ -687,6 +701,13 @@ with **one** `submit-pull-request-review` call carrying the plan's `event` and submit nothing. The dispatch-conformance gate blocks any deviation from the plan, so a mis-typed or "improved" body is a red run, never a posted one. +When the plan's `event` is `HOLD_FOR_HUMAN`, there is no review to submit: +post the plan's `body` verbatim as one standalone PR comment with the +`add-comment` safe output, and queue nothing else (no review submission, no +inline comments, no thread resolutions). The body already explains the hold +and how the author gets unstuck; the gate blocks a hold run that submits a +review, posts inline comments, resolves threads, or drops the comment. + ## Step 7: On Approval — Post Risk and Patterns as a PR Comment **Only run this step when the verdict is APPROVE.** When requesting changes, skip