From 3ad93cd02fd12f28a51e81348c41a3b1b5a7f1c7 Mon Sep 17 00:00:00 2001 From: mptyl Date: Tue, 7 Jul 2026 00:42:48 +0200 Subject: [PATCH] feat(gate): deterministic v2 review-gate payloads (cte_plan/cte_result/phase) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The tht-gate.js reviewer_confirm now builds structured v2 artifacts before the widget so the reviewer approves gate-derived data, not raw model text: - new pure modules gate/artifact-contracts.js (soft validators, {ok,errors}, legacy-passthrough) and gate/enrich.js (index/description enrichment, buildCteResultV2 fusing thin model data with `tht cte info`, phase enrichment) - cte_plan v2: validate + enrich + persist via `tht cte plan --name … --doc -` (names derived from data.ctes[]); legacy `names` param kept as fallback - cte_result v2: rebuild from `tht cte next`/`tht cte info` (sql + preview from the persisted test record); null/error last_test -> actionable textResult - phase v2: soft-validate + fill phase from meta + catalog descriptions - prepareReviewerArguments coerces artifact.data too (GLM double-stringify); legacy markdown strings pass through unchanged - SKILL.md: Phase 6 cte_plan payload A + thin cte_result guidance; Discipline 6 payload C example; Discipline 7 reworded for gate-rebuilt cte_result Legacy (non-v2) paths unchanged. TypeBox stays Type.Any() for artifact.data; validation is soft (textResult) so models self-correct instead of looping. All 102 gate JS tests green. Co-Authored-By: Claude Fable 5 --- .../gate/__tests__/artifact_contracts.test.js | 157 +++++++ .../extensions/gate/__tests__/enrich.test.js | 192 ++++++++ .../__tests__/gate_confirm_cte_v2.test.js | 420 ++++++++++++++++++ .../__tests__/gate_stringified_params.test.js | 27 ++ .../.pi/extensions/gate/artifact-contracts.js | 125 ++++++ harness/.pi/extensions/gate/enrich.js | 160 +++++++ harness/.pi/extensions/tht-gate.js | 144 +++++- harness/.pi/skills/tht-sessione/SKILL.md | 71 ++- 8 files changed, 1278 insertions(+), 18 deletions(-) create mode 100644 harness/.pi/extensions/gate/__tests__/artifact_contracts.test.js create mode 100644 harness/.pi/extensions/gate/__tests__/enrich.test.js create mode 100644 harness/.pi/extensions/gate/__tests__/gate_confirm_cte_v2.test.js create mode 100644 harness/.pi/extensions/gate/artifact-contracts.js create mode 100644 harness/.pi/extensions/gate/enrich.js diff --git a/harness/.pi/extensions/gate/__tests__/artifact_contracts.test.js b/harness/.pi/extensions/gate/__tests__/artifact_contracts.test.js new file mode 100644 index 00000000..7c023f82 --- /dev/null +++ b/harness/.pi/extensions/gate/__tests__/artifact_contracts.test.js @@ -0,0 +1,157 @@ +// Soft-validators for the v2 artifact payloads (contracts.md A/B/C). Pure, no I/O. +// A payload without schema_version===2 is NOT an error -- it's legacy, and the caller +// falls back to the current (pre-v2) rendering path. +const test = require("node:test"); +const assert = require("node:assert"); +const { + validateCtePlanV2, + validateCteResultThin, + validatePhaseSummaryV2, +} = require("../artifact-contracts.js"); + +// --- validateCtePlanV2 (payload A) ------------------------------------------------ + +test("validateCtePlanV2: legacy (no schema_version) is ok:true, legacy:true", () => { + const out = validateCtePlanV2({ some: "legacy shape" }); + assert.deepEqual(out, { ok: true, legacy: true }); +}); + +test("validateCtePlanV2: legacy when data is a plain string (markdown)", () => { + const out = validateCtePlanV2("### Piano CTE\n1. foo"); + assert.deepEqual(out, { ok: true, legacy: true }); +}); + +test("validateCtePlanV2: valid v2 payload", () => { + const out = validateCtePlanV2({ + schema_version: 2, + question: "domanda", + strategy: "razionale", + ctes: [ + { name: "base_pazienti", purpose: "p", rationale: "r", depends_on: [], tables: [], keys: [], filters: [], output_columns: [] }, + { name: "eventi", purpose: "p2", rationale: "r2", depends_on: ["base_pazienti"], tables: [], keys: [], filters: [], output_columns: [] }, + ], + }); + assert.equal(out.ok, true); + assert.ok(!out.errors || out.errors.length === 0); +}); + +test("validateCtePlanV2: rejects missing ctes array", () => { + const out = validateCtePlanV2({ schema_version: 2, question: "q", strategy: "s" }); + assert.equal(out.ok, false); + assert.ok(out.errors.some((e) => /ctes/.test(e))); +}); + +test("validateCtePlanV2: rejects a CTE missing name", () => { + const out = validateCtePlanV2({ + schema_version: 2, question: "q", strategy: "s", + ctes: [{ purpose: "p" }], + }); + assert.equal(out.ok, false); + assert.ok(out.errors.some((e) => /ctes\[0\].*name/.test(e))); +}); + +test("validateCtePlanV2: rejects depends_on referencing an unknown/forward CTE", () => { + const out = validateCtePlanV2({ + schema_version: 2, question: "q", strategy: "s", + ctes: [ + { name: "a", depends_on: ["b"] }, // b does not precede a + { name: "b", depends_on: [] }, + ], + }); + assert.equal(out.ok, false); + assert.ok(out.errors.some((e) => /depends_on/.test(e) && /'b'/.test(e))); +}); + +test("validateCtePlanV2: depends_on referencing an earlier CTE is fine", () => { + const out = validateCtePlanV2({ + schema_version: 2, question: "q", strategy: "s", + ctes: [ + { name: "a", depends_on: [] }, + { name: "b", depends_on: ["a"] }, + ], + }); + assert.equal(out.ok, true); +}); + +// --- validateCteResultThin (payload B, model side is THIN) ------------------------ + +test("validateCteResultThin: legacy (string/markdown) is ok:true, legacy:true", () => { + assert.deepEqual(validateCteResultThin("**CTE risultato**: ok"), { ok: true, legacy: true }); + assert.deepEqual(validateCteResultThin({ some: "legacy" }), { ok: true, legacy: true }); +}); + +test("validateCteResultThin: valid thin v2 payload (all fields optional)", () => { + const out = validateCteResultThin({ schema_version: 2 }); + assert.equal(out.ok, true); +}); + +test("validateCteResultThin: valid thin v2 payload with purpose/rationale/note", () => { + const out = validateCteResultThin({ + schema_version: 2, purpose: "seleziona pazienti", rationale: "base per la catena", note: "ok", + }); + assert.equal(out.ok, true); +}); + +test("validateCteResultThin: rejects non-string purpose/rationale/note", () => { + const out = validateCteResultThin({ schema_version: 2, purpose: 42 }); + assert.equal(out.ok, false); + assert.ok(out.errors.some((e) => /purpose/.test(e))); +}); + +test("validateCteResultThin: permissive on extra fields (e.g. a model that also sends sql) -- guidance against this lives in SKILL.md, not the validator", () => { + const out = validateCteResultThin({ schema_version: 2, sql: "WITH a AS (SELECT 1)" }); + assert.equal(out.ok, true); +}); + +// --- validatePhaseSummaryV2 (payload C) ------------------------------------------- + +test("validatePhaseSummaryV2: legacy (no schema_version) is ok:true, legacy:true", () => { + assert.deepEqual(validatePhaseSummaryV2({ recap: "legacy text" }), { ok: true, legacy: true }); + assert.deepEqual(validatePhaseSummaryV2("legacy markdown"), { ok: true, legacy: true }); +}); + +test("validatePhaseSummaryV2: valid minimal v2 payload", () => { + const out = validatePhaseSummaryV2({ schema_version: 2, summary: "riepilogo" }); + assert.equal(out.ok, true); +}); + +test("validatePhaseSummaryV2: valid full v2 payload", () => { + const out = validatePhaseSummaryV2({ + schema_version: 2, + summary: "riepilogo", + checks: [{ label: "schema valido", status: "ok" }], + sections: [{ title: "Criteri", items: [{ label: "flag attivo", table: "t", column: "c", value: "TRUE", kind: "filter" }] }], + tables: [{ name: "t", role: "promoted", columns: [{ name: "c" }] }], + open_questions: [], + }); + assert.equal(out.ok, true); +}); + +test("validatePhaseSummaryV2: rejects missing summary", () => { + const out = validatePhaseSummaryV2({ schema_version: 2 }); + assert.equal(out.ok, false); + assert.ok(out.errors.some((e) => /summary/.test(e))); +}); + +test("validatePhaseSummaryV2: rejects a check with an invalid status", () => { + const out = validatePhaseSummaryV2({ + schema_version: 2, summary: "s", + checks: [{ label: "x", status: "maybe" }], + }); + assert.equal(out.ok, false); + assert.ok(out.errors.some((e) => /checks\[0\].*status/.test(e))); +}); + +test("validatePhaseSummaryV2: rejects a section missing title or items", () => { + const out = validatePhaseSummaryV2({ + schema_version: 2, summary: "s", + sections: [{ items: [] }], + }); + assert.equal(out.ok, false); + assert.ok(out.errors.some((e) => /sections\[0\].*title/.test(e))); +}); + +test("validatePhaseSummaryV2: permissive on extra unknown fields", () => { + const out = validatePhaseSummaryV2({ schema_version: 2, summary: "s", extra_field_from_model: "whatever" }); + assert.equal(out.ok, true); +}); diff --git a/harness/.pi/extensions/gate/__tests__/enrich.test.js b/harness/.pi/extensions/gate/__tests__/enrich.test.js new file mode 100644 index 00000000..6a4bfdf6 --- /dev/null +++ b/harness/.pi/extensions/gate/__tests__/enrich.test.js @@ -0,0 +1,192 @@ +// Pure merge/enrichment functions (contracts.md A/B/C). Catalog lookups are injected +// as a `getColumns` function parameter so these are testable with no `tht` shell-out. +const test = require("node:test"); +const assert = require("node:assert"); +const { + enrichCtePlanV2, + buildCteResultV2, + enrichPhaseSummaryV2, +} = require("../enrich.js"); + +// --- enrichCtePlanV2 -------------------------------------------------------------- + +test("enrichCtePlanV2 assigns 1-based index in ctes[] order", () => { + const data = { + schema_version: 2, question: "q", strategy: "s", + ctes: [ + { name: "a", tables: [], filters: [] }, + { name: "b", tables: [], filters: [] }, + ], + }; + const out = enrichCtePlanV2(data, () => null); + assert.equal(out.ctes[0].index, 1); + assert.equal(out.ctes[1].index, 2); +}); + +test("enrichCtePlanV2 fills tables[].description from the catalog", () => { + const getColumns = (table) => + table === "dim_patient" ? { description: "Anagrafica pazienti", columns: [{ name: "cod_paz", description: "Codice paziente" }] } : null; + const data = { + schema_version: 2, question: "q", strategy: "s", + ctes: [{ name: "a", tables: [{ name: "dim_patient" }], filters: [] }], + }; + const out = enrichCtePlanV2(data, getColumns); + assert.equal(out.ctes[0].tables[0].description, "Anagrafica pazienti"); +}); + +test("enrichCtePlanV2 fills filters[].description by resolving table.column", () => { + const getColumns = (table) => + table === "dim_patient" + ? { description: "Anagrafica", columns: [{ name: "flag_attivo", description: "Flag attivo" }] } + : null; + const data = { + schema_version: 2, question: "q", strategy: "s", + ctes: [{ name: "a", tables: [], filters: [{ column: "dim_patient.flag_attivo", op: "IS", value: "TRUE" }] }], + }; + const out = enrichCtePlanV2(data, getColumns); + assert.equal(out.ctes[0].filters[0].description, "Flag attivo"); +}); + +test("enrichCtePlanV2: catalog miss -> empty description string, never throws", () => { + const out = enrichCtePlanV2( + { + schema_version: 2, question: "q", strategy: "s", + ctes: [{ name: "a", tables: [{ name: "nonexistent_table" }], filters: [{ column: "nonexistent_table.x", op: "IS", value: "1" }] }], + }, + () => null, + ); + assert.equal(out.ctes[0].tables[0].description, ""); + assert.equal(out.ctes[0].filters[0].description, ""); +}); + +test("enrichCtePlanV2: getColumns throwing is tolerated by the caller contract (function itself does not catch)", () => { + // enrich.js documents that getColumns must itself be safe (try/catch -> null on miss); + // enrichCtePlanV2 does not need its own try/catch around calls, since getColumns never throws. + const getColumns = () => null; // simulates the safe wrapper's miss behavior + const out = enrichCtePlanV2( + { schema_version: 2, question: "q", strategy: "s", ctes: [{ name: "a", tables: [{ name: "t" }], filters: [] }] }, + getColumns, + ); + assert.equal(out.ctes[0].tables[0].description, ""); +}); + +test("enrichCtePlanV2 preserves model-authored fields (purpose/rationale/depends_on/keys/output_columns)", () => { + const data = { + schema_version: 2, question: "q", strategy: "s", + ctes: [{ + name: "a", purpose: "scopo", rationale: "motivo", depends_on: [], + keys: ["cod_paz"], output_columns: ["cod_paz"], tables: [], filters: [], + }], + }; + const out = enrichCtePlanV2(data, () => null); + assert.equal(out.ctes[0].purpose, "scopo"); + assert.equal(out.ctes[0].rationale, "motivo"); + assert.deepEqual(out.ctes[0].keys, ["cod_paz"]); + assert.deepEqual(out.ctes[0].output_columns, ["cod_paz"]); +}); + +// --- buildCteResultV2 -------------------------------------------------------------- + +test("buildCteResultV2 merges name/index/total/sql from cteInfo", () => { + const thin = { schema_version: 2, note: "nota" }; + const cteInfo = { + name: "base_pazienti", index: 1, total: 3, sql: "WITH base_pazienti AS (SELECT 1)", + doc: null, last_test: { status: "ok", execution_ms: 42, row_sample: 5, warnings: [], sql_hash: "h1", columns: ["x"], preview_rows: [[1]] }, + }; + const out = buildCteResultV2(thin, cteInfo); + assert.equal(out.name, "base_pazienti"); + assert.equal(out.index, 1); + assert.equal(out.total, 3); + assert.equal(out.sql, "WITH base_pazienti AS (SELECT 1)"); + assert.equal(out.note, "nota"); +}); + +test("buildCteResultV2 prefers doc purpose/rationale/depends_on, falls back to thin data", () => { + const cteInfoWithDoc = { + name: "a", index: 1, total: 1, sql: "WITH a AS (SELECT 1)", + doc: { purpose: "scopo dal doc", rationale: "motivo dal doc", depends_on: ["x"] }, + last_test: { status: "ok", execution_ms: 1, row_sample: 0, warnings: [], sql_hash: "h", columns: [], preview_rows: [] }, + }; + const out1 = buildCteResultV2({ schema_version: 2, purpose: "scopo thin" }, cteInfoWithDoc); + assert.equal(out1.purpose, "scopo dal doc"); + assert.equal(out1.rationale, "motivo dal doc"); + assert.deepEqual(out1.depends_on, ["x"]); + + const cteInfoNoDoc = { ...cteInfoWithDoc, doc: null }; + const out2 = buildCteResultV2({ schema_version: 2, purpose: "scopo thin", rationale: "motivo thin" }, cteInfoNoDoc); + assert.equal(out2.purpose, "scopo thin"); + assert.equal(out2.rationale, "motivo thin"); +}); + +test("buildCteResultV2 builds preview from last_test.columns/preview_rows", () => { + const cteInfo = { + name: "a", index: 1, total: 1, sql: "WITH a AS (SELECT 1)", doc: null, + last_test: { status: "ok", execution_ms: 10, row_sample: 2, warnings: [], sql_hash: "h1", columns: ["cod_paz", "eta"], preview_rows: [[1, 30], [2, 45]] }, + }; + const out = buildCteResultV2({ schema_version: 2 }, cteInfo); + assert.deepEqual(out.preview, { columns: ["cod_paz", "eta"], rows: [[1, 30], [2, 45]] }); + assert.equal(out.status, "ok"); + assert.equal(out.execution_ms, 10); + assert.equal(out.row_sample, 2); + assert.equal(out.sql_hash, "h1"); + assert.deepEqual(out.warnings, []); +}); + +test("buildCteResultV2: columns[] carries name, with description left as an empty placeholder (filled separately by the gate)", () => { + const cteInfo = { + name: "a", index: 1, total: 1, sql: "WITH a AS (SELECT 1)", doc: null, + last_test: { status: "ok", execution_ms: 1, row_sample: 1, warnings: [], sql_hash: "h", columns: ["cod_paz"], preview_rows: [[1]] }, + }; + const out = buildCteResultV2({ schema_version: 2 }, cteInfo); + assert.equal(out.columns.length, 1); + assert.equal(out.columns[0].name, "cod_paz"); + assert.equal(out.columns[0].description, ""); +}); + +// --- enrichPhaseSummaryV2 ----------------------------------------------------------- + +test("enrichPhaseSummaryV2 fills phase from phaseMeta", () => { + const data = { schema_version: 2, summary: "riepilogo" }; + const phaseMeta = { id: "F5", num: 5, name: "sintesi" }; + const out = enrichPhaseSummaryV2(data, phaseMeta, () => null); + assert.deepEqual(out.phase, { id: "F5", num: 5, name: "sintesi" }); +}); + +test("enrichPhaseSummaryV2 enriches tables[]/tables[].columns[]/sections[].items[] descriptions", () => { + const getColumns = (table) => + table === "dim_patient" + ? { description: "Anagrafica", columns: [{ name: "cod_paz", description: "Codice paziente" }] } + : null; + const data = { + schema_version: 2, summary: "s", + tables: [{ name: "dim_patient", role: "promoted", columns: [{ name: "cod_paz" }] }], + sections: [{ title: "Criteri", items: [{ label: "id paziente", table: "dim_patient", column: "cod_paz" }] }], + }; + const out = enrichPhaseSummaryV2(data, { id: "F5", num: 5, name: "sintesi" }, getColumns); + assert.equal(out.tables[0].description, "Anagrafica"); + assert.equal(out.tables[0].columns[0].description, "Codice paziente"); + assert.equal(out.sections[0].items[0].description, "Codice paziente"); +}); + +test("enrichPhaseSummaryV2: catalog miss -> empty description, never throws", () => { + const data = { + schema_version: 2, summary: "s", + tables: [{ name: "nope", role: "excluded", columns: [{ name: "x" }] }], + sections: [{ title: "t", items: [{ label: "l", table: "nope", column: "x" }] }], + }; + const out = enrichPhaseSummaryV2(data, { id: "F5", num: 5, name: "sintesi" }, () => null); + assert.equal(out.tables[0].description, ""); + assert.equal(out.tables[0].columns[0].description, ""); + assert.equal(out.sections[0].items[0].description, ""); +}); + +test("enrichPhaseSummaryV2 preserves open_questions/checks untouched", () => { + const data = { + schema_version: 2, summary: "s", + checks: [{ label: "x", status: "ok" }], + open_questions: ["domanda aperta"], + }; + const out = enrichPhaseSummaryV2(data, { id: "F1", num: 1, name: "chiarimento" }, () => null); + assert.deepEqual(out.checks, [{ label: "x", status: "ok" }]); + assert.deepEqual(out.open_questions, ["domanda aperta"]); +}); diff --git a/harness/.pi/extensions/gate/__tests__/gate_confirm_cte_v2.test.js b/harness/.pi/extensions/gate/__tests__/gate_confirm_cte_v2.test.js new file mode 100644 index 00000000..06d1a8e5 --- /dev/null +++ b/harness/.pi/extensions/gate/__tests__/gate_confirm_cte_v2.test.js @@ -0,0 +1,420 @@ +// L1.5 roundtrip for reviewer_confirm's v2 payloads (cte_plan / cte_result / phase), +// driven end-to-end against a fake Pi runtime with `tht` (execFileSync) stubbed. +// Same harness adaptations as gate_roundtrip.test.js / gate_schema_linking.test.js: +// ESM-loaded-via-require + a globalThis.require shim for loadEnvFromDotenv. +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); +} + +// tht-gate.js binds `execFileSync` at module-eval time: an ESM `import` over a +// CommonJS require is a SNAPSHOT, not a live binding — reassigning cp.execFileSync +// AFTER the first gate load is invisible to the gate, and ESM modules can't be +// dropped from the require cache. So we install a PERMANENT dispatcher on `cp` +// exactly once (before the first gate load) that forwards to a mutable `activeStub`, +// and intercept every `cp.execFileSync = fn` assignment (via a property setter) to +// swap `activeStub` instead of clobbering the dispatcher. Each test's existing +// `cp.execFileSync = fn` / restore-in-finally then Just Works. +let activeStub = cp.execFileSync; +const dispatcher = (...args) => activeStub(...args); +Object.defineProperty(cp, "execFileSync", { + configurable: true, + get: () => dispatcher, + set: (fn) => { activeStub = fn; }, +}); + +async function loadGate() { + 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); + // Reset the module-level _phaseMetaCache so an earlier test's `tht phase meta` + // stub (with a different phase num) does not leak into this test. + await pi.emit("session_start", {}); + return { ctx, tools }; +} + +function approveVia(ctx, respBuilder) { + ctx.ui.input = async (title) => { + const descriptor = JSON.parse(title); + return JSON.stringify(respBuilder(descriptor)); + }; +} + +const CATALOG = { + dim_patient: { + table: "dim_patient", + description: "Anagrafica pazienti", + columns: [ + { name: "cod_paz", description: "Codice paziente", type: "bigint", pk: true }, + { name: "flag_attivo", description: "Flag attivo", type: "boolean", pk: false }, + ], + }, +}; + +// --- kind: cte_plan v2 -------------------------------------------------------------- + +test("reviewer_confirm kind:cte_plan (v2): enriches via catalog, derives names from ctes[], persists via `tht cte plan --doc -`", async () => { + const calls = []; + const origExecFileSync = cp.execFileSync; + cp.execFileSync = (file, args, opts) => { + calls.push({ args: args.join(" "), input: opts && opts.input }); + if (args[0] === "phase" && args[1] === "meta") + return JSON.stringify({ phases: [{ num: 6, id: "F6", name: "cte" }] }); + if (args[0] === "phase" && args[1] === "show") return "Fase corrente: 6\n"; + if (args[0] === "schema" && args[1] === "columns") + return JSON.stringify(CATALOG[args[2]] || {}); + return ""; + }; + try { + const { ctx, tools } = await loadGate(); + approveVia(ctx, (d) => ({ id: d.id, choices: ["approve"] })); + + const tool = tools.get("reviewer_confirm"); + assert.ok(tool, "reviewer_confirm must be registered"); + + const artifactData = { + schema_version: 2, + question: "domanda riscritta", + strategy: "razionale della catena", + ctes: [ + { name: "base_pazienti", purpose: "seleziona pazienti attivi", rationale: "base", depends_on: [], tables: [{ name: "dim_patient" }], filters: [{ column: "dim_patient.flag_attivo", op: "IS", value: "TRUE" }], keys: ["cod_paz"], output_columns: ["cod_paz"] }, + ], + }; + + const result = await tool.def.execute( + "call-1", + { + session: "s1", + kind: "cte_plan", + title: "Piano CTE", + artifact: { kind: "cte_plan", data: artifactData }, + }, + null, null, ctx, + ); + + const planCall = calls.find((c) => c.args.startsWith("cte plan --session s1")); + assert.ok(planCall, `expected a 'cte plan' call in: ${JSON.stringify(calls.map((c) => c.args))}`); + assert.match(planCall.args, /--name base_pazienti/); + assert.match(planCall.args, /--doc -/); + const pipedDoc = JSON.parse(planCall.input); + assert.equal(pipedDoc.ctes[0].tables[0].description, "Anagrafica pazienti"); + assert.equal(pipedDoc.ctes[0].filters[0].description, "Flag attivo"); + assert.equal(pipedDoc.ctes[0].index, 1); + + assert.match(result.content[0].text, /CTE plan approvato/); + } finally { + cp.execFileSync = origExecFileSync; + } +}); + +test("reviewer_confirm kind:cte_plan (v2): invalid payload returns textResult with errors (model self-corrects), never throws", async () => { + const origExecFileSync = cp.execFileSync; + cp.execFileSync = (file, args) => { + if (args[0] === "phase" && args[1] === "meta") + return JSON.stringify({ phases: [{ num: 6, id: "F6", name: "cte" }] }); + if (args[0] === "phase" && args[1] === "show") return "Fase corrente: 6\n"; + return ""; + }; + try { + const { ctx, tools } = await loadGate(); + const tool = tools.get("reviewer_confirm"); + + const result = await tool.def.execute( + "call-1", + { + session: "s1", + kind: "cte_plan", + title: "Piano CTE", + artifact: { kind: "cte_plan", data: { schema_version: 2, question: "q", strategy: "s" } }, // missing ctes + }, + null, null, ctx, + ); + assert.match(result.content[0].text, /ctes/); + } finally { + cp.execFileSync = origExecFileSync; + } +}); + +test("reviewer_confirm kind:cte_plan legacy (non-v2 data): unchanged path via params.names", async () => { + const calls = []; + const origExecFileSync = cp.execFileSync; + cp.execFileSync = (file, args) => { + calls.push(args.join(" ")); + if (args[0] === "phase" && args[1] === "meta") + return JSON.stringify({ phases: [{ num: 6, id: "F6", name: "cte" }] }); + if (args[0] === "phase" && args[1] === "show") return "Fase corrente: 6\n"; + return ""; + }; + try { + const { ctx, tools } = await loadGate(); + approveVia(ctx, (d) => ({ id: d.id, choices: ["approve"] })); + const tool = tools.get("reviewer_confirm"); + + const result = await tool.def.execute( + "call-1", + { + session: "s1", + kind: "cte_plan", + title: "Piano CTE", + artifact: { kind: "cte_plan", data: "### Piano CTE legacy" }, + names: ["a", "b"], + }, + null, null, ctx, + ); + assert.ok(calls.some((c) => c === "cte plan --session s1 --name a --name b")); + assert.match(result.content[0].text, /CTE plan approvato/); + } finally { + cp.execFileSync = origExecFileSync; + } +}); + +// --- kind: cte_result v2 ------------------------------------------------------------- + +test("reviewer_confirm kind:cte_result (v2 thin): builds full payload via `tht cte next`/`tht cte info`, enriches columns, emitted descriptor has sql + preview", async () => { + const calls = []; + const origExecFileSync = cp.execFileSync; + cp.execFileSync = (file, args) => { + calls.push(args.join(" ")); + if (args[0] === "phase" && args[1] === "meta") + return JSON.stringify({ phases: [{ num: 6, id: "F6", name: "cte" }] }); + if (args[0] === "phase" && args[1] === "show") return "Fase corrente: 6\n"; + if (args[0] === "cte" && args[1] === "next") return "base_pazienti\n"; + if (args[0] === "cte" && args[1] === "info") + return JSON.stringify({ + name: "base_pazienti", index: 1, total: 2, + plan: ["base_pazienti", "eventi"], + sql: "WITH base_pazienti AS (SELECT cod_paz FROM dim_patient WHERE flag_attivo)", + approved: false, + doc: { purpose: "seleziona pazienti", rationale: "base", depends_on: [] }, + last_test: { + name: "base_pazienti", ts: "2026-01-01T00:00:00Z", sql_hash: "h1", status: "ok", + columns: ["cod_paz"], row_sample: 1, execution_ms: 12, warnings: [], preview_rows: [[1]], + }, + }); + if (args[0] === "schema" && args[1] === "columns") + return JSON.stringify(CATALOG[args[2]] || {}); + return ""; + }; + try { + const { ctx, tools } = await loadGate(); + let capturedDescriptor = null; + ctx.ui.input = async (title) => { + capturedDescriptor = JSON.parse(title); + return JSON.stringify({ id: capturedDescriptor.id, choices: ["approve"] }); + }; + const tool = tools.get("reviewer_confirm"); + + const result = await tool.def.execute( + "call-1", + { + session: "s1", + kind: "cte_result", + title: "Esito CTE", + artifact: { kind: "cte_result", data: { schema_version: 2, note: "prima passata ok" } }, + }, + null, null, ctx, + ); + + assert.equal(capturedDescriptor.artifact.data.sql, "WITH base_pazienti AS (SELECT cod_paz FROM dim_patient WHERE flag_attivo)"); + assert.deepEqual(capturedDescriptor.artifact.data.preview, { columns: ["cod_paz"], rows: [[1]] }); + assert.equal(capturedDescriptor.artifact.data.name, "base_pazienti"); + assert.ok(calls.some((c) => c === "cte next --session s1")); + assert.ok(calls.some((c) => c.startsWith("cte info base_pazienti --session s1"))); + assert.match(result.content[0].text, /base_pazienti.*approvato/s); + } finally { + cp.execFileSync = origExecFileSync; + } +}); + +test("reviewer_confirm kind:cte_result (v2): last_test null -> clear textResult error, no widget shown", async () => { + const origExecFileSync = cp.execFileSync; + cp.execFileSync = (file, args) => { + if (args[0] === "phase" && args[1] === "meta") + return JSON.stringify({ phases: [{ num: 6, id: "F6", name: "cte" }] }); + if (args[0] === "phase" && args[1] === "show") return "Fase corrente: 6\n"; + if (args[0] === "cte" && args[1] === "next") return "base_pazienti\n"; + if (args[0] === "cte" && args[1] === "info") + return JSON.stringify({ + name: "base_pazienti", index: 1, total: 1, plan: ["base_pazienti"], + sql: "WITH base_pazienti AS (SELECT 1)", approved: false, doc: null, last_test: null, + }); + return ""; + }; + try { + const { ctx, tools } = await loadGate(); + let uiCalled = false; + ctx.ui.input = async () => { uiCalled = true; return undefined; }; + const tool = tools.get("reviewer_confirm"); + + const result = await tool.def.execute( + "call-1", + { + session: "s1", + kind: "cte_result", + title: "Esito CTE", + artifact: { kind: "cte_result", data: { schema_version: 2 } }, + }, + null, null, ctx, + ); + assert.equal(uiCalled, false, "the gate must not present a widget without a test outcome"); + assert.match(result.content[0].text, /tht cte test/); + } finally { + cp.execFileSync = origExecFileSync; + } +}); + +test("reviewer_confirm kind:cte_result (v2): last_test status error -> clear textResult error", async () => { + const origExecFileSync = cp.execFileSync; + cp.execFileSync = (file, args) => { + if (args[0] === "phase" && args[1] === "meta") + return JSON.stringify({ phases: [{ num: 6, id: "F6", name: "cte" }] }); + if (args[0] === "phase" && args[1] === "show") return "Fase corrente: 6\n"; + if (args[0] === "cte" && args[1] === "next") return "base_pazienti\n"; + if (args[0] === "cte" && args[1] === "info") + return JSON.stringify({ + name: "base_pazienti", index: 1, total: 1, plan: ["base_pazienti"], + sql: "WITH base_pazienti AS (SELECT 1)", approved: false, doc: null, + last_test: { name: "base_pazienti", ts: "t", sql_hash: "h", status: "error", error: "colonna inesistente" }, + }); + return ""; + }; + try { + const { ctx, tools } = await loadGate(); + let uiCalled = false; + ctx.ui.input = async () => { uiCalled = true; return undefined; }; + const tool = tools.get("reviewer_confirm"); + + const result = await tool.def.execute( + "call-1", + { + session: "s1", kind: "cte_result", title: "Esito CTE", + artifact: { kind: "cte_result", data: { schema_version: 2 } }, + }, + null, null, ctx, + ); + assert.equal(uiCalled, false); + assert.match(result.content[0].text, /tht cte test/); + } finally { + cp.execFileSync = origExecFileSync; + } +}); + +test("reviewer_confirm kind:cte_result legacy (markdown string data): unchanged path (cte_approved by next name)", async () => { + const calls = []; + const origExecFileSync = cp.execFileSync; + cp.execFileSync = (file, args) => { + calls.push(args.join(" ")); + if (args[0] === "phase" && args[1] === "meta") + return JSON.stringify({ phases: [{ num: 6, id: "F6", name: "cte" }] }); + if (args[0] === "phase" && args[1] === "show") return "Fase corrente: 6\n"; + if (args[0] === "cte" && args[1] === "next") return "base_pazienti\n"; + return ""; + }; + try { + const { ctx, tools } = await loadGate(); + approveVia(ctx, (d) => ({ id: d.id, choices: ["approve"] })); + const tool = tools.get("reviewer_confirm"); + + const result = await tool.def.execute( + "call-1", + { + session: "s1", kind: "cte_result", title: "Esito CTE", + artifact: { kind: "cte_result", data: "### Esito CTE\nOK, 5 righe." }, + }, + null, null, ctx, + ); + assert.ok(calls.some((c) => c === "decision add --session s1 --type cte_approved --subject base_pazienti")); + assert.match(result.content[0].text, /base_pazienti.*approvato/); + } finally { + cp.execFileSync = origExecFileSync; + } +}); + +// --- kind: phase v2 -------------------------------------------------------------- + +test("reviewer_confirm kind:phase (v2): fills phase from phaseMeta and enriches descriptions", async () => { + const origExecFileSync = cp.execFileSync; + cp.execFileSync = (file, args) => { + if (args[0] === "phase" && args[1] === "meta") + return JSON.stringify({ phases: [{ num: 5, id: "F5", name: "sintesi" }] }); + if (args[0] === "phase" && args[1] === "show") return "Fase corrente: 5\n"; + if (args[0] === "phase" && args[1] === "advance") return ""; + if (args[0] === "schema" && args[1] === "columns") + return JSON.stringify(CATALOG[args[2]] || {}); + return ""; + }; + try { + const { ctx, tools } = await loadGate(); + let capturedDescriptor = null; + ctx.ui.input = async (title) => { + capturedDescriptor = JSON.parse(title); + return JSON.stringify({ id: capturedDescriptor.id, choices: ["approve"] }); + }; + const tool = tools.get("reviewer_confirm"); + + const result = await tool.def.execute( + "call-1", + { + session: "s1", kind: "phase", title: "Chiusura fase", + artifact: { + kind: "phase", + data: { + schema_version: 2, + summary: "riepilogo sintesi", + tables: [{ name: "dim_patient", role: "promoted", columns: [{ name: "cod_paz" }] }], + }, + }, + }, + null, null, ctx, + ); + + assert.deepEqual(capturedDescriptor.artifact.data.phase, { num: 5, id: "F5", name: "sintesi" }); + assert.equal(capturedDescriptor.artifact.data.tables[0].description, "Anagrafica pazienti"); + assert.equal(capturedDescriptor.artifact.data.tables[0].columns[0].description, "Codice paziente"); + assert.match(result.content[0].text, /Fase approvata/); + } finally { + cp.execFileSync = origExecFileSync; + } +}); + +test("reviewer_confirm kind:phase legacy (non-v2 data): unchanged path", async () => { + const origExecFileSync = cp.execFileSync; + cp.execFileSync = (file, args) => { + if (args[0] === "phase" && args[1] === "meta") + return JSON.stringify({ phases: [{ num: 1, id: "F1", name: "chiarimento" }] }); + if (args[0] === "phase" && args[1] === "show") return "Fase corrente: 1\n"; + if (args[0] === "phase" && args[1] === "advance") return ""; + return ""; + }; + try { + const { ctx, tools } = await loadGate(); + let capturedDescriptor = null; + ctx.ui.input = async (title) => { + capturedDescriptor = JSON.parse(title); + return JSON.stringify({ id: capturedDescriptor.id, choices: ["approve"] }); + }; + const tool = tools.get("reviewer_confirm"); + + const result = await tool.def.execute( + "call-1", + { + session: "s1", kind: "phase", title: "Chiusura fase", + artifact: { kind: "phase", data: { recap: "legacy recap text" } }, + }, + null, null, ctx, + ); + assert.deepEqual(capturedDescriptor.artifact.data, { recap: "legacy recap text" }); + assert.match(result.content[0].text, /Fase approvata/); + } finally { + cp.execFileSync = origExecFileSync; + } +}); diff --git a/harness/.pi/extensions/gate/__tests__/gate_stringified_params.test.js b/harness/.pi/extensions/gate/__tests__/gate_stringified_params.test.js index 3e87e645..1c24c6c3 100644 --- a/harness/.pi/extensions/gate/__tests__/gate_stringified_params.test.js +++ b/harness/.pi/extensions/gate/__tests__/gate_stringified_params.test.js @@ -46,6 +46,33 @@ test("prepareReviewerArguments parses stringified tables array", () => { assert.equal(out.tables[0].id, "t1"); }); +// WS2: GLM sometimes double-stringifies -- artifact.data itself arrives as a JSON +// string even after artifact is (or already was) an object. Coerce it too. +test("prepareReviewerArguments coerces a stringified artifact.data into an object (GLM case)", () => { + const out = prepareReviewerArguments({ + artifact: { kind: "cte_plan", data: JSON.stringify({ schema_version: 2, ctes: [] }) }, + }); + assert.equal(typeof out.artifact.data, "object"); + assert.equal(out.artifact.data.schema_version, 2); + assert.deepEqual(out.artifact.data.ctes, []); +}); + +test("prepareReviewerArguments coerces artifact.data even when artifact itself arrived stringified", () => { + const out = prepareReviewerArguments({ + artifact: JSON.stringify({ kind: "phase", data: JSON.stringify({ schema_version: 2, summary: "s" }) }), + }); + assert.equal(typeof out.artifact, "object"); + assert.equal(typeof out.artifact.data, "object"); + assert.equal(out.artifact.data.summary, "s"); +}); + +test("prepareReviewerArguments leaves a legacy markdown string artifact.data UNCHANGED", () => { + const out = prepareReviewerArguments({ + artifact: { kind: "cte_result", data: "### Risultato CTE\nOK, 5 righe." }, + }); + assert.equal(out.artifact.data, "### Risultato CTE\nOK, 5 righe."); +}); + test("GATE_CODE_FILES blocks writes to gate extensions, not session artifacts", () => { assert.ok(GATE_CODE_FILES.test("harness/.pi/extensions/tht-gate.js")); assert.ok(GATE_CODE_FILES.test(".pi/extensions/reserved-labels.mjs")); diff --git a/harness/.pi/extensions/gate/artifact-contracts.js b/harness/.pi/extensions/gate/artifact-contracts.js new file mode 100644 index 00000000..116b2b93 --- /dev/null +++ b/harness/.pi/extensions/gate/artifact-contracts.js @@ -0,0 +1,125 @@ +// artifact-contracts.js -- PURE soft-validators for the v2 gate payloads (WS2). +// +// No I/O, no TypeBox: hand-rolled, permissive on extra fields. Each validator +// returns { ok: boolean, errors: string[] } with actionable messages so the model +// can self-correct (the gate returns them as a textResult, never a hard TypeBox +// failure -- models loop when validation fails BEFORE execute()). +// +// A payload without `schema_version === 2` is NOT an error: it returns +// { ok: true, legacy: true } and the caller proceeds on the legacy path. + +"use strict"; + +function isNonEmptyString(v) { + return typeof v === "string" && v.length > 0; +} + +function isArray(v) { + return Array.isArray(v); +} + +const VALID_CHECK_STATUS = new Set(["ok", "warn", "fail"]); + +// Contract A -- cte_plan v2. Validates the model-owned skeleton; catalog-derived +// fields (index, table/filter descriptions) are filled later by enrich, so they +// are NOT required here. +function validateCtePlanV2(data) { + if (!data || typeof data !== "object" || data.schema_version !== 2) { + return { ok: true, legacy: true }; + } + const errors = []; + if (!isArray(data.ctes) || data.ctes.length === 0) { + errors.push("ctes deve essere un array non vuoto."); + return { ok: false, errors }; + } + const seen = []; + data.ctes.forEach((c, i) => { + if (!c || typeof c !== "object") { + errors.push(`ctes[${i}] deve essere un oggetto.`); + return; + } + if (!isNonEmptyString(c.name)) { + errors.push(`ctes[${i}].name mancante o vuoto.`); + } + if (isArray(c.depends_on)) { + for (const dep of c.depends_on) { + if (!seen.includes(dep)) { + errors.push( + `ctes[${i}].depends_on contiene '${dep}' che non è un CTE precedente.`, + ); + } + } + } + if (isNonEmptyString(c.name)) seen.push(c.name); + }); + return { ok: errors.length === 0, errors }; +} + +// Contract B -- cte_result THIN (what the model sends). The gate builds the full +// payload from `tht cte info`; the model only supplies {purpose?, rationale?, note?}. +// Everything is optional, so a thin v2 artifact is valid as long as it declares v2. +function validateCteResultThin(data) { + if (!data || typeof data !== "object" || data.schema_version !== 2) { + return { ok: true, legacy: true }; + } + const errors = []; + for (const field of ["purpose", "rationale", "note"]) { + if (data[field] !== undefined && typeof data[field] !== "string") { + errors.push(`${field} deve essere una stringa se presente.`); + } + } + return { ok: errors.length === 0, errors }; +} + +// Contract C -- phase summary v2. +function validatePhaseSummaryV2(data) { + if (!data || typeof data !== "object" || data.schema_version !== 2) { + return { ok: true, legacy: true }; + } + const errors = []; + if (!isNonEmptyString(data.summary)) { + errors.push("summary mancante o vuoto."); + } + if (data.checks !== undefined) { + if (!isArray(data.checks)) { + errors.push("checks deve essere un array se presente."); + } else { + data.checks.forEach((c, i) => { + if (!c || typeof c !== "object" || !isNonEmptyString(c.label)) { + errors.push(`checks[${i}].label mancante o vuoto.`); + } + if (!c || !VALID_CHECK_STATUS.has(c.status)) { + errors.push(`checks[${i}].status deve essere uno tra ok|warn|fail.`); + } + }); + } + } + if (data.sections !== undefined) { + if (!isArray(data.sections)) { + errors.push("sections deve essere un array se presente."); + } else { + data.sections.forEach((s, i) => { + if (!s || typeof s !== "object") { + errors.push(`sections[${i}] deve essere un oggetto.`); + return; + } + if (!isNonEmptyString(s.title)) { + errors.push(`sections[${i}].title mancante o vuoto.`); + } + if (s.items !== undefined && !isArray(s.items)) { + errors.push(`sections[${i}].items deve essere un array se presente.`); + } + }); + } + } + if (data.tables !== undefined && !isArray(data.tables)) { + errors.push("tables deve essere un array se presente."); + } + return { ok: errors.length === 0, errors }; +} + +module.exports = { + validateCtePlanV2, + validateCteResultThin, + validatePhaseSummaryV2, +}; diff --git a/harness/.pi/extensions/gate/enrich.js b/harness/.pi/extensions/gate/enrich.js new file mode 100644 index 00000000..fd629d0f --- /dev/null +++ b/harness/.pi/extensions/gate/enrich.js @@ -0,0 +1,160 @@ +// enrich.js -- PURE merge/enrichment for the v2 gate payloads (WS2). +// +// Catalog lookups are INJECTED as `getColumns` params (for testability): a function +// (tableName) => { description, columns: [{ name, description, ... }] } | null. +// A catalog miss (null) yields empty descriptions, never an error (the hard-fail +// stays only in reviewer_schema_linking). +// +// No dependency on tht-gate.js: importable and testable in isolation. + +"use strict"; + +// Memoize a raw catalog lookup so repeated table hits do a single I/O call. The +// gate wraps its `tht schema columns --json` (try/catch -> null on miss) and +// passes it here; enrich itself stays pure. +function memoizeGetColumns(rawGetColumns) { + const cache = new Map(); + return function getColumns(tableName) { + if (cache.has(tableName)) return cache.get(tableName); + const v = rawGetColumns(tableName); + cache.set(tableName, v); + return v; + }; +} + +function columnDescription(cat, colName) { + if (!cat || !Array.isArray(cat.columns)) return ""; + const col = cat.columns.find((c) => c.name === colName); + return (col && col.description) || ""; +} + +// Contract A. Fills index (1-based), tables[].description and filters[].description +// via getColumns. For filters, `column: "tab.col"` resolves to the column's catalog +// description. Returns a NEW object; model-owned fields pass through unchanged. +function enrichCtePlanV2(data, getColumns) { + const ctes = (data.ctes || []).map((cte, i) => { + const out = { ...cte, index: i + 1 }; + if (Array.isArray(cte.tables)) { + out.tables = cte.tables.map((t) => { + const cat = getColumns(t.name); + return { ...t, description: (cat && cat.description) || "" }; + }); + } + if (Array.isArray(cte.filters)) { + out.filters = cte.filters.map((f) => { + const out2 = { ...f }; + if (typeof f.column === "string" && f.column.includes(".")) { + const idx = f.column.indexOf("."); + const tableName = f.column.slice(0, idx); + const colName = f.column.slice(idx + 1); + out2.description = columnDescription(getColumns(tableName), colName); + } else { + out2.description = ""; + } + return out2; + }); + } + return out; + }); + return { ...data, ctes }; +} + +// Contract B. Fuses the model's thin data with `tht cte info` output. +// name/index/total/sql <- cteInfo +// purpose/rationale/depends_on <- cteInfo.doc (fallback: thinData) +// status/execution_ms/row_sample/warnings/sql_hash/preview <- cteInfo.last_test +// note <- thinData +// preview = { columns: last_test.columns, rows: last_test.preview_rows }. +function buildCteResultV2(thinData, cteInfo) { + const thin = thinData || {}; + const doc = cteInfo.doc || {}; + const lt = cteInfo.last_test || {}; + const pick = (docVal, thinVal) => (docVal !== undefined ? docVal : thinVal); + return { + schema_version: 2, + name: cteInfo.name, + index: cteInfo.index, + total: cteInfo.total, + purpose: pick(doc.purpose, thin.purpose), + rationale: pick(doc.rationale, thin.rationale), + depends_on: pick(doc.depends_on, thin.depends_on), + sql: cteInfo.sql, + status: lt.status, + execution_ms: lt.execution_ms, + row_sample: lt.row_sample, + warnings: lt.warnings || [], + sql_hash: lt.sql_hash, + columns: (lt.columns || []).map((name) => ({ name, description: "" })), + preview: { columns: lt.columns || [], rows: lt.preview_rows || null }, + note: thin.note, + }; +} + +// Contract C. Fills `phase` from phaseMeta and the `description` fields in +// tables[]/tables[].columns[]/sections[].items[] via getColumns. phaseMeta is +// { id, num, name }. Returns a NEW object. +function enrichPhaseSummaryV2(data, phaseMeta, getColumns) { + const out = { ...data, phase: phaseMeta }; + if (Array.isArray(data.tables)) { + out.tables = data.tables.map((t) => { + const cat = getColumns(t.name); + const enriched = { ...t, description: (cat && cat.description) || "" }; + if (Array.isArray(t.columns)) { + enriched.columns = t.columns.map((c) => ({ + ...c, + description: columnDescription(cat, c.name), + })); + } + return enriched; + }); + } + if (Array.isArray(data.sections)) { + out.sections = data.sections.map((s) => { + if (!Array.isArray(s.items)) return { ...s }; + return { + ...s, + items: s.items.map((it) => { + // An item's description resolves against its declared table+column. + let desc = ""; + if (typeof it.table === "string" && typeof it.column === "string") { + desc = columnDescription(getColumns(it.table), it.column); + } else if (typeof it.table === "string") { + const cat = getColumns(it.table); + desc = (cat && cat.description) || ""; + } + return { ...it, description: desc }; + }), + }; + }); + } + return out; +} + +// Fills columns[].description of a built cte_result payload by matching each column +// name against the catalog of the plan's tables (`planTables`: string[]). First +// table whose catalog carries the column wins; unresolved -> empty description. +// Mutates a copy of payload.columns; returns the payload. +function enrichCteResultColumns(payload, planTables, getColumns) { + if (!Array.isArray(payload.columns)) return payload; + const tables = Array.isArray(planTables) ? planTables : []; + const columns = payload.columns.map((col) => { + let desc = ""; + for (const t of tables) { + const d = columnDescription(getColumns(t), col.name); + if (d) { + desc = d; + break; + } + } + return { ...col, description: desc }; + }); + return { ...payload, columns }; +} + +module.exports = { + memoizeGetColumns, + enrichCtePlanV2, + buildCteResultV2, + enrichCteResultColumns, + enrichPhaseSummaryV2, +}; diff --git a/harness/.pi/extensions/tht-gate.js b/harness/.pi/extensions/tht-gate.js index efef59a8..8e89418e 100644 --- a/harness/.pi/extensions/tht-gate.js +++ b/harness/.pi/extensions/tht-gate.js @@ -31,6 +31,18 @@ import { buildArtifactGate, buildSchemaLinkingRequest, } from "./gate/builders.js"; +import { + validateCtePlanV2, + validateCteResultThin, + validatePhaseSummaryV2, +} from "./gate/artifact-contracts.js"; +import { + memoizeGetColumns, + enrichCtePlanV2, + buildCteResultV2, + enrichCteResultColumns, + enrichPhaseSummaryV2, +} from "./gate/enrich.js"; import { isReserved } from "./reserved-labels.mjs"; // --- prepareArguments: parse stringified arrays (workaround for models that send @@ -80,6 +92,13 @@ export function prepareReviewerArguments(input) { // stringified JSON object, which fails validation BEFORE execute() and makes the // model loop. Coerce it back to an object so the gate proceeds. if (args.artifact !== undefined) args.artifact = jsonObjectOrSelf(args.artifact); + // GLM sometimes double-stringifies: artifact.data itself arrives as a JSON + // string (v2 payloads) even once artifact is an object. Coerce it too; a + // legacy markdown string in artifact.data is not valid JSON and jsonObjectOrSelf + // already leaves non-JSON strings unchanged. + if (args.artifact && typeof args.artifact === "object" && args.artifact.data !== undefined) { + args.artifact = { ...args.artifact, data: jsonObjectOrSelf(args.artifact.data) }; + } return args; } @@ -210,6 +229,24 @@ function phaseId(ctx, num) { const p = meta.phases.find((x) => x.num === num); return p ? p.id : "?"; } +// Full phase descriptor {id, num, name} for the v2 phase-summary payload's `phase`. +function phaseMetaForNum(ctx, num) { + const meta = phaseMeta(ctx); + const p = meta.phases.find((x) => x.num === num); + return p ? { id: p.id, num: p.num, name: p.name } : { id: "?", num, name: "" }; +} +// Memoized, never-throwing catalog lookup for enrich: `tht schema columns --json` +// -> {description, columns:[...]} or null on any miss (same hardened pattern as +// reviewer_schema_linking, but a miss is a soft null here, not a hard fail). +function makeGetColumns(ctx) { + return memoizeGetColumns((tableName) => { + try { + return JSON.parse(tht(ctx, ["schema", "columns", tableName, "--json"])); + } catch { + return null; + } + }); +} function currentPhase(ctx, session) { const out = tht(ctx, ["phase", "show", "--session", session]); const m = out.match(/Fase corrente:\s*(\d+)/); @@ -753,8 +790,86 @@ export default function (pi) { prepareArguments: prepareReviewerArguments, async execute(_id, params, _signal, _onUpdate, ctx) { lockActive = true; - const { session, kind, title, artifact } = params; - const phase = phaseId(ctx, currentPhase(ctx, session)); + const { session, kind, title } = params; + let artifact = params.artifact; + const curNum = currentPhase(ctx, session); + const phase = phaseId(ctx, curNum); + + // --- v2 payload construction (WS2) ------------------------------------- + // The gate builds/validates the structured payload BEFORE showing the + // widget, so the reviewer approves a deterministic artifact (not the raw + // model data). Legacy (non-v2) payloads fall through unchanged. + const data = artifact && typeof artifact === "object" ? artifact.data : undefined; + const isV2 = data && typeof data === "object" && data.schema_version === 2; + + // derived from the (possibly rebuilt) cte_plan doc; the names list to persist. + let ctePlanNames = null; + + if (kind === "cte_plan" && isV2) { + const v = validateCtePlanV2(data); + if (!v.legacy && !v.ok) { + return textResult( + "Piano CTE v2 non valido:\n- " + v.errors.join("\n- ") + + "\nCorreggi il payload e ripresenta il gate.", + ); + } + const getColumns = makeGetColumns(ctx); + const enriched = enrichCtePlanV2(data, getColumns); + ctePlanNames = enriched.ctes.map((c) => c.name); + artifact = { ...artifact, data: enriched }; + } else if (kind === "cte_result" && isV2) { + const thinCheck = validateCteResultThin(data); + if (!thinCheck.legacy && !thinCheck.ok) { + return textResult( + "Dati cte_result non validi:\n- " + thinCheck.errors.join("\n- "), + ); + } + const cteName = tht(ctx, ["cte", "next", "--session", session]).trim(); + if (!cteName) { + return textResult( + `Nessun CTE in attesa di approvazione (sessione ${session}).`, + ); + } + let cteInfo; + try { + cteInfo = JSON.parse( + tht(ctx, ["cte", "info", cteName, "--session", session, "--json"]), + ); + } catch (e) { + const msg = (e.stderr || e.message || String(e)).toString().trim(); + return textResult( + `Impossibile leggere le info del CTE '${cteName}': ${msg}`, + ); + } + if (!cteInfo.last_test || cteInfo.last_test.status === "error") { + return textResult( + `Il CTE '${cteName}' non ha un test con esito ok: devi eseguire ` + + "`tht cte test` con esito ok prima di presentare il gate cte_result.", + ); + } + let built = buildCteResultV2(data, cteInfo); + // Enrich column descriptions against the plan's tables (best-effort). + const planTables = []; + if (cteInfo.doc && Array.isArray(cteInfo.doc.tables)) { + for (const t of cteInfo.doc.tables) if (t && t.name) planTables.push(t.name); + } + built = enrichCteResultColumns(built, planTables, makeGetColumns(ctx)); + artifact = { ...artifact, data: built }; + } else if (kind === "phase" && isV2) { + const v = validatePhaseSummaryV2(data); + if (!v.legacy && !v.ok) { + return textResult( + "Riepilogo di fase v2 non valido:\n- " + v.errors.join("\n- "), + ); + } + const enriched = enrichPhaseSummaryV2( + data, + phaseMetaForNum(ctx, curNum), + makeGetColumns(ctx), + ); + artifact = { ...artifact, data: enriched }; + } + const widget = buildArtifactGate({ id: `u${Date.now()}`, phase, @@ -793,19 +908,32 @@ export default function (pi) { return textResult(`Fase approvata (sessione ${session}).`); } if (kind === "cte_plan") { - // The CTE plan is the ordered list of CTE names; the model passes them in - // params.names (tht cte plan requires at least one --name). - const names = Array.isArray(params.names) ? params.names : []; + // v2: names derive from data.ctes[]; persist the plan AND the chain doc + // (payload A, enriched) via `tht cte plan --name ... --doc -`. + // Legacy: the ordered names come from params.names. + const names = ctePlanNames ?? (Array.isArray(params.names) ? params.names : []); if (names.length === 0) { return textResult( "Nessun nome CTE fornito: il piano CTE richiede l'elenco ordinato dei CTE " + - "(parametro names di reviewer_confirm).", + "(parametro names di reviewer_confirm, o data.ctes[].name nel payload v2).", ); } const planArgs = ["cte", "plan", "--session", session]; for (const n of names) planArgs.push("--name", n); - const err = relayIfThtFails(ctx, planArgs, ""); - if (err) return err; + if (ctePlanNames) { + // v2: pass the enriched doc on stdin; the CLI validates the name list + // matches (same order) and writes cte_plan_doc.json. + planArgs.push("--doc", "-"); + try { + tht(ctx, planArgs, JSON.stringify(artifact.data)); + } catch (e) { + const cliMsg = (e.stderr || e.message || String(e)).toString().trim(); + return textResult(`${cliMsg} Correggi il piano CTE e riprova.`); + } + } else { + const err = relayIfThtFails(ctx, planArgs, ""); + if (err) return err; + } return textResult( `CTE plan approvato (${names.length} CTE, sessione ${session}).`, ); diff --git a/harness/.pi/skills/tht-sessione/SKILL.md b/harness/.pi/skills/tht-sessione/SKILL.md index 61e24649..89fc198c 100644 --- a/harness/.pi/skills/tht-sessione/SKILL.md +++ b/harness/.pi/skills/tht-sessione/SKILL.md @@ -78,13 +78,38 @@ substantive decisions. in the `message` (or in the `options`' labels/descriptions) a concise recap of the context the reviewer needs to decide: what was asked, what you found, what each option means. The reviewer does not see your internal reasoning — only the widget. + For a phase-closing gate (`reviewer_confirm kind:"phase"`) prefer the **structured + v2 recap** `artifact:{kind:"phase", data:{schema_version:2, …}}`: you author + `summary` (1-3 sentence markdown), `checks[]`, `sections[]` and `tables[]`; the gate + fills `phase` (from workflow meta) and every `description` from the catalog. Every + `sections[].items[]` MUST cite the concrete **table**, **column** and the **value** + that motivates the choice (booleans, time windows, thresholds) — not just prose. + Compact example: + ```json + {"schema_version":2,"summary":"Selezionati pazienti attivi con ricoveri nel 2023.", + "checks":[{"label":"schema_linking valido","status":"ok"}], + "sections":[{"title":"Criteri di selezione","items":[ + {"label":"solo pazienti attivi","table":"dim_patient","column":"flag_attivo", + "value":"IS TRUE","kind":"filter","rationale":"esclude i cessati"}, + {"label":"finestra temporale","table":"dim_time","column":"year", + "value":"= 2023","kind":"filter","rationale":"anno richiesto"}]}], + "tables":[{"name":"dim_patient","role":"promoted", + "columns":[{"name":"cod_paz","value_filter":""}]}], + "open_questions":[]} + ``` + Legacy free-text recaps still work (no `schema_version`), but prefer v2. Note: the + F4 schema-linking recap travels in `tables` of this v2 phase payload — do NOT reuse + `kind:"schema_linking"` for a phase recap. 7. **Artifact = first-class output.** `schema_linking.json`, `cte_plan.json`, `ctes/*.sql`, `sql_final.sql` are produced and reviewed explicitly, never hidden. - For CTE (`kind:"cte_result"`) and final SQL (`kind:"sql"`) the gate **reads the - file from disk and shows it integral** to the reviewer — so the file content is - what the reviewer approves. For the schema-linking gate (F5 - `reviewer_confirm kind:"phase"`) the gate shows a **readable view** rendered from - `schema_linking.json`. Write the artifacts with care; they are the decision surface. + For a v2 `kind:"cte_result"` gate the gate **rebuilds the artifact from + deterministic sources** (`tht cte info`: the persisted `.sql` + the last CTE + test record) — you send only the thin `{purpose?, rationale?, note?}` and the + reviewer approves the gate-built payload, not your text. For final SQL + (`kind:"sql"`) the gate reads `sql_final.sql` from disk and shows it integral. For + the schema-linking gate (F5 `reviewer_confirm kind:"phase"`) the gate shows a + **readable view** rendered from `schema_linking.json`. Write the artifacts with + care; they are the decision surface. 8. **Candidates are candidates, not truth.** Present LSH/vector/evidence matches with their **provenance** (LSH / vector / evidence) and their scores, never as absolute truth. The reviewer may reject them. Verify filter values with `tht search find @@ -260,13 +285,39 @@ Prerequisite: Phase 5 closed. 1. Read `cte.md`. Decompose the rewritten question into CTEs (Agent View Generation): each CTE captures an informative subset with a clear purpose, named in snake_case. -2. Present the full CTE plan to the reviewer (`reviewer_decide` with the plan). +2. Present the full CTE plan to the reviewer with `reviewer_confirm kind:"cte_plan"`, + passing a **structured v2 artifact** (`artifact:{kind:"cte_plan", data:{…}}`). You + author `question`, `strategy` and each `ctes[]` entry (`name`, `purpose`, + `rationale`, `depends_on`, `tables[].name`, `keys`, `filters[]` with + `column`/`op`/`value`/`rationale`, `output_columns`); the gate fills `index` + (1-based) and every `description` from the catalog, derives the ordered `--name` + list from `data.ctes[].name`, and on approval persists both `cte_plan.json` and the + chain doc (`cte_plan_doc.json`). Compact example (2 CTE): + ```json + {"schema_version":2,"question":"pazienti attivi con almeno un ricovero nel 2023", + "strategy":"prima la base dei pazienti attivi, poi i loro ricoveri filtrati per anno", + "ctes":[ + {"name":"base_pazienti","purpose":"pazienti attivi","rationale":"insieme di partenza", + "depends_on":[],"tables":[{"name":"dim_patient"}],"keys":["cod_paz"], + "filters":[{"column":"dim_patient.flag_attivo","op":"IS","value":"TRUE","rationale":"solo attivi"}], + "output_columns":["cod_paz"]}, + {"name":"ricoveri_2023","purpose":"ricoveri dei pazienti nel 2023","rationale":"restringe al 2023", + "depends_on":["base_pazienti"],"tables":[{"name":"fact_ricoveri"}],"keys":["cod_paz"], + "filters":[{"column":"dim_time.year","op":"=","value":"2023","rationale":"finestra temporale"}], + "output_columns":["cod_paz","data_ricovero"]} + ]} + ``` 3. For each CTE (in plan order): write `sessions//ctes/.sql` (ONLY the `WITH ... AS (...)` block, NO trailing SELECT), test with `tht cte test --session - `, present the result in `reviewer_confirm kind:"cte_result"`. The next - CTE is testable ONLY after the previous one is approved (CLI exit 5 if out of - order). Copy table/column names EXACTLY from the schema context; use values - verified with `tht search`. + ` (with an **ok** outcome), then present it with + `reviewer_confirm kind:"cte_result"`. Pass ONLY the thin v2 data + `artifact:{kind:"cte_result", data:{schema_version:2, purpose?, rationale?, note?}}` — + NEVER paste SQL, columns or preview rows as text: the gate reads them + deterministically from `tht cte info` (the persisted `.sql` + the last test + record) and builds the full artifact the reviewer approves. The next CTE is testable + ONLY after the previous one is approved (CLI exit 5 if out of order). Copy + table/column names EXACTLY from the schema context; use values verified with + `tht search`. 4. After the last CTE is approved, close with `reviewer_confirm kind:"phase"`. ## Phase 7 — Final SQL