From 27af6733c865b814ba6d8692d35687f31197c7c3 Mon Sep 17 00:00:00 2001 From: User Date: Tue, 14 Jul 2026 15:06:16 +0200 Subject: [PATCH] fix: make join review read only --- .../2026-07-14-workflow-ui-regressions.md | 14 +- frontend/src/api/types.ts | 4 +- .../src/widgets/JoinReviewWidget.test.tsx | 59 +++++++++ frontend/src/widgets/JoinReviewWidget.tsx | 87 +++++++++++++ frontend/src/widgets/index.ts | 2 + frontend/src/widgets/registry.test.tsx | 6 + .../gate/__tests__/builders.test.js | 29 +++++ .../gate/__tests__/gate_join_review.test.js | 122 ++++++++++++++++++ harness/.pi/extensions/gate/builders.js | 23 ++++ harness/.pi/extensions/tht-gate.js | 38 ++++-- harness/.pi/skills/tht-sessione/SKILL.md | 16 ++- 11 files changed, 378 insertions(+), 22 deletions(-) create mode 100644 frontend/src/widgets/JoinReviewWidget.test.tsx create mode 100644 frontend/src/widgets/JoinReviewWidget.tsx create mode 100644 harness/.pi/extensions/gate/__tests__/gate_join_review.test.js diff --git a/docs/superpowers/plans/2026-07-14-workflow-ui-regressions.md b/docs/superpowers/plans/2026-07-14-workflow-ui-regressions.md index 53341a7b..5dbc2b23 100644 --- a/docs/superpowers/plans/2026-07-14-workflow-ui-regressions.md +++ b/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. diff --git a/frontend/src/api/types.ts b/frontend/src/api/types.ts index aa1fac4c..3ebb15de 100644 --- a/frontend/src/api/types.ts +++ b/frontend/src/api/types.ts @@ -1,6 +1,8 @@ export interface WidgetOption { id: string; label: string; + detail?: string; + rationale?: string; meta?: Record; 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; diff --git a/frontend/src/widgets/JoinReviewWidget.test.tsx b/frontend/src/widgets/JoinReviewWidget.test.tsx new file mode 100644 index 00000000..b1b867f4 --- /dev/null +++ b/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(); + + 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(); + + 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", + }); +}); diff --git a/frontend/src/widgets/JoinReviewWidget.tsx b/frontend/src/widgets/JoinReviewWidget.tsx new file mode 100644 index 00000000..88f18b50 --- /dev/null +++ b/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 ( +
+
+ {descriptor.title && ( +

+ {descriptor.title} +

+ )} +

+ These joins are required by the selected tables and are shown for review. + To request a correction, use Other — specify. +

+
+ +
+ {options.map((option, index) => ( +
+
+ + +
+
+ + {index + 1} + +

+ {option.label} +

+
+ {option.detail && ( + + {option.detail} + + )} + {option.rationale && ( +
+ + Rationale + +

+ {option.rationale} +

+
+ )} +
+
+
+ ))} +
+ + + + + onRespond({ + id: descriptor.id, + control, + ...(text !== undefined ? { text } : {}), + }) + } + /> +
+ ); +} diff --git a/frontend/src/widgets/index.ts b/frontend/src/widgets/index.ts index c5802a45..ab6e8cad 100644 --- a/frontend/src/widgets/index.ts +++ b/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"; diff --git a/frontend/src/widgets/registry.test.tsx b/frontend/src/widgets/registry.test.tsx index 547ddcd2..9aba9a32 100644 --- a/frontend/src/widgets/registry.test.tsx +++ b/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"); +}); diff --git a/harness/.pi/extensions/gate/__tests__/builders.test.js b/harness/.pi/extensions/gate/__tests__/builders.test.js index 9d7ec885..25a6c0e9 100644 --- a/harness/.pi/extensions/gate/__tests__/builders.test.js +++ b/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); diff --git a/harness/.pi/extensions/gate/__tests__/gate_join_review.test.js b/harness/.pi/extensions/gate/__tests__/gate_join_review.test.js new file mode 100644 index 00000000..6f0b0907 --- /dev/null +++ b/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; + } +}); diff --git a/harness/.pi/extensions/gate/builders.js b/harness/.pi/extensions/gate/builders.js index 8ea3087f..e6532706 100644 --- a/harness/.pi/extensions/gate/builders.js +++ b/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, diff --git a/harness/.pi/extensions/tht-gate.js b/harness/.pi/extensions/tht-gate.js index bc029ed8..80268823 100644 --- a/harness/.pi/extensions/tht-gate.js +++ b/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), ""); diff --git a/harness/.pi/skills/tht-sessione/SKILL.md b/harness/.pi/skills/tht-sessione/SKILL.md index be7e46b7..454a5780 100644 --- a/harness/.pi/skills/tht-sessione/SKILL.md +++ b/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