review: a downvoted finding joins the adjudicated corpus; document the feedback contract

.changeset/review-adjudicated-thread-suppression.md

@@ -0,0 +1,5 @@
+---
+"review": minor
+---
+
+Resolving a bot thread now means "settled", not "open season for a rephrase". The staging collects the bot's threads a HUMAN resolved into a new adjudicated corpus (`adjudicated-threads.json`), and the dispatcher suppresses any non-blocking candidate that re-derives a defect that corpus already settled (same defect-identity match as open-thread suppression). Previously, resolution removed the thread from the only suppression corpus, so the next run could re-post the same concern with fresh wording as a brand-new thread, which every later accountability recap then reported as "still unaddressed" (webapp#41290: six resolved variants of one concern, then a seventh). Two safety asymmetries: a thread the BOT resolved (the reconciler, after a fix) never joins the corpus, and a BLOCKING candidate is never suppressed by it, so a fixed-then-regressed defect worth stopping the PR for always posts.

.changeset/review-downvote-adjudication.md

@@ -0,0 +1,5 @@
+---
+"review": minor
+---
+
+A 👎 on a bot finding now adjudicates it, exactly like resolving its thread: the staging reads each thread opener's THUMBS_DOWN reaction count, and a bot-opened thread with a downvoted opener joins the adjudicated suppression corpus whatever its resolution state, so the settled defect cannot re-post under fresh wording (blocking re-flags still always post, and a still-open downvoted thread stays in the open corpus, which keeps the verdict-floor bookkeeping). Previously the downvote channel the bot advertises (the thumbs sweep asks "why?" on exactly this signal) dead-ended in counters and changed nothing about what posts. Also documents the full feedback signal contract (reply / resolve / 👎 / hide) in the README.

workflows/review/README.md

@@ -104,7 +104,11 @@ CLI invocation (`lib/dispatch.ts`) that runs triage, the reviewer fan-out
 (roster, budget cap, and planned sheds computed from `routing.json`), the
 provenance gate, the scope filter, cross-source dedup, open-thread suppression
 (a candidate describing a defect an open bot thread already tracks posts no
-duplicate; a suppressed blocking candidate still floors the verdict), and
+duplicate; a suppressed blocking candidate still floors the verdict),
+adjudicated-thread suppression (a non-blocking candidate re-deriving a defect
+a human settled by resolving the bot's thread or downvoting its opener posts
+no new thread; a blocking candidate is never suppressed this way, so a
+regression re-flag stays visible), and
 claim validation, inside the same firewall sandbox (the api-proxy meters and
 caps script-spawned sub-agents exactly like Task-spawned ones). Each sub-agent
 delivers its result through an in-process `submit_result` MCP tool whose input
@@ -125,6 +129,42 @@ run-local JSONL append that needs no credentials, but the agent sandbox mounts
 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.
 
+## What your feedback does
+
+The reviewer reads four signals off its own threads. What each one means, so
+you can pick the one that says what you mean:
+
+- **Reply to the thread.** Read in full: the reconciler sees every reply
+  chain verbatim, and the orchestrator surfaces replies that factually
+  dispute a finding to claim validation. But a reply alone does not close
+  anything: the reconciler resolves a thread when the CODE changes to address
+  it, so arguing a finding down in prose and pushing nothing leaves it open
+  (and blocking, if it was blocking).
+- **Resolve the thread.** "This is settled." The thread leaves the
+  accountability recap, and the defect joins the adjudicated corpus: a later
+  run that re-derives the same defect (any wording, any nearby line) posts
+  nothing, unless it comes back at BLOCKING severity, which always posts (a
+  regression worth stopping the PR for must never be silenced by an old
+  resolution). Threads the bot resolved itself (because a push fixed them)
+  do not join the corpus; a fixed defect that reappears is a fresh finding.
+- **👎 the finding's comment.** Same adjudication as resolving, through the
+  reaction channel: a 👎 on a thread's OPENING comment puts its defect in the
+  adjudicated corpus whether or not you also resolve. The feedback sweep may
+  additionally ask one follow-up ("why?"), which calibrates the eval suite;
+  answering it is welcome but the 👎 alone is what suppresses. Reactions on
+  replies are conversation, not adjudication. 👎 is the ONLY adjudicating
+  reaction: a 😕 triggers the sweep's follow-up question like a 👎 does, but
+  it does not suppress (😕 reads as "unclear", not "wrong", and ambiguity is
+  worth a question, not a standing suppression). The bot's own seeded nudge
+  reactions never count as adjudication either.
+- **Hide the comment.** Reads as nothing. The reviewer does not see hidden
+  state; resolve or 👎 instead.
+
+Per-PR opt-out and re-runs are consumer-trigger concerns: repos using the
+stock push trigger skip any PR carrying the `skip-ai-review` label, and
+consumers with comment triggers or shims should honor the same label (see
+Khan/webapp's kore shim).
+
 ## Install
 
 ```sh

workflows/review/lib/dedup-adjudicated.test.ts

@@ -0,0 +1,240 @@
+import {describe, it, expect} from "vitest";
+
+import {
+    adjudicatedThreadsFromStaged,
+    suppressAdjudicatedDuplicates,
+    suppressTrackedDuplicates,
+} from "./dedup-adjudicated";
+import type {Claim} from "./dispatch-contracts";
+
+/**
+ * Adjudicated-thread suppression tests, split from dedup.test.ts for its
+ * max-lines budget (the dedup-cluster.test.ts precedent); the `claim` factory
+ * mirrors that file's. The scenario throughout is webapp#41290's: a human
+ * resolved the bot's thread, and a later run re-derived the same defect with
+ * fresh wording at a nearby line, which the adjudicated corpus must absorb
+ * without ever absorbing a blocking regression re-flag.
+ */
+
+const claim = (over: Partial<Claim> & {id: string; source: string}): Claim => ({
+    path: "services/ai-guide/memory/expiration.go",
+    line: 38,
+    label: "issue (blocking)",
+    subject: "s",
+    discussion: "d",
+    failure_scenario: "f",
+    confidence: 0.7,
+    ...over,
+});
+
+describe("adjudicatedThreadsFromStaged", () => {
+    const adjudicated = (over: Record<string, unknown> = {}) => ({
+        thread_id: "T1",
+        path: "a.ts",
+        resolved: true,
+        resolvedBy: "sxkosone",
+        comments: [
+            {
+                author: "github-actions",
+                body: "**suggestion (non-blocking):** opener",
+            },
+        ],
+        ...over,
+    });
+
+    it("admits only bot-opened threads a human resolved", () => {
+        expect(adjudicatedThreadsFromStaged([adjudicated()])).toEqual([
+            {
+                thread_id: "T1",
+                path: "a.ts",
+                body: "**suggestion (non-blocking):** opener",
+            },
+        ]);
+    });
+
+    it("admits a bot thread whose opener a reviewer downvoted, whatever its resolution state", () => {
+        // The 👎 channel: the same judgment as resolving, delivered through
+        // the reaction the thumbs sweep advertises. Resolution state does not
+        // gate it; a still-open downvoted thread is also in the open corpus,
+        // and the composed pass attributes a double match to the open thread.
+        for (const state of [
+            {resolved: false, resolvedBy: ""},
+            {resolved: true, resolvedBy: "github-actions"},
+            {resolved: undefined, resolvedBy: undefined},
+        ]) {
+            expect(
+                adjudicatedThreadsFromStaged([
+                    adjudicated({...state, openerDownvotes: 1}),
+                ]),
+            ).toEqual([
+                {
+                    thread_id: "T1",
+                    path: "a.ts",
+                    body: "**suggestion (non-blocking):** opener",
+                },
+            ]);
+        }
+    });
+
+    it("fails closed on every guard: unresolved, bot-resolved, unattributable resolver, human opener, malformed staging", () => {
+        // Each rejected shape degrades to a duplicate comment, never to a
+        // suppression the staging cannot justify: this corpus grants the
+        // strongest suppression in the pipeline (a human's explicit "settled"
+        // outlives rephrasings), so membership must be unmanufacturable.
+        const rejected: unknown[] = [
+            adjudicated({resolved: false}),
+            adjudicated({resolved: undefined}),
+            adjudicated({resolved: "true"}),
+            // A downvote count must be an explicit positive number: absent,
+            // zero, or malformed reads as no downvote, and a downvote alone
+            // never launders a thread that fails the bot-opener guard.
+            adjudicated({resolved: false, openerDownvotes: 0}),
+            adjudicated({resolved: false, openerDownvotes: "1"}),
+            adjudicated({
+                resolved: false,
+                openerDownvotes: 1,
+                comments: [{author: "jwbron", body: "human opener"}],
+            }),
+            // The bot resolving its own thread is the reconciler marking a
+            // defect FIXED; a fixed defect that reappears is a fresh finding.
+            adjudicated({resolvedBy: "github-actions"}),
+            adjudicated({resolvedBy: "github-actions[bot]"}),
+            adjudicated({resolvedBy: ""}),
+            adjudicated({resolvedBy: undefined}),
+            adjudicated({
+                comments: [{author: "jwbron", body: "human opener"}],
+            }),
+            adjudicated({comments: []}),
+            adjudicated({thread_id: undefined}),
+            "not a record",
+        ];
+        for (const thread of rejected) {
+            expect(adjudicatedThreadsFromStaged([thread])).toEqual([]);
+        }
+        expect(adjudicatedThreadsFromStaged(undefined)).toEqual([]);
+        expect(adjudicatedThreadsFromStaged({not: "an array"})).toEqual([]);
+    });
+});
+
+describe("suppressAdjudicatedDuplicates", () => {
+    const adjudicatedThread = (over: Record<string, unknown> = {}) => ({
+        thread_id: "T-adj",
+        path: "services/ai-guide/memory/expiration.go",
+        body: "**suggestion (non-blocking):** No test exercises the deletion path: TestExpiration only asserts that expired keys are identified, so a regression that identifies but never deletes expired memories stays green.",
+        ...over,
+    });
+    const rederivation = (over: Partial<Claim> = {}) =>
+        claim({
+            id: "correctness-reviewer-2",
+            source: "correctness-reviewer",
+            line: 42,
+            label: "suggestion (non-blocking)",
+            subject:
+                "Missing deletion test: the expiration path has no test covering the delete.",
+            discussion:
+                "No test exercises the deletion path; TestExpiration asserts expired keys are identified but a regression that never deletes expired memories stays green.",
+            failure_scenario:
+                "A regression that identifies expired memories but skips the deletion is not caught by TestExpiration and ships green.",
+            ...over,
+        });
+
+    it("suppresses a non-blocking re-derivation of an adjudicated defect, marked as adjudicated", () => {
+        const {kept, suppressed} = suppressAdjudicatedDuplicates(
+            [rederivation()],
+            [adjudicatedThread()],
+        );
+        expect(kept).toEqual([]);
+        expect(suppressed).toEqual([
+            {
+                id: "correctness-reviewer-2",
+                source: "correctness-reviewer",
+                label: "suggestion (non-blocking)",
+                path: "services/ai-guide/memory/expiration.go",
+                line: 42,
+                thread_id: "T-adj",
+                threadBlocking: false,
+                adjudicated: true,
+            },
+        ]);
+    });
+
+    it("never suppresses a blocking candidate: a regression re-flag must stay visible", () => {
+        // The adjudicated thread is closed and floors nothing, so suppressing
+        // a blocker on it would let a re-confirmed blocking defect vanish
+        // without a trace. This asymmetry is also the regression escape
+        // hatch: a fixed-then-regressed defect worth stopping the PR for
+        // re-presents at blocking severity and posts.
+        const blocking = rederivation({label: "issue (blocking)"});
+        const {kept, suppressed} = suppressAdjudicatedDuplicates(
+            [blocking],
+            [adjudicatedThread()],
+        );
+        expect(kept).toEqual([blocking]);
+        expect(suppressed).toEqual([]);
+    });
+
+    it("keeps unrelated and pathless claims, and everything when the corpus is empty", () => {
+        const unrelated = rederivation({
+            subject: "Retention window subtracts months, not days.",
+            discussion:
+                "AddDate(0, -MemoryTTLDays, 0) subtracts 180 months so the window never expires anything.",
+            failure_scenario:
+                "Memories never expire because the cutoff is 15 years in the past.",
+        });
+        const pathless = rederivation({path: undefined, line: undefined});
+        const empty = suppressAdjudicatedDuplicates([rederivation()], []);
+        expect(empty.kept).toHaveLength(1);
+        expect(empty.suppressed).toEqual([]);
+        const {kept, suppressed} = suppressAdjudicatedDuplicates(
+            [unrelated, pathless],
+            [adjudicatedThread()],
+        );
+        expect(kept).toEqual([unrelated, pathless]);
+        expect(suppressed).toEqual([]);
+    });
+
+    it("attributes a candidate matching BOTH corpora to the OPEN thread (the verdict floor reads its blocking state)", () => {
+        // The composed pass order is the guarantee dispatch.ts relies on: the
+        // open corpus runs first, so a defect that is simultaneously tracked
+        // by an open thread and settled on an older resolved one suppresses
+        // against the OPEN thread, whose blocking state floors the verdict.
+        const openStaged = [
+            {
+                thread_id: "T-open",
+                path: "services/ai-guide/memory/expiration.go",
+                resolved: false,
+                comments: [
+                    {
+                        author: "github-actions",
+                        body: adjudicatedThread().body,
+                    },
+                ],
+            },
+        ];
+        const adjudicatedStaged = [
+            {
+                thread_id: "T-adj",
+                path: "services/ai-guide/memory/expiration.go",
+                resolved: true,
+                resolvedBy: "octo",
+                comments: [
+                    {
+                        author: "github-actions",
+                        body: adjudicatedThread().body,
+                    },
+                ],
+            },
+        ];
+        const result = suppressTrackedDuplicates(
+            [rederivation()],
+            openStaged,
+            adjudicatedStaged,
+            new Set(),
+        );
+        expect(result.kept).toEqual([]);
+        expect(result.suppressed).toHaveLength(1);
+        expect(result.suppressed[0].thread_id).toBe("T-open");
+        expect(result.suppressed[0].adjudicated).toBeUndefined();
+        expect(result.shapeFailure).toBeUndefined();
+    });
+});