fix: make join review read only

docs/superpowers/plans/2026-07-14-workflow-ui-regressions.md

@@ -104,20 +104,20 @@ Expected: PASS.
 - Consumes: options `{ id, label, detail?, rationale? }`.
 - Produces: response `{ id, kind: "join-review", choices: allOptionIds }` on `Continue`.
 
-- [ ] **Step 1: Write failing builder and gate tests**
+- [x] **Step 1: Write failing builder and gate tests**
 
 Assert the builder emits read-only option details, `confirm_label: "Continue"`, and reserved controls.
 Exercise `reviewer_decide` with only `join_modified` decisions; queue a Continue response containing
 all ids and assert every `tht decision add` call occurs. Queue `control:"freetext"` and assert no
 decision is written.
 
-- [ ] **Step 2: Run harness tests and verify RED**
+- [x] **Step 2: Run harness tests and verify RED**
 
 Run: `cd harness && node --test .pi/extensions/gate/__tests__/builders.test.js .pi/extensions/gate/__tests__/gate_join_review.test.js`
 
 Expected: builder/export/widget contract is missing and join calls still emit `multiselect`.
 
-- [ ] **Step 3: Implement builder and gate routing**
+- [x] **Step 3: Implement builder and gate routing**
 
 Add `buildJoinReviewRequest`. In `reviewer_decide`, detect a non-empty, join-only merit list:
 
@@ -128,24 +128,24 @@ const joinOnly = opts.length > 0 && opts.every((o) => o.decision.type === "join_
 Emit `join-review` with `detail` and `rationale`; after Continue persist all original decisions.
 On free text, return feedback without persistence. Keep all other decisions on `multiselect`.
 
-- [ ] **Step 4: Write failing frontend widget tests**
+- [x] **Step 4: Write failing frontend widget tests**
 
 Render two join cards and assert there are no checkboxes. Click `Continue` and expect all ids in the
 response. Open `Other — specify`, submit correction text, and expect a freetext control response.
 
-- [ ] **Step 5: Run frontend widget tests and verify RED**
+- [x] **Step 5: Run frontend widget tests and verify RED**
 
 Run: `cd frontend && npx vitest run src/widgets/JoinReviewWidget.test.tsx src/widgets/registry.test.tsx`
 
 Expected: widget and registry entry are missing.
 
-- [ ] **Step 6: Implement and register JoinReviewWidget**
+- [x] **Step 6: Implement and register JoinReviewWidget**
 
 Render semantic cards with label, detail, and rationale, one `Continue` primary button, and
 `ReservedControls`. Register `join-review` and extend `WidgetOption` with optional `detail` and
 `rationale` strings.
 
-- [ ] **Step 7: Update model instructions and verify targeted tests**
+- [x] **Step 7: Update model instructions and verify targeted tests**
 
 Document that joins must be a separate join-only `reviewer_decide` call; the reviewer cannot remove
 individual joins and textual corrections require a complete revised proposal.

frontend/src/api/types.ts

@@ -1,6 +1,8 @@
 export interface WidgetOption {
   id: string;
   label: string;
+  detail?: string;
+  rationale?: string;
   meta?: Record<string, unknown>;
   selected?: boolean;
   recommended?: boolean;
@@ -32,7 +34,7 @@ export interface WidgetDescriptor {
   phase?: string;
   title?: string;
   intro?: string;
-  widget: "info" | "select" | "multiselect" | "freetext" | "artifact-gate" | "artifact" | string;
+  widget: "info" | "select" | "multiselect" | "join-review" | "freetext" | "artifact-gate" | "artifact" | string;
   options?: WidgetOption[];
   reserved?: string[];
   allow_empty?: boolean;

frontend/src/widgets/JoinReviewWidget.test.tsx

@@ -0,0 +1,59 @@
+import { render, screen } from "@testing-library/react";
+import userEvent from "@testing-library/user-event";
+import type { WidgetDescriptor } from "../api/types";
+import { JoinReviewWidget } from "./JoinReviewWidget";
+
+const descriptor = {
+  id: "u-joins",
+  widget: "join-review",
+  title: "F4 — Proposed joins",
+  confirm_label: "Continue",
+  reserved: ["other"],
+  options: [
+    {
+      id: "procedure-event",
+      label: "Procedure to event",
+      detail: "JOIN procedure p ON p.patient_id = e.patient_id",
+      rationale: "Patient grain",
+    },
+    {
+      id: "event-time",
+      label: "Event to time",
+      detail: "JOIN dim_time d ON d.day_key = e.time_key",
+      rationale: "DWH time key",
+    },
+  ],
+} satisfies WidgetDescriptor;
+
+test("renders joins as read-only information and Continue accepts the complete set", async () => {
+  const onRespond = vi.fn();
+  render(<JoinReviewWidget descriptor={descriptor} onRespond={onRespond} />);
+
+  expect(screen.queryAllByRole("checkbox")).toHaveLength(0);
+  expect(screen.getByText("Procedure to event")).toBeInTheDocument();
+  expect(screen.getByText(descriptor.options[0].detail)).toBeInTheDocument();
+  expect(screen.getByText("Patient grain")).toBeInTheDocument();
+
+  await userEvent.click(screen.getByRole("button", { name: "Continue" }));
+
+  expect(onRespond).toHaveBeenCalledWith({
+    id: "u-joins",
+    kind: "join-review",
+    choices: ["procedure-event", "event-time"],
+  });
+});
+
+test("Other specify sends textual feedback instead of a join selection", async () => {
+  const onRespond = vi.fn();
+  render(<JoinReviewWidget descriptor={descriptor} onRespond={onRespond} />);
+
+  await userEvent.click(screen.getByRole("button", { name: "Other — specify" }));
+  await userEvent.type(screen.getByPlaceholderText("Describe your alternative…"), "Use the episode key");
+  await userEvent.click(screen.getByRole("button", { name: "Send" }));
+
+  expect(onRespond).toHaveBeenCalledWith({
+    id: "u-joins",
+    control: "freetext",
+    text: "Use the episode key",
+  });
+});

frontend/src/widgets/JoinReviewWidget.tsx

@@ -0,0 +1,87 @@
+import { Link2 } from "lucide-react";
+import type { WidgetProps } from "./types";
+import { ReservedControls } from "./ReservedControls";
+
+export function JoinReviewWidget({ descriptor, onRespond }: WidgetProps) {
+  const options = descriptor.options ?? [];
+
+  return (
+    <div className="space-y-4 rounded-xl border border-border/70 bg-card p-4 shadow-sm">
+      <div className="space-y-1.5">
+        {descriptor.title && (
+          <p className="text-[0.95rem] font-semibold leading-snug text-foreground">
+            {descriptor.title}
+          </p>
+        )}
+        <p className="text-sm leading-relaxed text-muted-foreground">
+          These joins are required by the selected tables and are shown for review.
+          To request a correction, use Other — specify.
+        </p>
+      </div>
+
+      <div className="space-y-2.5">
+        {options.map((option, index) => (
+          <article
+            key={option.id}
+            className="rounded-lg border border-border/70 bg-background px-3.5 py-3"
+          >
+            <div className="flex items-start gap-2.5">
+              <span className="mt-0.5 flex size-7 shrink-0 items-center justify-center rounded-md bg-primary/10 text-primary">
+                <Link2 className="size-3.5" aria-hidden="true" />
+              </span>
+              <div className="min-w-0 flex-1 space-y-2.5">
+                <div className="flex items-baseline gap-2">
+                  <span className="text-xs font-medium tabular-nums text-muted-foreground">
+                    {index + 1}
+                  </span>
+                  <h3 className="text-sm font-semibold leading-snug text-foreground">
+                    {option.label}
+                  </h3>
+                </div>
+                {option.detail && (
+                  <code className="block whitespace-pre-wrap break-words rounded-md bg-muted/70 px-3 py-2 font-mono text-xs leading-relaxed text-foreground">
+                    {option.detail}
+                  </code>
+                )}
+                {option.rationale && (
+                  <div className="grid gap-1 sm:grid-cols-[5rem_minmax(0,1fr)] sm:gap-3">
+                    <span className="text-xs font-medium uppercase tracking-wide text-muted-foreground">
+                      Rationale
+                    </span>
+                    <p className="text-sm leading-relaxed text-muted-foreground">
+                      {option.rationale}
+                    </p>
+                  </div>
+                )}
+              </div>
+            </div>
+          </article>
+        ))}
+      </div>
+
+      <button
+        className="rounded-md bg-primary px-4 py-2 text-sm font-semibold text-primary-foreground shadow-xs transition-colors hover:bg-[oklch(var(--primary-hover))]"
+        onClick={() =>
+          onRespond({
+            id: descriptor.id,
+            kind: "join-review",
+            choices: options.map((option) => option.id),
+          })
+        }
+      >
+        {descriptor.confirm_label ?? "Continue"}
+      </button>
+
+      <ReservedControls
+        reserved={descriptor.reserved}
+        onControl={(control, text) =>
+          onRespond({
+            id: descriptor.id,
+            control,
+            ...(text !== undefined ? { text } : {}),
+          })
+        }
+      />
+    </div>
+  );
+}

frontend/src/widgets/index.ts

@@ -6,6 +6,7 @@ import { MultiselectWidget } from "./MultiselectWidget";
 import { ArtifactGateWidget } from "./ArtifactGateWidget";
 import { ArtifactWidget } from "./ArtifactWidget";
 import { SchemaLinkingGateWidget } from "./SchemaLinkingGateWidget";
+import { JoinReviewWidget } from "./JoinReviewWidget";
 register("select", SelectWidget);
 register("info", InfoWidget);
 register("freetext", FreetextWidget);
@@ -13,4 +14,5 @@ register("multiselect", MultiselectWidget);
 register("artifact-gate", ArtifactGateWidget);
 register("artifact", ArtifactWidget);
 register("schema-linking", SchemaLinkingGateWidget);
+register("join-review", JoinReviewWidget);
 export { resolve } from "./registry";

frontend/src/widgets/registry.test.tsx

@@ -19,3 +19,9 @@ test("index registers the schema-linking widget", async () => {
   const Comp = resolve("schema-linking");
   expect(Comp.name).toBe("SchemaLinkingGateWidget");
 });
+
+test("index registers the read-only join review widget", async () => {
+  await import("./index");
+  const Comp = resolve("join-review");
+  expect(Comp.name).toBe("JoinReviewWidget");
+});

harness/.pi/extensions/gate/__tests__/builders.test.js

@@ -16,6 +16,7 @@ const {
   buildInfoRequest,
   buildFreetextRequest,
   buildSchemaLinkingRequest,
+  buildJoinReviewRequest,
   withChildLinkage,
 } = require("../builders.js");
 
@@ -130,6 +131,34 @@ test("buildSchemaLinkingRequest carries tables + reserved", () => {
   assert.deepEqual(out.reserved, ["back", "exit", "other"]);
 });
 
+test("buildJoinReviewRequest exposes joins as read-only review items", () => {
+  const out = buildJoinReviewRequest({
+    id: "u8",
+    phase: "F4",
+    title: "F4 — Proposed joins",
+    options: [
+      {
+        id: "patient-events",
+        label: "Patient to events",
+        detail: "JOIN dim_patient p ON p.cod_paz = e.cod_paz",
+        rationale: "Standard patient key",
+      },
+    ],
+  });
+
+  assert.equal(out.widget, "join-review");
+  assert.equal(out.confirm_label, "Continue");
+  assert.deepEqual(out.options, [
+    {
+      id: "patient-events",
+      label: "Patient to events",
+      detail: "JOIN dim_patient p ON p.cod_paz = e.cod_paz",
+      rationale: "Standard patient key",
+    },
+  ]);
+  assert.deepEqual(out.reserved, ["back", "exit", "other"]);
+});
+
 test("withChildLinkage sets option.opens and returns the option", () => {
   const child = { widget: "freetext", title: "Motivazione del rifiuto" };
   const option = withChildLinkage({ id: "reject", label: "Rifiuta" }, child);

harness/.pi/extensions/gate/__tests__/gate_join_review.test.js

@@ -0,0 +1,122 @@
+const test = require("node:test");
+const assert = require("node:assert");
+const cp = require("node:child_process");
+const { createRequire } = require("node:module");
+const path = require("node:path");
+
+const GATE = path.join(__dirname, "..", "..", "tht-gate.js");
+
+if (typeof globalThis.require === "undefined") {
+  globalThis.require = createRequire(GATE);
+}
+
+const JOIN_OPTIONS = [
+  {
+    id: "procedure-event",
+    label: "Procedure to event",
+    decision: {
+      type: "join_modified",
+      subject: "procedure-event",
+      detail: "JOIN procedure p ON p.patient_id = e.patient_id",
+      rationale: "Patient grain",
+    },
+  },
+  {
+    id: "event-time",
+    label: "Event to time",
+    decision: {
+      type: "join_modified",
+      subject: "event-time",
+      detail: "JOIN dim_time d ON d.day_key = e.time_key",
+      rationale: "DWH time key",
+    },
+  },
+];
+
+function shellStub(calls) {
+  return (_file, args) => {
+    calls.push(args.join(" "));
+    if (args[0] === "phase" && args[1] === "meta") {
+      return JSON.stringify({ phases: [{ num: 4, id: "F4" }] });
+    }
+    if (args[0] === "phase" && args[1] === "show") return "Fase corrente: 4\n";
+    return "";
+  };
+}
+
+test("join-only reviewer_decide is read-only and Continue persists every join", async () => {
+  const calls = [];
+  const original = cp.execFileSync;
+  cp.execFileSync = shellStub(calls);
+  try {
+    const gate = require(GATE);
+    const { createFakePi } = require("./fake_pi_runtime.js");
+    const { pi, ctx, tools } = createFakePi();
+    ctx.cwd = "/nonexistent-thothii-test-cwd";
+    gate.default(pi);
+
+    let descriptor;
+    ctx.ui.input = async (title) => {
+      descriptor = JSON.parse(title);
+      return JSON.stringify({
+        id: descriptor.id,
+        kind: "join-review",
+        choices: descriptor.options.map((option) => option.id),
+      });
+    };
+
+    const tool = tools.get("reviewer_decide");
+    await tool.def.execute(
+      "call-joins",
+      { session: "s1", title: "Review joins", options: JOIN_OPTIONS, advance: false },
+      null,
+      null,
+      ctx,
+    );
+
+    assert.equal(descriptor.widget, "join-review");
+    assert.equal(descriptor.options[0].detail, JOIN_OPTIONS[0].decision.detail);
+    assert.equal(descriptor.options[0].rationale, JOIN_OPTIONS[0].decision.rationale);
+    const decisions = calls.filter((call) => call.startsWith("decision add"));
+    assert.equal(decisions.length, 2);
+    assert.ok(decisions.every((call) => call.includes("--type join_modified")));
+  } finally {
+    cp.execFileSync = original;
+  }
+});
+
+test("Other specify rejects the current join proposal without persisting it", async () => {
+  const calls = [];
+  const original = cp.execFileSync;
+  cp.execFileSync = shellStub(calls);
+  try {
+    const gate = require(GATE);
+    const { createFakePi } = require("./fake_pi_runtime.js");
+    const { pi, ctx, tools } = createFakePi();
+    ctx.cwd = "/nonexistent-thothii-test-cwd";
+    gate.default(pi);
+
+    ctx.ui.input = async (title) => {
+      const descriptor = JSON.parse(title);
+      return JSON.stringify({
+        id: descriptor.id,
+        control: "freetext",
+        text: "Use the episode composite key",
+      });
+    };
+
+    const tool = tools.get("reviewer_decide");
+    const result = await tool.def.execute(
+      "call-other",
+      { session: "s1", title: "Review joins", options: JOIN_OPTIONS, advance: false },
+      null,
+      null,
+      ctx,
+    );
+
+    assert.equal(calls.filter((call) => call.startsWith("decision add")).length, 0);
+    assert.match(result.content[0].text, /Use the episode composite key/);
+  } finally {
+    cp.execFileSync = original;
+  }
+});

harness/.pi/extensions/gate/builders.js

@@ -177,6 +177,28 @@ function buildSchemaLinkingRequest({ id, phase, title, tables }) {
   };
 }
 
+// Build a blocking, read-only join review. The reviewer can accept the complete
+// proposal or use Other to request a textual correction; individual joins are
+// deliberately not selectable because omitting one could create a Cartesian product.
+function buildJoinReviewRequest({ id, phase, title, options }) {
+  requireString(title, "title", "join-review");
+  const opts = requireArray(options, "options", "join-review");
+  if (opts.length === 0) {
+    throw new Error("builders: join-review requires at least one option");
+  }
+  return {
+    type: "ui_request",
+    id,
+    phase,
+    schema_version: SCHEMA_VERSION,
+    widget: "join-review",
+    title,
+    options: [...opts],
+    confirm_label: "Continue",
+    reserved: RESERVED,
+  };
+}
+
 // Attach a child-widget spec to an option (linkage, §4.2). Returns the option.
 function withChildLinkage(option, widgetSpec) {
   option.opens = widgetSpec;
@@ -190,6 +212,7 @@ module.exports = {
   buildInfoRequest,
   buildFreetextRequest,
   buildSchemaLinkingRequest,
+  buildJoinReviewRequest,
   withChildLinkage,
   SCHEMA_VERSION,
   RESERVED,

harness/.pi/extensions/tht-gate.js

@@ -31,6 +31,7 @@ import {
 	buildMultiselectRequest,
 	buildArtifactGate,
 	buildSchemaLinkingRequest,
+	buildJoinReviewRequest,
 } from "./gate/builders.js";
 import {
 	validateCtePlanV2,
@@ -728,6 +729,9 @@ export default function (pi) {
 			const meritOptions = opts
 				.filter((o) => !isReserved(o.label))
 				.map((o) => ({ id: o.id, label: o.label }));
+			const joinOnly = meritOptions.length > 0 && opts
+				.filter((o) => !isReserved(o.label))
+				.every((o) => o.decision.type === "join_modified");
 			if (shouldSkipEmptyDecide({ meritCount: meritOptions.length, allowEmpty: params.allow_empty ?? false, advance })) {
 				await ctx.ui.notify(
 					"Nessuna memory riutilizzabile per questa domanda — passo alla fase successiva.",
@@ -738,14 +742,28 @@ export default function (pi) {
 					"Fase memoria vuota: nessuna decisione da registrare, avanzamento automatico alla fase successiva.",
 				);
 			}
-			const widget = buildMultiselectRequest({
-				id: `u${Date.now()}`,
-				phase,
-				title: phase === "F2" ? memorySelectionWidgetProps(opts).title : title,
-				allowEmpty: params.allow_empty ?? false,
-				options: meritOptions,
-				...(phase === "F2" ? memorySelectionWidgetProps(opts) : {}),
-			});
+			const widget = joinOnly
+				? buildJoinReviewRequest({
+					id: `u${Date.now()}`,
+					phase,
+					title,
+					options: opts
+						.filter((o) => !isReserved(o.label))
+						.map((o) => ({
+							id: o.id,
+							label: o.label,
+							detail: o.decision.detail ?? "",
+							rationale: o.decision.rationale ?? "",
+						})),
+				})
+				: buildMultiselectRequest({
+					id: `u${Date.now()}`,
+					phase,
+					title: phase === "F2" ? memorySelectionWidgetProps(opts).title : title,
+					allowEmpty: params.allow_empty ?? false,
+					options: meritOptions,
+					...(phase === "F2" ? memorySelectionWidgetProps(opts) : {}),
+				});
 			const resp = await emitAndWait(ctx, widget);
 			if (resp.control === "freetext") {
 				return textResult(
@@ -756,7 +774,9 @@ export default function (pi) {
 				return textResult("Il reviewer vuole tornare indietro.");
 			if (resp.control === "exit")
 				return textResult("Il reviewer vuole uscire.");
-			const chosen = opts.filter((o) => (resp.choices ?? []).includes(o.id));
+			const chosen = joinOnly
+				? opts.filter((o) => !isReserved(o.label))
+				: opts.filter((o) => (resp.choices ?? []).includes(o.id));
 			for (const c of chosen) {
 				const d = c.decision;
 				const err = relayIfThtFails(ctx, decisionAddArgs(session, d), "");

harness/.pi/skills/tht-sessione/SKILL.md

@@ -13,8 +13,8 @@ NEVER advance a phase or record a decision without explicit reviewer confirmatio
 The reviewer answers via the gate's **widgets** (built by `tht-gate.js`):
 `reviewer_select` (single pick; a chosen option carrying a `decision` payload IS the
 confirmation and is persisted directly — an option without a payload only asks),
-`reviewer_decide` (multiselect, each selected option IS a decision — the choice is the
-confirmation), `reviewer_confirm` (gate on an artifact / phase transition). Free text
+`reviewer_decide` (normally a multiselect; a join-only proposal is rendered read-only and
+Continue records the complete join set), `reviewer_confirm` (gate on an artifact / phase transition). Free text
 arrives via the "Altro/Other" option or by prefixing `!` in chat.
 
 **Language contract (from the workspace `language` field):** the table/column
@@ -36,7 +36,7 @@ substantive decisions.
 |-------|--------------|--------------------|
 | F1 chiarimento | — | `reviewer_confirm kind:"phase"` |
 | F2 memoria | — | `advance:true` only if nothing recorded; else `reviewer_confirm kind:"phase"` |
-| F3 riscrittura | `question.md` | `reviewer_confirm kind:"phase"` (after `rewrite_question`) |
+| F3 riscrittura | `question.md` | `rewrite_question` records approval and advances automatically |
 | F4 schema_linking | `schema_linking.json` | `reviewer_confirm kind:"phase"` (after `reviewer_schema_linking` + `write_schema_linking`). Promoted columns are the reviewer-approved OUTPUT columns — project exactly those in the final SELECT. |
 | F5 sintesi | — | `reviewer_confirm kind:"phase"` (after `tht session check`) |
 | F6 cte | `cte_plan.json`, `ctes/`, `cte_tests.json` | approve each CTE `kind:"cte_result"`, then `reviewer_confirm kind:"phase"` |
@@ -278,8 +278,14 @@ Prerequisite: Phase 3 closed.
    columns: project exactly those in the final SELECT (Phase 6/7); you remain free
    to reference other columns as join keys or filter predicates when the query
    requires them.
-   Propose joins separately in `reviewer_decide(advance:false)`, registering
-   `join_modified`. Ground them in the `【Foreign keys】` section of the mschema-text
+   Propose **all required joins together in a separate, join-only**
+   `reviewer_decide(advance:false)`, registering `join_modified`. Do not mix
+   `join_modified` with other decision types in that call. The gate renders this proposal
+   as read-only information: **Continue records every proposed join**; the reviewer cannot
+   remove individual joins (which could create an accidental Cartesian product). If the
+   reviewer uses **Other — specify**, none of the current joins is recorded: incorporate
+   the textual correction and present the complete revised join set again.
+   Ground joins in the `【Foreign keys】` section of the mschema-text
    render: it lists the curated logical FKs of the workspace (e.g.
    `fact_x.cod_paz=dim_patient.cod_paz`, `*_time_key=dim_time.day_key`) — prefer
    those to joins you derive yourself, and flag to the reviewer any join you need