Reviewer Phase 3: three-way diff with canonical path normalization

src/agents/contract-extract.ts

@@ -7,6 +7,104 @@ export type Endpoint = {
   path: string;
 };
 
+export type ContractDiff = {
+  // Endpoints in Rails not implemented by EITHER mobile client. Informational
+  // only — Rails has admin / server-only endpoints that aren't called from
+  // mobile apps; this list helps surface what's available but unused.
+  railsOnly: Endpoint[];
+  // Endpoints called by a mobile client that Rails doesn't expose. Real
+  // failure: client will get 404 at runtime.
+  iosOrphan: Endpoint[];
+  androidOrphan: Endpoint[];
+  // Endpoints implemented by exactly one mobile client. Real failure if
+  // both should be in feature parity (per "mobile clients must agree").
+  iosOnly: Endpoint[];
+  androidOnly: Endpoint[];
+};
+
+export type CanonicalizationContext = {
+  // Lowercased rename target for "Shopkeeper" — the API role segment in the
+  // URL (`/vet/...` for Vet, `/shopkeeper/...` for the unrenamed substrate).
+  // Defaults to "shopkeeper" if the rename plan doesn't include Shopkeeper.
+  role: string;
+};
+
+const TENANT_PLACEHOLDER = "{account_id}";
+const UUID_RE = /^[a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12}$/i;
+const OPENAPI_PARAM_RE = /^\{[^}]+\}$/;
+const SWIFT_INTERPOLATION_RE = /^\\\([^)]+\)$/;
+
+// Strip tenant prefix, /api/v1, role segment, and normalize path parameter
+// names to {*} so the three platforms' encodings reduce to the same
+// comparable string. Rails OpenAPI server URLs (e.g. `/api/v1/vet`) are
+// already factored out by extractRailsContract, so Rails endpoints arrive
+// here with just the per-operation path.
+export function canonicalizeEndpoint(
+  endpoint: Endpoint,
+  ctx: CanonicalizationContext,
+): Endpoint {
+  const role = ctx.role.toLowerCase();
+  const segments = endpoint.path.split("/").filter(Boolean);
+
+  // Drop tenant segment (literal placeholder or runtime UUID).
+  if (segments[0] === TENANT_PLACEHOLDER || (segments[0] && UUID_RE.test(segments[0]))) {
+    segments.shift();
+  }
+  // Drop /api/v<N>/.
+  if (segments[0] === "api") {
+    segments.shift();
+    if (segments[0] && /^v\d+$/.test(segments[0])) segments.shift();
+  }
+  // Drop role segment.
+  if (segments[0] === role) segments.shift();
+
+  // Normalize path-param names to {*} so {shopId} === {id} === \(id).
+  const normalized = segments.map((s) =>
+    OPENAPI_PARAM_RE.test(s) || SWIFT_INTERPOLATION_RE.test(s) ? "{*}" : s,
+  );
+
+  return { method: endpoint.method, path: "/" + normalized.join("/") };
+}
+
+// Three-way diff. Endpoints are first canonicalized so cross-platform
+// encoding differences don't show up as drift.
+export function diffContracts(
+  rails: readonly Endpoint[],
+  ios: readonly Endpoint[],
+  android: readonly Endpoint[],
+  ctx: CanonicalizationContext,
+): ContractDiff {
+  const canon = (e: Endpoint) => canonicalizeEndpoint(e, ctx);
+  const key = (e: Endpoint) => `${e.method} ${e.path}`;
+
+  const railsSet = new Map(rails.map(canon).map((e) => [key(e), e]));
+  const iosSet = new Map(ios.map(canon).map((e) => [key(e), e]));
+  const androidSet = new Map(android.map(canon).map((e) => [key(e), e]));
+
+  const railsOnly: Endpoint[] = [];
+  for (const [k, e] of railsSet) {
+    if (!iosSet.has(k) && !androidSet.has(k)) railsOnly.push(e);
+  }
+  const iosOrphan: Endpoint[] = [];
+  for (const [k, e] of iosSet) {
+    if (!railsSet.has(k)) iosOrphan.push(e);
+  }
+  const androidOrphan: Endpoint[] = [];
+  for (const [k, e] of androidSet) {
+    if (!railsSet.has(k)) androidOrphan.push(e);
+  }
+  const iosOnly: Endpoint[] = [];
+  for (const [k, e] of iosSet) {
+    if (railsSet.has(k) && !androidSet.has(k)) iosOnly.push(e);
+  }
+  const androidOnly: Endpoint[] = [];
+  for (const [k, e] of androidSet) {
+    if (railsSet.has(k) && !iosSet.has(k)) androidOnly.push(e);
+  }
+
+  return { railsOnly, iosOrphan, androidOrphan, iosOnly, androidOnly };
+}
+
 export type RailsContract = {
   openapiVersion: string;
   title: string;

src/agents/judge.ts

@@ -77,13 +77,15 @@ export async function runJudge(input: JudgeInput): Promise<JudgeResult> {
   const visualPass = visualReport
     ? Object.values(visualReport).every((r): r is VisualJudgePlatformReport => Boolean(r) && r!.pass)
     : true;
-  const overallPass = layer1Layer2Pass && visualPass;
+  const reviewerPass = input.reviewer.contractParity === "pass";
+  const overallPass = layer1Layer2Pass && visualPass && reviewerPass;
   const l1Total = reports.filter((r) => r.layer1Pass).length;
   const l2Total = reports.filter((r) => r.layer2Pass).length;
+  const reviewerSummary = reviewerPass ? "reviewer PASS" : "reviewer FAIL";
 
   return {
     overallPass,
-    summary: `Layer 1 ${l1Total}/3 pass · Layer 2 ${l2Total}/3 pass · ${layer3Summary}`,
+    summary: `Layer 1 ${l1Total}/3 pass · Layer 2 ${l2Total}/3 pass · ${layer3Summary} · ${reviewerSummary}`,
     ...(visualReport ? { visual: visualReport } : {}),
   };
 }

src/agents/reviewer.ts

@@ -5,6 +5,9 @@ import {
   extractRailsContract,
   extractAndroidEndpoints,
   extractIosEndpoints,
+  diffContracts,
+  type ContractDiff,
+  type Endpoint,
 } from "./contract-extract.js";
 import type { DomainSpec, ReviewerResult, WorkerResult } from "./types.js";
 
@@ -15,18 +18,19 @@ export type ReviewerInput = {
   android: WorkerResult;
 };
 
-// Phase 2: extract API surface from all three platforms — Rails OpenAPI,
-// Android Retrofit interfaces, iOS Request structs — and surface counts in
-// the trace + result. This proves rename left every platform's network
-// layer parseable and provides the contract objects Phase 3 will use to
-// detect actual drift (paths in Rails but missing from a client, methods
-// that disagree, etc.).
+// Phase 3: extract from all three platforms (Rails OpenAPI, iOS Request
+// structs, Android Retrofit interfaces), canonicalize their path encodings
+// (strip tenant {account_id}, /api/v<N>, role segment, normalize path-param
+// names), then diff.
 //
-// For now reviewer stays at contractParity: "pass" unless extraction itself
-// fails — count mismatches alone don't fail the run, since iOS / Android
-// may legitimately implement a subset of Rails endpoints (e.g. the Rails
-// admin namespace isn't called from mobile clients). Phase 3 wires the
-// pass/fail logic against a normalized path-level diff.
+// Pass/fail policy:
+//   - PASS  if every iOS and Android endpoint maps to a Rails endpoint AND
+//           the two mobile clients implement the same set.
+//   - FAIL  if any mobile-orphan exists (client calls something Rails
+//           doesn't expose — runtime 404), or if iOS and Android implement
+//           different subsets ("mobile clients must agree" per project USP).
+//   - rails-only endpoints are informational (admin / server-only routes
+//     that aren't supposed to be called from mobile).
 export async function runReviewer(input: ReviewerInput): Promise<ReviewerResult> {
   if (isStub("reviewer")) return runStubReviewer(input);
 
@@ -49,28 +53,74 @@ export async function runReviewer(input: ReviewerInput): Promise<ReviewerResult>
     `Rails: ${railsContract.endpoints.length} endpoints, ${railsContract.schemaCount} schemas, openapi=${railsContract.openapiVersion}, title="${railsContract.title}"`,
   );
 
-  trace("reviewer", `extracting iOS Request structs from ${ios.outDir}`);
   const iosEndpoints = await extractIosEndpoints(iosDir);
-  trace("reviewer", `iOS: ${iosEndpoints.length} request endpoints`);
-
-  trace("reviewer", `extracting Android Retrofit annotations from ${android.outDir}`);
   const androidEndpoints = await extractAndroidEndpoints(androidDir);
+  trace("reviewer", `iOS: ${iosEndpoints.length} request endpoints`);
   trace("reviewer", `Android: ${androidEndpoints.length} Retrofit endpoints`);
 
-  trace("reviewer", "three-way diff — not yet implemented (Phase 3+)");
-  trace("reviewer", `${domain.displayName}: contract parity PASS (Phase 2 extraction-only)`);
-
-  return {
-    contractParity: "pass",
-    diffs: [
-      `rails:openapi=${railsContract.openapiVersion}`,
-      `rails:title=${railsContract.title}`,
-      `rails:endpoints=${railsContract.endpoints.length}`,
-      `rails:schemas=${railsContract.schemaCount}`,
-      `ios:endpoints=${iosEndpoints.length}`,
-      `android:endpoints=${androidEndpoints.length}`,
-    ],
-  };
+  const role = deriveRole(domain);
+  const diff = diffContracts(railsContract.endpoints, iosEndpoints, androidEndpoints, { role });
+
+  const summary = formatDiffSummary(diff);
+  for (const line of summary) trace("reviewer", line);
+
+  const fatalCount =
+    diff.iosOrphan.length + diff.androidOrphan.length + diff.iosOnly.length + diff.androidOnly.length;
+
+  const baseDiffs: string[] = [
+    `rails:openapi=${railsContract.openapiVersion}`,
+    `rails:title=${railsContract.title}`,
+    `rails:endpoints=${railsContract.endpoints.length}`,
+    `rails:schemas=${railsContract.schemaCount}`,
+    `ios:endpoints=${iosEndpoints.length}`,
+    `android:endpoints=${androidEndpoints.length}`,
+    `role=${role}`,
+    `rails-only=${diff.railsOnly.length}`,
+    `ios-orphan=${diff.iosOrphan.length}`,
+    `android-orphan=${diff.androidOrphan.length}`,
+    `ios-only=${diff.iosOnly.length}`,
+    `android-only=${diff.androidOnly.length}`,
+  ];
+  const findings: string[] = [
+    ...formatFindings("ios-orphan", diff.iosOrphan),
+    ...formatFindings("android-orphan", diff.androidOrphan),
+    ...formatFindings("ios-only", diff.iosOnly),
+    ...formatFindings("android-only", diff.androidOnly),
+  ];
+
+  if (fatalCount > 0) {
+    trace("reviewer", `${domain.displayName}: contract parity FAIL — ${fatalCount} drift(s)`);
+    return { contractParity: "fail", diffs: [...baseDiffs, ...findings] };
+  }
+
+  trace("reviewer", `${domain.displayName}: contract parity PASS`);
+  return { contractParity: "pass", diffs: [...baseDiffs, ...findings] };
+}
+
+function deriveRole(domain: DomainSpec): string {
+  const pair = domain.renamePlan.find((p) => p.from === "Shopkeeper");
+  return (pair?.to ?? "Shopkeeper").toLowerCase();
+}
+
+function formatDiffSummary(diff: ContractDiff): string[] {
+  const lines: string[] = [];
+  lines.push(
+    `diff: rails-only=${diff.railsOnly.length}, ios-orphan=${diff.iosOrphan.length}, android-orphan=${diff.androidOrphan.length}, ios-only=${diff.iosOnly.length}, android-only=${diff.androidOnly.length}`,
+  );
+  if (diff.iosOrphan.length > 0) lines.push(`iOS calls endpoints not in Rails: ${preview(diff.iosOrphan)}`);
+  if (diff.androidOrphan.length > 0) lines.push(`Android calls endpoints not in Rails: ${preview(diff.androidOrphan)}`);
+  if (diff.iosOnly.length > 0) lines.push(`iOS-only (Android missing): ${preview(diff.iosOnly)}`);
+  if (diff.androidOnly.length > 0) lines.push(`Android-only (iOS missing): ${preview(diff.androidOnly)}`);
+  return lines;
+}
+
+function formatFindings(label: string, endpoints: readonly Endpoint[]): string[] {
+  return endpoints.map((e) => `${label}:${e.method} ${e.path}`);
+}
+
+function preview(endpoints: readonly Endpoint[], max: number = 3): string {
+  const sample = endpoints.slice(0, max).map((e) => `${e.method} ${e.path}`).join(", ");
+  return endpoints.length > max ? `${sample}, ... (+${endpoints.length - max} more)` : sample;
 }
 
 const delay = (ms: number): Promise<void> => new Promise((r) => { setTimeout(r, ms); });

tests/smoke.test.ts

@@ -3,6 +3,7 @@ import assert from "node:assert/strict";
 import { runLayer1, runLayer2, runLayer3, captureScreenshot, installAndLaunch, runVisualJudge, DEFAULT_STAGE1_RUBRIC, discoverIosArtifact, discoverAndroidArtifact, runStage1Visual } from "../src/validation/index.js";
 import { dispatch } from "../src/dispatch.js";
 import { runReviewer } from "../src/agents/reviewer.js";
+import { canonicalizeEndpoint, diffContracts } from "../src/agents/contract-extract.js";
 
 test("validation layers are exported as functions", () => {
   assert.equal(typeof runLayer1, "function");
@@ -165,6 +166,38 @@ test("runReviewer in stub mode passes without touching disk", async () => {
   assert.deepEqual(result.diffs, []);
 });
 
+test("canonicalizeEndpoint reduces all three platform encodings to the same string", async () => {
+  const ctx = { role: "vet" };
+  // Rails OpenAPI: server is /api/v1/vet, path is just /clinics
+  const rails = canonicalizeEndpoint({ method: "GET", path: "/clinics" }, ctx);
+  // iOS Request struct: "/vet/shops/\(id)" (literal Swift interpolation in extracted string)
+  const ios = canonicalizeEndpoint({ method: "GET", path: "/vet/clinics/\\(id)" }, ctx);
+  // Android Retrofit: literal full path
+  const android = canonicalizeEndpoint(
+    { method: "GET", path: "{account_id}/api/v1/vet/clinics/{id}" },
+    ctx,
+  );
+  assert.equal(rails.path, "/clinics");
+  assert.equal(ios.path, "/clinics/{*}");
+  assert.equal(android.path, "/clinics/{*}");
+});
+
+test("diffContracts surfaces ios orphan when client calls endpoint not in rails", async () => {
+  const ctx = { role: "vet" };
+  const rails = [{ method: "GET" as const, path: "/clinics" }];
+  const ios = [
+    { method: "GET" as const, path: "/vet/clinics" },
+    { method: "DELETE" as const, path: "/vet/clinics/\\(id)/reset" },
+  ];
+  const android = [{ method: "GET" as const, path: "{account_id}/api/v1/vet/clinics" }];
+  const diff = diffContracts(rails, ios, android, ctx);
+  assert.equal(diff.iosOrphan.length, 1);
+  assert.equal(diff.iosOrphan[0]?.method, "DELETE");
+  assert.equal(diff.iosOrphan[0]?.path, "/clinics/{*}/reset");
+  assert.equal(diff.iosOnly.length, 0);
+  assert.equal(diff.androidOnly.length, 0);
+});
+
 test("dispatch runs planner + workers + reviewer + judge end-to-end (stub pipeline)", async () => {
   const result = await dispatch("a walk-in clinic queue for small veterinary practices");
   assert.equal(result.overallPass, true);