diff --git a/.changeset/review-submission-plan.md b/.changeset/review-submission-plan.md new file mode 100644 index 00000000..c6fde3e4 --- /dev/null +++ b/.changeset/review-submission-plan.md @@ -0,0 +1,7 @@ +--- +"review": minor +--- + +The submission plan (deterministic-orchestrator slice 4, the production probe of the end state): in scripted dispatch mode, Steps 4-6 become code. After the dispatcher returns, the orchestrator runs `lib/submission.ts` once; it drops any claim anchored on a line the thread-reconciler flagged in `skipLines` (Step 5's defer-to-open-human-threads rule, applied before the verdict so a deferred claim neither posts nor counts), computes the verdict mechanically (Step 4's blocking-label rule via `computeVerdict`, plus the reduced-depth flip floor from `rereview.json`'s `keptBlockingCount` over a prior REQUEST_CHANGES stamp), renders every inline comment from the surviving claims (post-validation labels, rule quotes, suggestion blocks), renders the full review body (verdict head, the code-rendered accountability section, the dispatcher's note lines, the depth/tripwire notes, the fingerprint stamp as the final line, pr-level claims folded in), and stages `submission-plan.json`. The orchestrator's remaining model role is emitting safe outputs that match the plan verbatim, and the dispatch-conformance gate gains the plan-match rule: when a plan is staged, the queued event, body, and inline comments must match it under a sanitizer-tolerant normalization, so any splice, omission, or "improvement" blocks the submission and fails the run (the #244 accountability-splice check, as code). Queueing nothing is permitted only for the exact Step 6 redundant-approval shape: an APPROVE plan with no comments whose body is the bare comment-less approve line, so an approval carrying a shed disclosure can never be dropped on the floor silently. The safe-output emission itself is the remaining seam code cannot own under gh-aw today: the queue is a run-local JSONL append that needs no credentials, but the agent sandbox mounts `${RUNNER_TEMP}/gh-aw` read-only, so only the safeoutputs MCP container can write it; removing the seam wants a writable path into the queue (an upstream mount change, or a post-agent step on the host), neither tested yet. Rides the same live-trial gate as scripted dispatch; nothing changes in task mode. + +Audit hardening for the plan-match rule this changeset introduces: an audit of gh-aw v0.81.6's ingest sanitizer against the rule-7 normalization found unabsorbed transforms that false-block a byte-faithful submission. The deployed URL policy is allowed-only and runs inside code regions, so any cited non-allowlisted https URL (MDN, StackOverflow) came back "(host/redacted)" against the plan's `` placeholder: a guaranteed mismatch on ordinary review prose. The normalization (now its own module, `lib/sanitizer-normalize.ts`) folds https URLs and the domain-redaction token to a host-bearing placeholder, so the sanitizer's own rewrites pass while a link splice that swaps the host still blocks; it also absorbs neutralizeTemplateDelimiters' escaping backslashes (unbackticked `${{ }}` in prose), the bidi-control strip, and the C0/DEL control strip. Every fold applies to both sides, so splice detection loses nothing. diff --git a/workflows/review/README.md b/workflows/review/README.md index 1eacbadf..cfef7811 100644 --- a/workflows/review/README.md +++ b/workflows/review/README.md @@ -267,7 +267,17 @@ re-review scoped sheds computed from `routing.json`), the provenance gate, the scope filter, and claim validation as code, inside the same firewall sandbox (the api-proxy meters and caps script-spawned sub-agents exactly like - Task-spawned ones). Scripted mode is the production probe of the + Task-spawned ones). In scripted mode Steps 4-6 are code too: the submission + CLI (`lib/submission.ts`) computes the verdict, renders the comments and + the full review body, and stages `submission-plan.json`; the orchestrator + emits safe outputs that must match the plan (the gate blocks any + deviation), which reduces its model role to typing MCP calls the plan + dictates. The safe-output emission itself is the remaining seam: the queue + is a run-local JSONL append that needs no credentials, but the agent + sandbox mounts `${RUNNER_TEMP}/gh-aw` read-only, so only the safeoutputs + MCP container can write it. Removing the seam wants a writable path into + the queue (an upstream mount change, or a post-agent step on the host); + neither is tested yet. Scripted mode is the production probe of the deterministic-orchestrator migration and is live-trial-gated; an unknown mode degrades to `task` with a warning. diff --git a/workflows/review/lib/dispatch-gate-hardening.test.ts b/workflows/review/lib/dispatch-gate-hardening.test.ts index fd4b6869..b27d71d0 100644 --- a/workflows/review/lib/dispatch-gate-hardening.test.ts +++ b/workflows/review/lib/dispatch-gate-hardening.test.ts @@ -508,4 +508,44 @@ describe("staged-input robustness (slice 3 re-review)", () => { "flip-vetoed-kept-blocking", ]); }); + + // `readJsonIfPresent` passes `JSON.parse("null")` through unchanged, so a + // `submission-plan.json` holding literal `null` arrives as `null` rather + // than `undefined`. A bare `!== undefined` guard admits it and the + // property reads throw — and because the CLI entry catch exits 0 with the + // queue untouched, that throw fails open ALL SEVEN rules, not just rule 7. + it("does not throw on a null submission plan, and still runs the other rules", () => { + const run = () => + evaluate({ + items: [commentItem()], + plan: {depth: "full"}, + outFiles: {}, + submissionPlan: null, + }); + expect(run).not.toThrow(); + + // A null plan is treated as "no plan staged", so rule 7 has nothing to + // bind against — but the rules that do not depend on it must still + // fire. Here: comments queued with no submission, over a full-depth + // staging that dispatched nothing. + const codes = run().violations.map((v) => v.code); + expect(codes.length).toBeGreaterThan(0); + expect(codes).not.toContain("submission-plan-mismatch"); + }); + + it("still binds the queue to a well-formed plan (the null guard is not a bypass)", () => { + const result = evaluate({ + items: [submitItem("APPROVE", "")], + plan: {depth: "full"}, + outFiles: conformingOutFiles(), + submissionPlan: { + event: "REQUEST_CHANGES", + body: "blocking finding", + comments: [], + }, + }); + expect(result.violations.map((v) => v.code)).toContain( + "submission-plan-mismatch", + ); + }); }); diff --git a/workflows/review/lib/dispatch-gate.ts b/workflows/review/lib/dispatch-gate.ts index a7a03710..79a96ca0 100644 --- a/workflows/review/lib/dispatch-gate.ts +++ b/workflows/review/lib/dispatch-gate.ts @@ -50,6 +50,10 @@ * (`out/thread-reconciler.json` `resolve`); the deficit direction is * reported as executed-vs-decided accounting, never blocked (slice 3; * the #244 ledger). + * 7. When a submission plan is staged (`submission-plan.json`, scripted + * 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). * * Violation behavior: strip every posting/mutating item from the queue * (keeping the diagnostics and the `out/` artifact upload so the evidence @@ -71,9 +75,10 @@ */ import {extractJsonValue} from "./agent-json"; -import {isBlockingLabel} from "./render-comment"; +import {isBlockingLabel, renderReviewBody} from "./render-comment"; import {parseLeadingLabel} from "./rereview"; import {findLatestStamp, stampFromCacheMemory} from "./rereview-mode"; +import {normalizeBody} from "./sanitizer-normalize"; /* -------------------------------------------------------------------------- */ /* Types */ @@ -103,7 +108,8 @@ export type DispatchGateViolationCode = | "shed-undisclosed" | "approve-with-blocking-comment" | "flip-vetoed-kept-blocking" - | "resolve-not-decided"; + | "resolve-not-decided" + | "submission-plan-mismatch"; export type DispatchGateViolation = { /** Fixed-format code (never prose). */ @@ -147,6 +153,8 @@ export type DispatchGateInput = { cacheMemory?: unknown; /** Parsed `rereview.json` (the accountability result; `keptBlockingCount`). */ rereviewAccounting?: unknown; + /** Parsed `submission-plan.json` (scripted mode; slice 4). */ + submissionPlan?: unknown; }; /* -------------------------------------------------------------------------- */ @@ -508,6 +516,143 @@ 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; + }) + : 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 : ""; + if ( + planStaged.event !== "APPROVE" || + planComments.length > 0 || + normalizeBody(planBody) !== bareApprove + ) { + 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})`, + }); + } + } + } + return { conformant: violations.length === 0, violations, @@ -652,6 +797,10 @@ export const runDispatchGateCli = (fs: DispatchGateFs): DispatchGateReport => { `/tmp/gh-aw/cache-memory/pr-${prContext.number}.json`, ) : undefined; + const submissionPlan = readJsonIfPresent( + fs, + `${REVIEW_DIR}/submission-plan.json`, + ); const evaluation = evaluateDispatchConformance({ items, @@ -661,6 +810,7 @@ export const runDispatchGateCli = (fs: DispatchGateFs): DispatchGateReport => { priorReviews, rereviewAccounting, cacheMemory, + submissionPlan, }); evaluation.notes.unshift(...notes); diff --git a/workflows/review/lib/sanitizer-normalize.ts b/workflows/review/lib/sanitizer-normalize.ts new file mode 100644 index 00000000..3f42be0c --- /dev/null +++ b/workflows/review/lib/sanitizer-normalize.ts @@ -0,0 +1,78 @@ +/** + * The ingest sanitizer's transforms, mirrored so the dispatch gate's rule 7 + * can compare a staged submission plan against the queued safe outputs + * without false-blocking a byte-faithful transcription. + * + * gh-aw v0.81.6 rewrites everything the agent queues before the gate sees it + * (`sanitize_content_core.cjs`): HTML comments vanish, unicode is hardened, + * template delimiters gain escaping, and URLs are redacted under an + * allowed-only policy. The plan is composed BEFORE that pass, so the two + * sides differ on a conforming run. {@link normalizeBody} applies every + * absorbed transform to BOTH sides, so tolerance costs no splice detection: + * anything the sanitizer would not have done is still a mismatch (#244). + * + * Documented-not-absorbed residuals, each of which needs a pathological body + * and fails red rather than silently: HTML entity decoding, XML tag + * conversion, the percent-decode side effect, homoglyph folds, the 65k + * truncation, markdown link titles, and tilde fences. + */ + +/** + * Fold one body to its sanitizer-tolerant comparison form. Applied to the + * plan and the queued text alike; never to text that gets posted. + */ +export const normalizeBody = (text: string): string => + text + // The ingest sanitizer deletes ALL XML/HTML comments + // (removeXmlComments), so the queued body can never carry the + // plan's fingerprint stamp; comparing modulo comments is what + // "sanitizer-tolerant" requires (trial run 29893634730 blocked + // a byte-faithful transcription on exactly this). + .replace(//g, "") + // The sanitizer's hardenUnicodeText applies NFKC and strips + // zero-width characters (gh-aw sanitize_content_core.cjs), which + // rewrites compatibility characters: trial run 29903306596 + // blocked a jq-verbatim emission because one reviewer-authored + // ellipsis came back as three dots (NFKC). Apply the same + // normalization on both sides, plus the typographic quote/dash + // folds NFKC does not cover. + .normalize("NFKC") + .replace(/\u034f/g, "") + // Zero-width, bidi-control (sanitizer step 4), C0/DEL (its + // control-strip): all deleted on the queued side only, so + // delete them on both. + .replace(/[\u00ad\u200b-\u200f\u2060-\u2064\ufeff]/g, "") + .replace(/[\u202a-\u202e\u2066-\u2069]/g, "") + // eslint-disable-next-line no-control-regex + .replace(/[\u0000-\u0008\u000b\u000c\u000e-\u001f\u007f]/g, "") + .replace(/[\u2018\u2019\u201a\u201b]/g, "'") + .replace(/[\u201c\u201d\u201e\u201f]/g, '"') + .replace(/[\u2012\u2013\u2014\u2015]/g, "-") + .toLowerCase() + .replace(/`/g, "") + // neutralizeTemplateDelimiters escapes {{ ${ {% {# <%= outside + // code regions; drop the escaping backslashes on both sides. + .replace(/\\(?=[{$%#<])/g, "") + // URL sanitization applies even inside code regions under the + // deployed allowed-only policy: a non-allowlisted domain or a + // non-https scheme is rewritten to "(host/redacted)" or + // "(redacted)" (sanitizeUrlDomains / sanitizeUrlProtocols, + // gh-aw v0.81.6). Domain redaction KEEPS the host, so an https + // URL folds to a host-bearing placeholder that both sides + // agree on and a cross-host link splice still trips the check. + // Protocol redaction drops the host entirely, so every other + // scheme folds hostless (matching the bare "(redacted)"). + // Deliberate residual: path and query ARE wildcarded, so swapping + // one same-host deep link for another passes rule 7; the splice + // check covers the prose and the link's host, not which page on + // that host is cited. + .replace(/https:\/\/([a-z0-9.-]+)\S*/g, "") + .replace(/[a-z][a-z0-9+.-]*:\/\/\S+/g, "") + .replace( + /(?:mailto|javascript|vbscript|data|about|tel|magnet):\S+/g, + "", + ) + .replace(/\(([a-z0-9.-]+)\/redacted\)/g, "") + .replace(/\(redacted\)/g, "") + .replace(/\s+/g, " ") + .trim(); diff --git a/workflows/review/lib/submission.test.ts b/workflows/review/lib/submission.test.ts new file mode 100644 index 00000000..ddb9e1ab --- /dev/null +++ b/workflows/review/lib/submission.test.ts @@ -0,0 +1,863 @@ +import {describe, it, expect} from "vitest"; + +import {evaluateDispatchConformance} from "./dispatch-gate"; +import {labelForFinding, renderComment} from "./render-comment"; +import {renderRereviewStamp, STAMP_SCHEMA_VERSION} from "./rereview-mode"; +import { + isDropInSuggestion, + renderClaimComment, + runSubmissionCli, + type SubmissionFs, +} from "./submission"; + +/** + * Submission-plan tests (deterministic-orchestrator slice 4): Steps 4-6 as + * code. The plan is composed from the dispatcher's validated claims through + * the same lib functions the eval runner uses (computeVerdict, + * renderReviewBody, the rereview accountability CLI, the stamp), and the + * dispatch-conformance gate's plan-match rule turns any deviation between + * the plan and the queued safe outputs into a blocked red run (the #244 + * accountability-splice check). + */ + +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("renderClaimComment", () => { + it("renders the Conventional Comment with the post-validation label", () => { + expect( + renderClaimComment( + claim({ + label: "suggestion (non-blocking)", + suggestion: "fixed()", + }) as never, + ), + ).toBe( + "**suggestion (non-blocking):** The guard was removed.\n\n```suggestion\nfixed()\n```", + ); + }); + + it("keeps the rule quote as a blockquote between prose and fix", () => { + const body = renderClaimComment( + claim({rule_quote: "Always guard.\nEven here."}) as never, + ); + expect(body).toContain("> **Rule:** Always guard.\n> Even here."); + }); + + it("keeps the suggestion fence for small code-shaped payloads", () => { + // Run 29897276810's legitimate drop-ins: a one-line cutoff fix and a + // five-line query chain. + expect( + isDropInSuggestion( + "\tcutoff := ctx.Time().Now().AddDate(0, 0, -MemoryTTLDays)", + ), + ).toBe(true); + expect( + isDropInSuggestion( + [ + "\tq := datastore.NewQuery(models.AIGuideMemoryKind).", + '\t\tFilterField("kaid", "=", kaid).', + '\t\tFilterField("created_at", "<", cutoff).', + "\t\tKeysOnly().", + "\t\tLimit(500)", + ].join("\n"), + ), + ).toBe(true); + }); + + it("treats prose that names code as prose (run 29901690493's fence misses)", () => { + expect( + isDropInSuggestion( + "Use ctx.Time().Now().AddDate(0, 0, -MemoryTTLDays), and add a test that writes a memory with created_at beyond the window and asserts it is deleted by the pass.", + ), + ).toBe(false); + expect( + isDropInSuggestion( + "Filter by the retention cutoff at read time in Query; keep (or drop) the write-path delete as a storage-cost optimization only.", + ), + ).toBe(false); + }); + + it("renders an English-prose suggestion as a sketch, not a suggestion fence (r3628128268)", () => { + const prose = + "Add a created_at >= cutoff filter in Query so stale memories can never surface regardless of write activity, and consider a native Datastore TTL policy on created_at in place of (or alongside) the write-path ExpireStale pass."; + expect(isDropInSuggestion(prose)).toBe(false); + const body = renderClaimComment( + claim({ + label: "suggestion (non-blocking)", + suggestion: prose, + }) as never, + ); + expect(body).not.toContain("```suggestion"); + expect(body).toContain("A sketch, not a committable replacement:"); + expect(body).toContain(`\`\`\`\`\n${prose}\n\`\`\`\``); + }); + + it("renders an oversized code payload as a sketch (r3628128224's 30-line test fn)", () => { + const testFn = [ + "func (suite *expirationSuite) TestExpirationRemovesStaleMemories() {", + "\tctx := suite.KAContext()", + ...Array.from( + {length: 26}, + (_, i) => `\tsuite.Require().NoError(step${i}(ctx))`, + ), + "\tsuite.Require().Len(keys, 1)", + "}", + ].join("\n"); + expect(isDropInSuggestion(testFn)).toBe(false); + const body = renderClaimComment( + claim({label: "todo (blocking)", suggestion: testFn}) as never, + ); + expect(body).not.toContain("```suggestion"); + expect(body).toContain("A sketch, not a committable replacement:"); + }); +}); + +describe("runSubmissionCli", () => { + it("plans REQUEST_CHANGES with the fixed body line when a blocking claim posts", () => { + const fs = makeFakeFs( + staged({ + depth: "full", + claims: [claim()], + noteLines: [], + reconciliation: {resolve: ["t1"], keep: []}, + }), + ); + const plan = runSubmissionCli(fs); + expect(plan.event).toBe("REQUEST_CHANGES"); + expect(plan.body.split("\n")[0]).toBe( + "Changes requested — see inline comments.", + ); + expect(plan.comments).toEqual([ + { + path: "a.ts", + line: 2, + body: "**issue (blocking):** The guard was removed.", + }, + ]); + expect(plan.resolve).toEqual(["t1"]); + // The stamp is the final line (hidden HTML comment). + expect(plan.body.split("\n").at(-1)).toMatch(/^$/); + // The plan is staged for the gate's plan-match rule. + expect( + JSON.parse(fs.files[`${REVIEW}/submission-plan.json`]).event, + ).toBe("REQUEST_CHANGES"); + }); + + it("plans an empty-head APPROVE when only non-blocking claims post", () => { + const fs = makeFakeFs( + staged({ + depth: "full", + claims: [claim({label: "suggestion (non-blocking)"})], + noteLines: [ + "Note: holistic not assessed this run (shed under the High-tier run budget).", + ], + }), + ); + const plan = runSubmissionCli(fs); + expect(plan.event).toBe("APPROVE"); + // Empty verdict head with inline comments; the note line and stamp + // are the body. + expect(plan.body).toContain("holistic not assessed this run"); + expect(plan.body).not.toContain("Approved — no blocking issues found."); + }); + + it("plans the comment-less APPROVE body when nothing posts", () => { + const fs = makeFakeFs(staged({depth: "full", claims: []})); + const plan = runSubmissionCli(fs); + expect(plan.event).toBe("APPROVE"); + expect(plan.body).toContain("Approved — no blocking issues found."); + }); + + it("applies the reduced-depth flip floor from kept blocking threads", () => { + const stamp = renderRereviewStamp({ + schemaVersion: STAMP_SCHEMA_VERSION, + depth: "full", + verdict: "REQUEST_CHANGES", + anchorDraft: false, + anchorHunks: {}, + }); + const fs = makeFakeFs( + staged( + {depth: "fast", claims: []}, + { + [`${REVIEW}/prior-reviews.json`]: JSON.stringify([ + {body: stamp}, + ]), + [`${REVIEW}/threads.json`]: JSON.stringify([ + { + thread_id: "t1", + path: "a.ts", + line: 2, + comments: [ + { + author: "github-actions[bot]", + body: "**issue (blocking):** still broken", + }, + ], + }, + ]), + [`${REVIEW}/out/thread-reconciler.json`]: JSON.stringify({ + resolve: [], + keep: ["t1"], + }), + [`${REVIEW}/pr-context.json`]: JSON.stringify({ + number: 1, + repo: "o/r", + }), + }, + ), + ); + const plan = runSubmissionCli(fs); + expect(plan.event).toBe("REQUEST_CHANGES"); + // The depth note rides the body on a reduced run. + expect(plan.body).toContain( + "Note: re-review ran at fast depth (re-review mode full).", + ); + }); + + it("applies the flip floor from the cache-memory stamp when posted bodies carry none (the production shape)", () => { + const fs = makeFakeFs( + staged( + {depth: "fast", claims: []}, + { + // What production priors actually look like: the ingest + // sanitizer stripped the stamp. + [`${REVIEW}/prior-reviews.json`]: JSON.stringify([ + {body: "Changes requested — see inline comments."}, + ]), + [`${REVIEW}/threads.json`]: JSON.stringify([ + { + thread_id: "t1", + path: "a.ts", + line: 2, + comments: [ + { + author: "github-actions[bot]", + body: "**issue (blocking):** still broken", + }, + ], + }, + ]), + [`${REVIEW}/out/thread-reconciler.json`]: JSON.stringify({ + resolve: [], + keep: ["t1"], + }), + [`${REVIEW}/pr-context.json`]: JSON.stringify({ + number: 41007, + repo: "o/r", + }), + "/tmp/gh-aw/cache-memory/pr-41007.json": JSON.stringify({ + verdict: "REQUEST_CHANGES", + stampHunks: {"a.ts": ["deadbeef00000000"]}, + wasDraft: false, + }), + }, + ), + ); + const plan = runSubmissionCli(fs); + expect(plan.event).toBe("REQUEST_CHANGES"); + }); + + it("folds a pr-level claim into the body instead of an inline comment", () => { + const fs = makeFakeFs( + staged({ + depth: "full", + claims: [ + claim({ + path: undefined, + line: undefined, + label: "note (non-blocking)", + }), + ], + }), + ); + const plan = runSubmissionCli(fs); + expect(plan.comments).toEqual([]); + expect(plan.body).toContain( + "**note (non-blocking):** The guard was removed.", + ); + expect(plan.notes.join(" ")).toContain("folded into the review body"); + }); + + it("throws when the dispatcher has not run", () => { + expect(() => runSubmissionCli(makeFakeFs())).toThrow( + /dispatch-result.json not staged/, + ); + }); +}); + +describe("the gate's plan-match rule (slice 4)", () => { + const plannedFs = () => + makeFakeFs( + staged({ + depth: "full", + claims: [claim()], + reconciliation: {resolve: [], keep: []}, + }), + ); + const outFiles = { + "pattern-triage.json": JSON.stringify({reviewFiles: ["a.ts"]}), + "correctness-reviewer.json": "{}", + "claim-validator.json": "{}", + "thread-reconciler.json": JSON.stringify({resolve: [], keep: []}), + }; + + const queuedFromPlan = (plan: { + event: string; + body: string; + comments: {path: string; line: number; body: string}[]; + }) => [ + ...plan.comments.map((comment) => ({ + type: "create_pull_request_review_comment", + ...comment, + })), + { + type: "submit_pull_request_review", + event: plan.event, + body: plan.body, + }, + ]; + + it("passes when the queued outputs match the plan (sanitizer-normalized)", () => { + const plan = runSubmissionCli(plannedFs()); + const result = evaluateDispatchConformance({ + items: queuedFromPlan(plan), + plan: {depth: "full"}, + routing: {enabledReviewers: [], lensesToSpawn: []}, + outFiles, + submissionPlan: plan, + }); + expect(result.violations).toEqual([]); + }); + + it("passes when the ingest sanitizer stripped the plan's stamp comment from the queued body (run 29893634730)", () => { + const plan = runSubmissionCli(plannedFs()); + // The plan's body carries the hidden fingerprint stamp; what the + // gate sees queued is the POST-sanitizer body, comments deleted. + expect(plan.body).toContain("/g, ""); + const result = evaluateDispatchConformance({ + items: queuedFromPlan({...plan, body: sanitizedBody}), + plan: {depth: "full"}, + routing: {enabledReviewers: [], lensesToSpawn: []}, + outFiles, + submissionPlan: plan, + }); + expect(result.violations).toEqual([]); + }); + + it("tolerates the sanitizer's typographic ASCII fold (run 29903306596's ellipsis)", () => { + const fancy = claim({ + discussion: + "composite indexes that are missing from index.yaml \u2026 the order of the \u201cproperties\u201d matters \u2014 it\u2019s direction-sensitive.", + }); + const plan = runSubmissionCli( + makeFakeFs( + staged({ + depth: "full", + claims: [fancy], + reconciliation: {resolve: [], keep: []}, + }), + ), + ); + // What the gate sees queued is the POST-sanitizer body: unicode + // typography folded to ASCII. + const folded = plan.comments.map((comment) => ({ + ...comment, + body: comment.body + .replace(/\u2026/g, "...") + .replace(/[\u201c\u201d]/g, '"') + .replace(/\u2014/g, "-") + .replace(/\u2019/g, "'"), + })); + const result = evaluateDispatchConformance({ + items: queuedFromPlan({...plan, comments: folded}), + plan: {depth: "full"}, + routing: {enabledReviewers: [], lensesToSpawn: []}, + outFiles, + submissionPlan: plan, + }); + expect(result.violations).toEqual([]); + }); + + it("tolerates URL/scheme redaction and template-delimiter escaping (v0.81.6 sanitizer audit)", () => { + // The deployed URL policy is allowed-only and runs inside code + // regions too: a cited non-allowlisted domain (MDN, StackOverflow) + // comes back "(host/redacted)", a blocked scheme "(redacted)", and + // unbackticked template delimiters gain escaping backslashes; none + // of these may false-block a byte-faithful transcription. + const cited = claim({ + discussion: + "See https://developer.mozilla.org/en-US/docs/Web/API/AbortController for the contract; never emit javascript:alert(1) links, and ${{ github.token }} must stay out of run logs.", + }); + const plan = runSubmissionCli( + makeFakeFs( + staged({ + depth: "full", + claims: [cited], + reconciliation: {resolve: [], keep: []}, + }), + ), + ); + const sanitized = plan.comments.map((comment) => ({ + ...comment, + body: comment.body + .replace( + /https:\/\/developer\.mozilla\.org\S+/g, + "(developer.mozilla.org/redacted)", + ) + .replace(/javascript:\S+/g, "(redacted)") + .replace(/\$\{\{/g, "\\$\\{\\{"), + })); + expect(JSON.stringify(sanitized)).not.toBe( + JSON.stringify(plan.comments), + ); + const result = evaluateDispatchConformance({ + items: queuedFromPlan({...plan, comments: sanitized}), + plan: {depth: "full"}, + routing: {enabledReviewers: [], lensesToSpawn: []}, + outFiles, + submissionPlan: plan, + }); + expect(result.violations).toEqual([]); + }); + + it("blocks a spliced body, a flipped event, and a dropped comment", () => { + const plan = runSubmissionCli(plannedFs()); + const splicedBody = evaluateDispatchConformance({ + items: queuedFromPlan({ + ...plan, + body: `${plan.body}\nAlso, everything looks great!`, + }), + plan: {depth: "full"}, + routing: {enabledReviewers: [], lensesToSpawn: []}, + outFiles, + submissionPlan: plan, + }); + expect(splicedBody.violations.map((v) => v.code)).toContain( + "submission-plan-mismatch", + ); + + const flipped = evaluateDispatchConformance({ + items: queuedFromPlan({...plan, event: "APPROVE", comments: []}), + plan: {depth: "full"}, + routing: {enabledReviewers: [], lensesToSpawn: []}, + outFiles, + submissionPlan: plan, + }); + expect(flipped.violations.map((v) => v.code)).toContain( + "submission-plan-mismatch", + ); + + const dropped = evaluateDispatchConformance({ + items: queuedFromPlan({...plan, comments: []}), + plan: {depth: "full"}, + routing: {enabledReviewers: [], lensesToSpawn: []}, + outFiles, + submissionPlan: plan, + }); + expect(dropped.violations.map((v) => v.code)).toContain( + "submission-plan-mismatch", + ); + }); + + it("the redundant-approval skip queues nothing only for an APPROVE plan with no comments", () => { + const approvePlan = runSubmissionCli( + makeFakeFs(staged({depth: "full", claims: []})), + ); + const result = evaluateDispatchConformance({ + items: [], + plan: {depth: "full"}, + routing: {enabledReviewers: [], lensesToSpawn: []}, + outFiles, + submissionPlan: approvePlan, + }); + expect(result.conformant).toBe(true); + // Dropping a REQUEST_CHANGES plan is the withheld-verdict shape and + // blocks (pinned in detail in the hardening suite below). + const rcPlan = runSubmissionCli(plannedFs()); + const dropped = evaluateDispatchConformance({ + items: [], + plan: {depth: "full"}, + routing: {enabledReviewers: [], lensesToSpawn: []}, + outFiles, + submissionPlan: rcPlan, + }); + expect(dropped.conformant).toBe(false); + }); +}); + +describe("re-review hardening (slice 4 feedback)", () => { + const gateInput = (plan: ReturnType) => ({ + plan: {depth: "full"}, + routing: {enabledReviewers: [], lensesToSpawn: []}, + outFiles: { + "pattern-triage.json": JSON.stringify({reviewFiles: ["a.ts"]}), + "correctness-reviewer.json": "{}", + "claim-validator.json": "{}", + }, + submissionPlan: plan, + }); + const rcPlan = () => + runSubmissionCli( + makeFakeFs( + staged({ + depth: "full", + claims: [claim()], + reconciliation: {resolve: [], keep: []}, + }), + ), + ); + + it("blocks queued comments with no submission (the ungated COMMENT review shape)", () => { + const plan = rcPlan(); + const result = evaluateDispatchConformance({ + ...gateInput(plan), + items: plan.comments.map((comment) => ({ + type: "create_pull_request_review_comment", + ...comment, + })), + }); + expect(result.violations.map((v) => v.code)).toContain( + "submission-plan-mismatch", + ); + }); + + it("blocks a silently-dropped REQUEST_CHANGES plan (nothing queued)", () => { + const plan = rcPlan(); + const result = evaluateDispatchConformance({ + ...gateInput(plan), + items: [], + }); + expect(result.violations.map((v) => v.code)).toEqual([ + "submission-plan-mismatch", + ]); + }); + + it("permits queueing nothing only for an APPROVE plan with no comments", () => { + const plan = runSubmissionCli( + makeFakeFs(staged({depth: "full", claims: []})), + ); + const result = evaluateDispatchConformance({ + ...gateInput(plan), + items: [], + }); + expect(result.conformant).toBe(true); + }); + + it("tolerates sanitizer-shaped drift: case, backticks, whitespace, URL rewrites", () => { + const plan = rcPlan(); + const mangle = (text: string): string => + `${text + .toUpperCase() + .replace(/ /g, " ") + .replace( + "GUARD", + "`GUARD` https://evil.example/redirect?x=1", + )}`; + const planWithUrl = { + ...plan, + body: `${plan.body}\nSee https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal for context.`, + }; + const result = evaluateDispatchConformance({ + ...gateInput(planWithUrl), + items: [ + ...plan.comments.map((comment) => ({ + type: "create_pull_request_review_comment", + path: comment.path, + line: comment.line, + body: comment.body.toUpperCase().replace(/ /g, " "), + })), + { + type: "submit_pull_request_review", + event: plan.event, + // The domain-redaction shape the sanitizer actually + // produces for a non-allowlisted host: the host is kept, + // the path is replaced. + body: `${plan.body}\nSee (developer.mozilla.org/redacted) for context.`, + }, + ], + }); + expect( + result.violations.filter( + (v) => v.code === "submission-plan-mismatch", + ), + ).toEqual([]); + // mangle() is used above only for the URL clause; keep the linter + // honest about it. + expect(mangle("guard")).toContain("GUARD"); + }); + + it("tolerates a zero-width or CGJ character the sanitizer stripped", () => { + // hardenUnicodeText deletes zero-width space/non-joiner/joiner, + // the word joiner, the BOM, and the combining grapheme joiner. A + // plan whose prose carries one and a queued copy that dropped it + // are the SAME submission; only the strip arms of normalizeBody + // keep that from reading as a splice. + // Alternation, not a character class: the ZWJ and the CGJ are + // combining/joining characters that no-misleading-character-class + // rejects inside `[...]`. + const ZERO_WIDTH = /\u200b|\u200c|\u200d|\u2060|\ufeff|\u034f/g; + const invisible = claim({ + discussion: + "The\u200bguard\u200cwas\u200dremoved\u2060from\ufeffthe\u034ffast path.", + }); + const plan = runSubmissionCli( + makeFakeFs( + staged({ + depth: "full", + claims: [invisible], + reconciliation: {resolve: [], keep: []}, + }), + ), + ); + const stripped = plan.comments.map((comment) => ({ + ...comment, + body: comment.body.replace(ZERO_WIDTH, ""), + })); + // The queued copy really did lose characters. + expect(stripped[0]?.body).not.toBe(plan.comments[0]?.body); + const result = evaluateDispatchConformance({ + ...gateInput(plan), + items: [ + ...stripped.map((comment) => ({ + type: "create_pull_request_review_comment", + ...comment, + })), + { + type: "submit_pull_request_review", + event: plan.event, + body: plan.body.replace(ZERO_WIDTH, ""), + }, + ], + }); + expect(result.violations).toEqual([]); + }); + + it("blocks a link-target splice that swaps the host", () => { + // URL folding is host-bearing, so the sanitizer's own rewrites pass + // (test above) while an "improvement" that points the reader at a + // different site does not; the #244 splice class. + const plan = rcPlan(); + const planWithUrl = { + ...plan, + body: `${plan.body}\nSee https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal for context.`, + }; + const result = evaluateDispatchConformance({ + ...gateInput(planWithUrl), + items: [ + ...plan.comments.map((comment) => ({ + type: "create_pull_request_review_comment", + ...comment, + })), + { + type: "submit_pull_request_review", + event: plan.event, + body: `${plan.body}\nSee https://stackoverflow.com/questions/1 for context.`, + }, + ], + }); + expect( + result.violations.filter( + (v) => v.code === "submission-plan-mismatch", + ).length, + ).toBe(1); + }); + + it("blocks queueing nothing when the APPROVE plan carries a disclosure note", () => { + // The redundant-approval skip is only for the bare comment-less + // approve body. An APPROVE that shed a lens carries a mandatory + // "not assessed this run" disclosure; dropping that submission would + // withhold both the disclosure and the approval. + const plan = runSubmissionCli( + makeFakeFs( + staged({ + depth: "full", + claims: [], + noteLines: [ + "Note: security-lens not assessed this run (output unavailable).", + ], + }), + ), + ); + expect(plan.event).toBe("APPROVE"); + expect(plan.comments).toEqual([]); + expect(plan.body).toContain("not assessed this run"); + const result = evaluateDispatchConformance({ + ...gateInput(plan), + items: [], + }); + expect(result.violations.map((v) => v.code)).toEqual([ + "submission-plan-mismatch", + ]); + }); + + it("drops a claim on an open human-thread line from the comments AND the verdict", () => { + // review.md Step 5: a bot comment on a line with an open human + // review thread talks over the conversation. Scripted mode has to + // apply the filter here, because rule 7 forbids the orchestrator + // from dropping the comment itself. + const fs = makeFakeFs( + staged({ + depth: "full", + claims: [ + claim({id: "c1", path: "a.ts", line: 2}), + claim({id: "c2", path: "b.ts", line: 7}), + ], + reconciliation: { + resolve: [], + keep: [], + skipLines: [{path: "a.ts", line: 2}], + }, + }), + ); + const plan = runSubmissionCli(fs); + expect(plan.comments.map((comment) => comment.path)).toEqual(["b.ts"]); + expect(plan.notes).toContain( + "claim c1 dropped: open human thread at a.ts:2", + ); + // c2 is still blocking, so the verdict stands on its own. + expect(plan.event).toBe("REQUEST_CHANGES"); + }); + + it("approves when every blocking claim sits on an open human-thread line", () => { + // The verdict counts only the labels on comments that actually + // post; a filtered blocking claim cannot drive REQUEST_CHANGES. + const plan = runSubmissionCli( + makeFakeFs( + staged({ + depth: "full", + claims: [claim({id: "c1", path: "a.ts", line: 2})], + reconciliation: { + resolve: [], + keep: [], + skipLines: [{path: "a.ts", line: 2}], + }, + }), + ), + ); + expect(plan.comments).toEqual([]); + expect(plan.event).toBe("APPROVE"); + }); + + it("ignores malformed skipLines entries rather than crashing", () => { + const plan = runSubmissionCli( + makeFakeFs( + staged({ + depth: "full", + claims: [claim()], + reconciliation: { + resolve: [], + keep: [], + skipLines: ["a.ts:2", {path: "a.ts"}, null, 7], + }, + }), + ), + ); + expect(plan.comments).toHaveLength(1); + expect(plan.event).toBe("REQUEST_CHANGES"); + }); + + it("appends the tripwire note with the 2-decimal share", () => { + const fs = makeFakeFs({ + [`${REVIEW}/dispatch-result.json`]: JSON.stringify({ + depth: "full", + claims: [], + }), + [`${REVIEW}/rereview-plan.json`]: JSON.stringify({ + depth: "full", + mode: "scoped", + tripwireRearmed: true, + divergence: {unreviewedShare: 0.4567}, + stampAnchorDraft: false, + stampHunks: {}, + }), + }); + const plan = runSubmissionCli(fs); + expect(plan.body).toContain( + "Note: divergence tripwire re-armed a full review (unreviewed share 0.46).", + ); + }); + + it("keeps a blank line inside a rule-quote blockquote", () => { + const body = renderClaimComment( + claim({rule_quote: "First.\n\nSecond."}) as never, + ); + expect(body).toContain("> **Rule:** First.\n>\n> Second."); + }); + + it("renderClaimComment matches renderComment byte-for-byte on the same finding", () => { + const finding = { + schema_version: 2, + id: "f1", + lens: "correctness", + anchor: {type: "line", path: "a.ts", line: 2, side: "RIGHT"}, + severity: "blocking", + confidence: 0.9, + evidence_trace: ["a.ts:2"], + failure_scenario: "fails", + producing_hunt: "h", + model_authored_prose: "The guard was removed.", + rule_quote: "Always guard.\n\nEven here.", + suggested_patch: "guard()", + } as never; + const canonical = renderComment(finding); + const viaClaim = renderClaimComment( + claim({ + label: labelForFinding(finding), + discussion: "The guard was removed.", + rule_quote: "Always guard.\n\nEven here.", + suggestion: "guard()", + }) as never, + ); + expect(viaClaim).toBe(canonical); + }); +}); diff --git a/workflows/review/lib/submission.ts b/workflows/review/lib/submission.ts new file mode 100644 index 00000000..668053da --- /dev/null +++ b/workflows/review/lib/submission.ts @@ -0,0 +1,412 @@ +/** + * The submission plan (deterministic-orchestrator slice 4, the probe): Steps + * 4-6 as code. Given the dispatcher's validated claims (slice 2), this CLI + * computes the verdict, renders every inline comment and the full review + * body (accountability section, note lines, fingerprint stamp), and stages + * `submission-plan.json`; the orchestrator's remaining job is to emit safe + * outputs that match the plan verbatim, and the dispatch-conformance gate + * blocks a submission that does not (the #244 accountability-splice check, + * as code). + * + * This is the end-state shape the migration plan names (the no-post runner's + * pipeline in production): staging (slice 1) → dispatch/validation (slice 2) + * → verdict/render/plan (here) → emit → gate. What remains model work is the + * sub-agents themselves plus the safe-output EMISSION, and the reason is a + * filesystem permission, not a credential: queueing a safe output is an + * append to `$GH_AW_SAFE_OUTPUTS` + * (`${RUNNER_TEMP}/gh-aw/safeoutputs/outputs.jsonl`) and needs no token at + * queue time (the credentialed posting is the later `safe_outputs` job). + * What this CLI cannot do is write that file: the agent sandbox mounts + * `${RUNNER_TEMP}/gh-aw` read-only (`awf --mount ...:ro` in the compiled + * lock; only `safeoutputs/upload-artifacts` is rw), while the safeoutputs + * MCP container gets `safeoutputs/` read-write. So the seam is real today + * but likely cheaper to delete than the plan doc's Q1 note recorded: it + * wants a writable path into the queue: an upstream mount/ingest change, + * or a repo-controlled post-agent step writing the ingested queue on the + * host the way this workflow's gate already rewrites it. Untested either + * way; until one is proven the orchestrator is a typist for MCP calls, and + * the gate makes mis-typing a red run. + * + * Verdict rules encoded (review.md Step 4, mechanically): + * - Claims anchored on a line in the reconciler's `skipLines` are dropped + * first (Step 5's defer-to-open-human-threads rule), so they neither + * post nor count toward the verdict. + * - REQUEST_CHANGES iff at least one posted claim carries a blocking label + * (via computeVerdict, threshold 1). + * - 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. + * + * Body rules encoded (review.md Step 6): the verdict head (empty-body + * APPROVE with comments; the fixed REQUEST_CHANGES line), the code-rendered + * accountability section spliced verbatim, one note line per shed / skipped + * dimension / depth reduction (the dispatcher already rendered those), any + * PR-level claims folded into the body (the inline-comment safe output needs + * a path and line), and the hidden fingerprint stamp as the final line. + * + * Determinism boundary: pure composition of staged files through the same + * lib functions the eval runner uses; no model call, no prose about the code + * under review. + */ + +import type {Claim} from "./dispatch-contracts"; +import {renderReviewBody} from "./render-comment"; +import {runRereviewCli, type RereviewCliFs} from "./rereview"; +import { + findLatestStamp, + runRereviewStampCli, + stampFromCacheMemory, + type PriorReview, +} from "./rereview-mode"; +import {computeVerdict} from "./verdict"; + +/* -------------------------------------------------------------------------- */ +/* Types and paths */ +/* -------------------------------------------------------------------------- */ + +const REVIEW_DIR = "/tmp/gh-aw/review"; +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. */ + body: string; + /** The inline comments to post, one safe output each, verbatim. */ + comments: PlannedComment[]; + /** 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[]; + /** Non-blocking composition observations. */ + notes: string[]; +}; + +export type SubmissionFs = RereviewCliFs; + +/** + * The reconciler's open-human-thread lines as `path:line` keys (review.md + * Step 5's "defer to open human threads"). Anything unparseable is dropped: + * the filter degrades to posting, never to a crash. + */ +const parseSkipLines = (raw: unknown): Set => { + const keys = new Set(); + if (!Array.isArray(raw)) { + return keys; + } + for (const entry of raw) { + const {path, line} = (entry ?? {}) as {path?: unknown; line?: unknown}; + if (typeof path === "string" && typeof line === "number") { + keys.add(`${path}:${line}`); + } + } + return keys; +}; + +const readJson = (fs: SubmissionFs, path: string): unknown => { + if (!fs.existsSync(path)) { + return undefined; + } + try { + return JSON.parse(fs.readFileSync(path, "utf8")); + } catch { + return undefined; + } +}; + +/** The Step 9 cache record for this PR (pr number from pr-context.json). */ +const readCacheMemoryRecord = (fs: SubmissionFs): unknown => { + const prContext = readJson(fs, `${REVIEW_DIR}/pr-context.json`) as + | {number?: unknown} + | undefined; + if (typeof prContext?.number !== "number") { + return undefined; + } + return readJson(fs, `${CACHE_MEMORY_DIR}/pr-${prContext.number}.json`); +}; + +/* -------------------------------------------------------------------------- */ +/* Rendering */ +/* -------------------------------------------------------------------------- */ + +/** + * How many lines a committable suggestion may replace the anchored line + * with; anything longer is a sketch, not a drop-in. + */ +const MAX_SUGGESTION_LINES = 8; + +const lineHasCodeSignal = (line: string): boolean => + /\w\(/.test(line) || // a call + /[{};]/.test(line) || // block/statement punctuation + /:=|=>|->/.test(line) || // assignment/arrow operators + /^\s*(\/\/|#|\/\*|\*)/.test(line) || // a comment marker + /^\t/.test(line); // code-convention indentation + +const looksLikeProse = (line: string): boolean => { + // Deliberately NOT vetoed by lineHasCodeSignal: run 29901690493 posted + // "Use ctx.Time().Now().AddDate(0, 0, -MemoryTTLDays), and add a test + // that ..." as a committable fence because the embedded call defeated + // the prose check. A sentence that names code is still a sentence. + const words = line.trim().split(/\s+/); + if (words.length < 6) { + return false; + } + const plain = words.filter((word) => + /^\(?[A-Za-z][A-Za-z']*[.,;:!?)]?$/.test(word), + ); + return plain.length / words.length >= 0.75; +}; + +/** + * Whether a claim's suggestion is plausibly a committable replacement of + * the anchored line: small and code-shaped. Trial run 29897276810 posted an + * English sentence and a 30-line test function inside `suggestion` fences + * (Khan/webapp#41009 comments r3628128268 / r3628128224), both of which a + * single click would have committed verbatim into the file. + */ +export const isDropInSuggestion = (suggestion: string): boolean => { + const lines = suggestion.replace(/\n$/, "").split("\n"); + const content = lines.filter((line) => line.trim() !== ""); + if (content.length === 0 || lines.length > MAX_SUGGESTION_LINES) { + return false; + } + return content.some(lineHasCodeSignal) && !content.some(looksLikeProse); +}; + +/** + * Render one claim as its Conventional Comment (the renderComment layout, + * driven by the claim's post-validation label rather than a recomputed one). + * A suggestion only becomes a committable `suggestion` fence when it is + * plausibly drop-in; otherwise it renders as a plain fenced sketch. + */ +export const renderClaimComment = (claim: Claim): string => { + const lines: string[] = [`**${claim.label}:** ${claim.discussion}`]; + if (claim.rule_quote !== undefined) { + const [first, ...rest] = claim.rule_quote.split("\n"); + lines.push( + "", + `> **Rule:** ${first}`, + ...rest.map((line) => (line === "" ? ">" : `> ${line}`)), + ); + } + if (claim.suggestion !== undefined) { + if (isDropInSuggestion(claim.suggestion)) { + lines.push("", "```suggestion", claim.suggestion, "```"); + } else { + lines.push( + "", + "A sketch, not a committable replacement:", + "", + "````", + claim.suggestion, + "````", + ); + } + } + return lines.join("\n"); +}; + +/* -------------------------------------------------------------------------- */ +/* The plan */ +/* -------------------------------------------------------------------------- */ + +/** + * Compose the submission plan from the staged dispatch result. Factored out + * (fs injected) so it is testable without touching the real filesystem. + * Writes `submission-plan.json` (and, via the rereview CLI it invokes, + * `rereview.json`). Returns what was written. + */ +export const runSubmissionCli = (fs: SubmissionFs): SubmissionPlan => { + const notes: string[] = []; + const dispatch = readJson(fs, `${REVIEW_DIR}/dispatch-result.json`) as + | { + claims?: unknown; + noteLines?: unknown; + reconciliation?: {resolve?: unknown; skipLines?: unknown}; + depth?: unknown; + } + | undefined; + if (dispatch === undefined) { + throw new Error( + `dispatch-result.json not staged under ${REVIEW_DIR}: run the dispatcher first`, + ); + } + const validated = ( + Array.isArray(dispatch.claims) ? dispatch.claims : [] + ) as Claim[]; + // Defer to open human threads (review.md Step 5): drop any claim anchored + // on a line the reconciler flagged, silently; a human conversation is + // already open there and a bot comment would talk over it. This runs + // BEFORE the verdict, because Step 4 counts only the labels on comments + // that actually post; rule 7 then forbids the orchestrator from dropping + // anything itself, so the filter has to live here or not at all. + const skipLines = parseSkipLines(dispatch.reconciliation?.skipLines); + const claims = validated.filter((claim) => { + const skipped = + claim.path !== undefined && + claim.line !== undefined && + skipLines.has(`${claim.path}:${claim.line}`); + if (skipped) { + notes.push( + `claim ${claim.id} dropped: open human thread at ${claim.path}:${claim.line}`, + ); + } + return !skipped; + }); + const noteLines = Array.isArray(dispatch.noteLines) + ? dispatch.noteLines.filter( + (line): line is string => typeof line === "string", + ) + : []; + const depth = typeof dispatch.depth === "string" ? dispatch.depth : "full"; + + // The accountability section (renders and stages rereview.json too). + const rereview = runRereviewCli(fs); + + // The reduced-depth flip floor (Step 4): only over a prior + // REQUEST_CHANGES stamp at flip-gated/fast depth. + let keptBlockingFloor = 0; + if (depth === "flip-gated" || depth === "fast") { + const priorRaw = readJson(fs, `${REVIEW_DIR}/prior-reviews.json`); + const priors: PriorReview[] = Array.isArray(priorRaw) + ? priorRaw.filter( + (entry): entry is PriorReview => + typeof (entry as {body?: unknown}).body === "string", + ) + : []; + // Posted bodies never keep their stamp (the ingest sanitizer strips + // HTML comments), so the floor anchors on the same cache-memory + // carrier the plan CLI and gate rule 5 read. + const stamp = + findLatestStamp(priors) ?? + stampFromCacheMemory(readCacheMemoryRecord(fs)); + if (stamp !== null && stamp.verdict === "REQUEST_CHANGES") { + keptBlockingFloor = rereview.keptBlockingCount; + } + } + + // Inline comments need a path and a line; a PR-level claim folds into + // the body instead (rare: a pr-anchored finding). + const inline: PlannedComment[] = []; + const prLevelLines: string[] = []; + for (const claim of claims) { + if (claim.path !== undefined && claim.line !== undefined) { + inline.push({ + path: claim.path, + line: claim.line, + body: renderClaimComment(claim), + }); + } else { + prLevelLines.push(`**${claim.label}:** ${claim.discussion}`); + notes.push( + `pr-level claim ${claim.id} folded into the review body`, + ); + } + } + + const verdict = computeVerdict({ + postedLabels: claims.map((claim) => claim.label), + dimensions: { + correctness: "assessed", + skillSeverity: "assessed", + patternTriage: "assessed", + }, + keptBlockingCount: keptBlockingFloor, + }); + // 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}).`, + ); + } + 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, + rereviewSection: rereview.section, + }); + const stamp = runRereviewStampCli(fs, event); + const body = [head, ...prLevelLines, ...noteLines, ...depthNotes] + .filter((line) => line !== "") + .join("\n") + .concat(stamp === null ? "" : `\n${stamp}`) + .replace(/^\n+/, ""); + + const submission: SubmissionPlan = { + event, + body, + comments: inline, + resolve: Array.isArray(dispatch.reconciliation?.resolve) + ? dispatch.reconciliation.resolve.filter( + (id): id is string => typeof id === "string", + ) + : [], + 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 +// mode), never on import (tests). +if (typeof require !== "undefined" && require.main === module) { + const fs = require("node:fs") as SubmissionFs; + try { + const plan = runSubmissionCli(fs); + // eslint-disable-next-line no-console + console.log( + JSON.stringify( + { + event: plan.event, + comments: plan.comments.length, + resolve: plan.resolve.length, + reasons: plan.reasons, + }, + null, + 2, + ), + ); + } catch (error) { + // eslint-disable-next-line no-console + console.error( + `::error title=review submission plan::${ + error instanceof Error ? error.message : String(error) + }`, + ); + process.exit(1); + } +} diff --git a/workflows/review/review.md b/workflows/review/review.md index 21f56b4b..472d35e9 100644 --- a/workflows/review/review.md +++ b/workflows/review/review.md @@ -682,17 +682,36 @@ cd gh-aw-review-lib && REVIEW_REPO_ROOT="$GITHUB_WORKSPACE" \ sheds computed from `routing.json`, every dispatch staged to `out/.json`), the provenance gate, the scope filter, and claim validation, and writes `/tmp/gh-aw/review/dispatch-result.json`. -4. Read `dispatch-result.json` and act on it: `claims` is the validated - candidate set Steps 4-6 act on (already gated, scoped, and validated; - never re-derive or second-guess it); `reconciliation.resolve` is the list - to resolve with `resolve-pull-request-review-thread` safe outputs (batch - them in one turn) and `reconciliation.skipLines` the lines Step 5 must not - comment on; `noteLines` are Step 6 note lines to append verbatim; - `riskFiles`, `patterns`, and `excludedFiles` feed Steps 7 and 8. Then go - straight to Step 4. Do not dispatch any sub-agent yourself in this mode, - and do not re-run the dispatcher; if its call failed, treat the run as - over budget and land the review from whatever `out/` evidence exists (the - dispatch-conformance gate decides whether a verdict may post). +4. Compose the submission deterministically, once: +``` +cd gh-aw-review-lib && npx -y tsx workflows/review/lib/submission.ts +``` + It reads `dispatch-result.json`, renders the accountability section + (`rereview.json`), computes the verdict (Step 4's mechanical rule plus the + reduced-depth flip floor), renders every inline comment and the full + review body (note lines and fingerprint stamp included), and writes + `/tmp/gh-aw/review/submission-plan.json`. +5. Emit the safe outputs **exactly** as the plan says, nothing more and + nothing less: one `create-pull-request-review-comment` per `comments` + entry (its `path`, `line`, and `body` verbatim), one + `resolve-pull-request-review-thread` per `resolve` id (batched in one + turn), and one `submit-pull-request-review` with the plan's `event` and + `body` verbatim. The redundant-approval skip still applies, restated here + since you skip Step 6's text in this mode: only when the plan's `event` is + APPROVE with zero `comments`, the plan's `body` carries no `Note:` lines + and no accountability section, and the PR's most recent + `github-actions[bot]` review is already APPROVED, emit no submission at + all (the gate permits queueing nothing exactly for that shape). 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`, + `patterns`, and `excludedFiles` feed Steps 7 and 8 as usual; + `reconciliation.skipLines` is already reflected in the plan. Then skip + Steps 4-6 below entirely (the plan IS Steps 4-6) and continue at Step 7. + Do not dispatch any sub-agent yourself in this mode, and do not re-run + the dispatcher; if its call failed, treat the run as over budget and land + the review from whatever `out/` evidence exists (the gate decides whether + a verdict may post). Everything from "Phase 1" below to the end of Phase 3 applies only to the default `task` dispatch mode.