review: hold for a human instead of approving when core lenses produced no output #338

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

jwbron merged 2 commits into main from jwies-review-hold-for-human

Aug 13, 2026

.changeset/hold-for-human-dead-core-lenses.md

-Original file line number
+Diff line change
@@ -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.

workflows/review/lib/cache-record.test.ts

-Original file line number
+Diff line change
@@ Expand Up @@
             expect(record.risksPatternsKey).toBe("prior-key");
         });
     });
+    describe("the hold record (HOLD_FOR_HUMAN plans)", () => {
+        const holdStaged = (
+            over: Record<string, string> = {},
+        ): Record<string, string> => ({
+            ...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);
+        });
+    });

workflows/review/lib/cache-record.ts

-Original file line number
+Diff line change
@@ Expand Up / @@ -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");
         }
@@ Expand Down @@

Navigation Menu

Uh oh!

review: hold for a human instead of approving when core lenses produced no output #338

Uh oh!

Diff view

Diff view

There are no files selected for viewing

Uh oh!

Uh oh!

Uh oh!