edobry
diff --git a/‎services/reviewer/scripts/verify-review-domain-deps.ts‎
Lines changed: 195 additions & 0 deletions b/‎services/reviewer/scripts/verify-review-domain-deps.ts‎
Lines changed: 195 additions & 0 deletions
diff --git a/‎services/reviewer/src/domain-container.ts‎
Lines changed: 61 additions & 0 deletions b/‎services/reviewer/src/domain-container.ts‎
Lines changed: 61 additions & 0 deletions
diff --git a/‎services/reviewer/src/server.ts‎
Lines changed: 15 additions & 24 deletions b/‎services/reviewer/src/server.ts‎
Lines changed: 15 additions & 24 deletions
@@ -0,0 +1,195 @@
+#!/usr/bin/env bun
+/**
+ * Live binding check for the review's domain context (mt#4998).
+ *
+ * ## What only a live run can answer
+ *
+ * `sweeper.test.ts`'s mt#4998 block proves the deps are FORWARDED — it hands
+ * `retriggerViaRunReview` opaque sentinels and asserts they arrive at
+ * `runReview`. That is a seam-injected test, and per
+ * `/implement-task` §7 item 8 (binding direction) a seam-injected test is not
+ * evidence for the REAL-WIRED binding of the seam. It would pass identically
+ * if `buildReviewDomainDeps` returned five nulls against a live container.
+ *
+ * That distinction is not hypothetical here. Every dep in this set is
+ * `?: T | null` and degrades SILENTLY when absent — a null `taskService`
+ * produces `specVerification: []`, a null `persistenceProvider` produces
+ * `Tier: unknown`, and the review posts and looks normal either way. A
+ * never-worked binding is therefore indistinguishable from "this PR has no
+ * bound task" at every downstream surface, which is exactly the fail-open
+ * shape §7 item 8 flags as the extra red flag. It is also how the original
+ * defect survived in production unnoticed.
+ *
+ * So this script boots the REAL domain container, runs the REAL
+ * `buildReviewDomainDeps`, and then EXERCISES the two deps whose absence
+ * caused the observed symptoms rather than merely null-checking them.
+ *
+ * ## What it deliberately does NOT cover
+ *
+ * It does not drive a review. Whether a sweeper-initiated review ultimately
+ * posts a populated `specVerification` array depends on the model as well as
+ * the wiring, and forcing a real sweeper retrigger requires a genuinely
+ * missed review in production, which cannot be manufactured pre-merge. This
+ * verifies the half that is ours: that the container yields live, working
+ * deps and that the builder passes them through. The remaining half is named
+ * UNVERIFIED in the PR body rather than implied.
+ *
+ * It also does not exercise `memoryLookup` / `askLookup` / `sessionLookup`
+ * beyond a null check — those resolve `mem#N` / `ask#N` / `ws#N` references
+ * that only appear in some specs, they were never implicated in the observed
+ * symptoms, and calling them would require a record known to exist. Stated
+ * rather than implied, because a check that looks total and is not is worse
+ * than one whose bound is written down.
+ *
+ * ## Usage
+ *
+ *   bun services/reviewer/scripts/verify-review-domain-deps.ts [--task=mt#4998] [--require]
+ *
+ * Exits 0 when every dep is present and both exercised deps respond, 2 on any
+ * failure, and 0 with a SKIP notice when the domain container cannot boot
+ * (no persistence configured — safe in an unconfigured CI job). Pass
+ * `--require` to turn that SKIP into an exit 2 instead. Writes
+ * `services/reviewer/scripts/verify-review-domain-deps-results.json`.
+ */
+
+// The domain container resolves tsyringe-decorated classes, which need the
+// reflect-metadata polyfill installed before any of them is constructed —
+// same reason `server.ts:16` and the other container-booting scripts import
+// it first. Without this the boot throws a polyfill error that this script's
+// catch would report as a SKIP, which reads exactly like "no persistence
+// configured" and would make the check silently incapable of passing.
+import "reflect-metadata";
+import { writeFileSync } from "node:fs";
+import { join, dirname } from "node:path";
+import { fileURLToPath } from "node:url";
+import { bootDomainContainer, buildReviewDomainDeps } from "../src/domain-container";
+
+const RESULTS_PATH = join(
+  dirname(fileURLToPath(import.meta.url)),
+  "verify-review-domain-deps-results.json"
+);
+
+/**
+ * The keys `buildReviewDomainDeps` must populate. Declared here rather than
+ * imported from `sweeper.ts` so this script fails if the two ever diverge —
+ * an independent statement of the contract, not an echo of it.
+ */
+const REQUIRED_KEYS = [
+  "taskService",
+  "persistenceProvider",
+  "memoryLookup",
+  "askLookup",
+  "sessionLookup",
+] as const;
+
+interface Results {
+  ranAt: string;
+  outcome: "pass" | "fail" | "skip";
+  skipReason?: string;
+  presentKeys: string[];
+  missingKeys: string[];
+  exercised: {
+    taskService: { ok: boolean; taskId: string; detail: string };
+    persistenceProvider: { ok: boolean; detail: string };
+  } | null;
+}
+
+function arg(name: string): string | undefined {
+  const hit = process.argv.find((a) => a.startsWith(`--${name}=`));
+  return hit?.slice(name.length + 3);
+}
+
+function finish(results: Results, code: number): never {
+  writeFileSync(RESULTS_PATH, `${JSON.stringify(results, null, 2)}\n`);
+  console.log(JSON.stringify(results, null, 2));
+  console.log(`\nWrote ${RESULTS_PATH}`);
+  process.exit(code);
+}
+
+async function main(): Promise<void> {
+  const taskId = arg("task") ?? "mt#4998";
+  const requireBoot = process.argv.includes("--require");
+  const ranAt = new Date().toISOString();
+
+  let domainServices: Awaited<ReturnType<typeof bootDomainContainer>>;
+  try {
+    domainServices = await bootDomainContainer();
+  } catch (err) {
+    const reason = err instanceof Error ? err.message : String(err);
+    console.log(`SKIP: domain container did not boot — ${reason}`);
+    finish(
+      {
+        ranAt,
+        outcome: "skip",
+        skipReason: reason,
+        presentKeys: [],
+        missingKeys: [...REQUIRED_KEYS],
+        exercised: null,
+      },
+      requireBoot ? 2 : 0
+    );
+  }
+
+  // The real builder, on the real container — the binding under test.
+  const deps = buildReviewDomainDeps(domainServices) as Record<string, unknown>;
+
+  const presentKeys = REQUIRED_KEYS.filter((k) => deps[k] != null);
+  const missingKeys = REQUIRED_KEYS.filter((k) => deps[k] == null);
+
+  // Exercise, don't just null-check. A non-null handle to a dead binding is
+  // the failure mode this script exists to catch (mem#654 / §7 item 8): the
+  // reviewer widget's DB layer threw on every query for ~5 weeks while
+  // rendering healthy zeros.
+  let taskOk = false;
+  let taskDetail = "not attempted (taskService missing)";
+  if (deps["taskService"] != null) {
+    try {
+      const spec = await domainServices.taskService.getTaskSpecContent(taskId);
+      const content = (spec as { content?: string } | null)?.content ?? "";
+      taskOk = content.length > 0;
+      taskDetail = taskOk
+        ? `getTaskSpecContent(${taskId}) returned ${content.length} chars`
+        : `getTaskSpecContent(${taskId}) returned empty content`;
+    } catch (err) {
+      taskDetail = `getTaskSpecContent(${taskId}) threw: ${
+        err instanceof Error ? err.message : String(err)
+      }`;
+    }
+  }
+
+  let persistenceOk = false;
+  let persistenceDetail = "not attempted (persistenceProvider missing)";
+  if (deps["persistenceProvider"] != null) {
+    try {
+      // `resolveTier`'s ProvenanceService lookup needs SQL capability; a
+      // provider that reports no SQL capability silently yields `Tier: unknown`
+      // exactly as a null provider does.
+      const caps = domainServices.persistenceProvider.capabilities;
+      persistenceOk = caps?.sql === true;
+      persistenceDetail = `capabilities.sql=${String(caps?.sql)}`;
+    } catch (err) {
+      persistenceDetail = `capabilities read threw: ${
+        err instanceof Error ? err.message : String(err)
+      }`;
+    }
+  }
+
+  const outcome: Results["outcome"] =
+    missingKeys.length === 0 && taskOk && persistenceOk ? "pass" : "fail";
+
+  finish(
+    {
+      ranAt,
+      outcome,
+      presentKeys: [...presentKeys],
+      missingKeys: [...missingKeys],
+      exercised: {
+        taskService: { ok: taskOk, taskId, detail: taskDetail },
+        persistenceProvider: { ok: persistenceOk, detail: persistenceDetail },
+      },
+    },
+    outcome === "pass" ? 0 : 2
+  );
+}
+
+await main();
@@ -29,6 +29,7 @@ import type {
   SqlCapablePersistenceProvider,
 } from "@minsky/domain/persistence/types";
 import type { MemoryLookup, AskLookup } from "./short-id-fetch";
+import type { RunReviewDeps } from "./review-worker";
 
 export interface DomainServices {
   container: AppContainerInterface;
@@ -123,3 +124,63 @@ export async function bootDomainContainer(): Promise<DomainServices> {
 
   return { container, sessionProvider, taskService, persistenceProvider, memoryLookup, askLookup };
 }
+
+/**
+ * The subset of `RunReviewDeps` that carries DOMAIN CONTEXT into a review —
+ * as opposed to the test seams that make up the rest of that interface.
+ *
+ * Every one of these is silently optional in `runReview`, and each degrades a
+ * different part of the review rather than failing it:
+ *
+ * | Missing dep           | Consequence                                              |
+ * | --------------------- | -------------------------------------------------------- |
+ * | `taskService`         | `resolveTaskSpec` → null → `specVerification: []`         |
+ * | `persistenceProvider` | `resolveTier` skips ProvenanceService → `Tier: unknown`   |
+ * | `memoryLookup`        | `mem#N` criteria references unresolved (mt#3964)          |
+ * | `askLookup`           | `ask#N` criteria references unresolved (mt#3964)          |
+ * | `sessionLookup`       | `ws#N` criteria references unresolved (mt#3964)           |
+ *
+ * @see mt#4998 — the defect that motivated extracting this.
+ */
+export type ReviewDomainDeps = Pick<
+  RunReviewDeps,
+  "taskService" | "persistenceProvider" | "memoryLookup" | "askLookup" | "sessionLookup"
+>;
+
+/**
+ * Map booted `DomainServices` onto the review's domain-context deps.
+ *
+ * ## Why this exists rather than each caller spreading the fields itself
+ *
+ * There are three `runReview` entry points — the webhook handler, boot
+ * recovery, and the missed-review sweeper's retrigger — and until mt#4998 each
+ * assembled this object by hand. Two did it identically; the sweeper passed
+ * only `{ db }`, so EVERY sweeper-initiated review ran with no tier resolution
+ * and no bound-task spec, posting `Tier: unknown` with an empty
+ * `specVerification` array. Nothing failed, because each dep degrades quietly
+ * by design (see the table above) — the review simply came out weaker than the
+ * one a webhook would have produced for the same commit.
+ *
+ * A hand-assembled dep list is exactly the shape that admits that defect: the
+ * omission is invisible at the call site and invisible in the output. One
+ * builder means a fourth entry point cannot repeat it.
+ *
+ * Returns `{}` when the container never booted (DB-less or degraded start), so
+ * callers keep today's graceful-degradation behaviour rather than throwing —
+ * the sweeper is a best-effort safety net and must still run without a
+ * container.
+ *
+ * @see mt#2121 — the direct-domain-import path these deps came from.
+ * @see mt#4998 — the sweeper omission this closes.
+ */
+export function buildReviewDomainDeps(domainServices?: DomainServices): ReviewDomainDeps {
+  if (!domainServices) return {};
+  return {
+    taskService: domainServices.taskService,
+    persistenceProvider: domainServices.persistenceProvider,
+    // mt#3964: mem#N / ask#N / ws#N criteria-reference resolution.
+    memoryLookup: domainServices.memoryLookup,
+    askLookup: domainServices.askLookup,
+    sessionLookup: domainServices.sessionProvider,
+  };
+}
@@ -42,7 +42,11 @@ import { loadAdoptionSweeperConfig, startAdoptionSweeper } from "./adoption-swee
 import { getDb, type ReviewerDb } from "./db/client";
 import { applyMigrations } from "./db/migrate";
 import { recoverPendingReviews, loadBootRecoveryConfig } from "./boot-recovery";
-import { bootDomainContainer, type DomainServices } from "./domain-container";
+import {
+  bootDomainContainer,
+  buildReviewDomainDeps,
+  type DomainServices,
+} from "./domain-container";
 import {
   recordWebhookReceipt,
   updateOutcome,
@@ -575,16 +579,9 @@ export function createApp(
       headSha,
       {
         ...(db !== undefined ? { db } : {}),
-        ...(domainServices
-          ? {
-              taskService: domainServices.taskService,
-              persistenceProvider: domainServices.persistenceProvider,
-              // mt#3964: mem#N / ask#N / ws#N criteria-reference resolution.
-              memoryLookup: domainServices.memoryLookup,
-              askLookup: domainServices.askLookup,
-              sessionLookup: domainServices.sessionProvider,
-            }
-          : {}),
+        // mt#4998: one builder for all three runReview entry points. Assembling
+        // this list by hand is what let the sweeper's copy omit it entirely.
+        ...buildReviewDomainDeps(domainServices),
       }
     )
       .then((result) => {
@@ -1779,18 +1776,8 @@ if (import.meta.main) {
     config,
     loadBootRecoveryConfig(),
     runReview,
-    {
-      ...(domainServices
-        ? {
-            taskService: domainServices.taskService,
-            persistenceProvider: domainServices.persistenceProvider,
-            // mt#3964: mem#N / ask#N / ws#N criteria-reference resolution.
-            memoryLookup: domainServices.memoryLookup,
-            askLookup: domainServices.askLookup,
-            sessionLookup: domainServices.sessionProvider,
-          }
-        : {}),
-    },
+    // mt#4998: shared builder — see the webhook site above.
+    buildReviewDomainDeps(domainServices),
     // mt#4881: a recovered review that fails must reach the operator the same
     // way a live one does. Built here from the same container the sweeper and
     // createApp use; undefined when no container booted.
@@ -1895,7 +1882,11 @@ if (import.meta.main) {
   // `undefined`) — so it never blocks startup either way. See sweeper.ts
   // module-header "Boot catch-up sweep" for the diagnosis of why the
   // pre-mt#2660 sweeper missed PR #1812's webhook for 25+ minutes.
-  startSweeper(config, loadSweeperConfig(), db, domainServices?.container, alertSink);
+  // mt#4998: pass the whole DomainServices, not just its container — the
+  // sweeper's retriggers must resolve the same tier and bound-task spec a
+  // webhook review does. Passing `.container` alone is what left every
+  // sweeper-initiated review posting `Tier: unknown` with no spec verification.
+  startSweeper(config, loadSweeperConfig(), db, domainServices, alertSink);
 
   // Start the PR-watch scheduler (mt#1618 / mt#1899).
   // Uses domain imports (mt#2121) via the booted domain container — no MCP-over-HTTP.