From 3e072fe6524d5d69f1ebb5dc79e16a2fb31d86dd Mon Sep 17 00:00:00 2001 From: mptyl Date: Mon, 20 Jul 2026 01:28:18 +0200 Subject: [PATCH] fix: realign workflow advance semantics and phase labels with workflow.yaml MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Audit findings 2.1 + 2.2 (high). 2.1 WorkflowBar's static phase list was fiction from F2 on (F2 "Schema linking" vs memoria, F4 "SQL plan" vs schema_linking, …): every live session showed the wrong phase name. Both maps now mirror harness/workflow.yaml (F1 chiarimento … F8 datamart). 2.2 forceAdvance (6ee5bda) let reviewer_decide advance:true bypass the phase gate on ANY phase, contradicting SKILL.md's "auto-advance only empty F2 / skipped F6". reviewer_decide is back on advanceIfReady (exit-6 no-op) and tells the model to close via reviewer_confirm; forceAdvance stays only where selection IS the approval by design: reviewer_schema_linking (F4) and the F8 promotion close path. SKILL.md now names the three self-closing gates (F3 rewrite_question, F4 schema-linking advance:true, F8 memory_promote) so gate and skill state one contract. Co-Authored-By: Claude Fable 5 --- frontend/src/shell/WorkflowBar.tsx | 33 +++++++++++++----------- harness/.pi/extensions/tht-gate.js | 17 ++++++++---- harness/.pi/skills/tht-sessione/SKILL.md | 15 ++++++----- 3 files changed, 39 insertions(+), 26 deletions(-) diff --git a/frontend/src/shell/WorkflowBar.tsx b/frontend/src/shell/WorkflowBar.tsx index 72206e7c..0e0d8341 100644 --- a/frontend/src/shell/WorkflowBar.tsx +++ b/frontend/src/shell/WorkflowBar.tsx @@ -2,15 +2,18 @@ import { useSessionStore } from "../store/sessionStore"; import { ElapsedTimer } from "./ElapsedTimer"; +// Mirror of harness/workflow.yaml phase ids/names (F1 chiarimento … F8 datamart). +// Keep the two lists in lockstep: the harness is the single source of phase truth, +// and a drifted label here mislabels every live session from that phase on. const PHASES = [ - { id: "F1", name: "Disambiguation" }, - { id: "F2", name: "Schema linking" }, - { id: "F3", name: "Exploration" }, - { id: "F4", name: "SQL plan" }, - { id: "F5", name: "SQL generation" }, - { id: "F6", name: "Validation" }, - { id: "F7", name: "Review" }, - { id: "F8", name: "Results" }, + { id: "F1", name: "Clarification" }, + { id: "F2", name: "Memory" }, + { id: "F3", name: "Rewrite" }, + { id: "F4", name: "Schema linking" }, + { id: "F5", name: "Plan" }, + { id: "F6", name: "CTE build" }, + { id: "F7", name: "Final SQL" }, + { id: "F8", name: "Datamart" }, ]; // Synthetic, human-readable title for the current phase, shown under the dots. @@ -19,13 +22,13 @@ const PHASES = [ const PHASE_LANG: "en" | "it" = "en"; const PHASE_TITLES: Record = { F1: { en: "Clarifying the question", it: "Chiarimento della domanda" }, - F2: { en: "Linking the schema", it: "Collegamento dello schema" }, - F3: { en: "Exploring the data", it: "Esplorazione dei dati" }, - F4: { en: "Planning the SQL", it: "Pianificazione della query" }, - F5: { en: "Generating the SQL", it: "Generazione della query" }, - F6: { en: "Validating the results", it: "Validazione dei risultati" }, - F7: { en: "Reviewing with you", it: "Revisione con te" }, - F8: { en: "Presenting results", it: "Presentazione dei risultati" }, + F2: { en: "Recalling relevant memory", it: "Recupero delle memorie utili" }, + F3: { en: "Rewriting the question", it: "Riscrittura della domanda" }, + F4: { en: "Linking the schema", it: "Collegamento dello schema" }, + F5: { en: "Planning the SQL", it: "Pianificazione della query" }, + F6: { en: "Building the CTEs", it: "Costruzione delle CTE" }, + F7: { en: "Finalizing the SQL", it: "Finalizzazione della query" }, + F8: { en: "Building the datamart", it: "Costruzione del datamart" }, }; type DotState = "done" | "running" | "error" | "pending"; diff --git a/harness/.pi/extensions/tht-gate.js b/harness/.pi/extensions/tht-gate.js index b07044ad..ea125d06 100644 --- a/harness/.pi/extensions/tht-gate.js +++ b/harness/.pi/extensions/tht-gate.js @@ -367,9 +367,10 @@ function advanceIfReady(ctx, session) { } } -// Unconditional phase advance — the reviewer already approved via the widget interaction -// (reviewer_decide selection IS the human confirmation). Used when reviewer_decide persists -// substantive decisions with advance:true, so no separate reviewer_confirm gate is needed. +// Unconditional phase advance — the reviewer already approved via the widget interaction. +// ONLY for gates whose selection IS the phase approval by design: reviewer_schema_linking +// (F4 curation) and the F8 close path. Generic reviewer_decide must use advanceIfReady — +// forcing there bypasses the reviewer_confirm phase gate and contradicts SKILL.md. function forceAdvance(ctx, session) { try { tht(ctx, ["phase", "advance", "--session", session]); @@ -891,9 +892,15 @@ export default function (pi) { const msg = "Nessuna decisione registrata (il reviewer non ha selezionato opzioni di merito)."; return textResult(adv.advanced ? msg + " Fase avanzata automaticamente." : msg); } - const adv = advance ? forceAdvance(ctx, session) : { advanced: false }; + // SKILL.md contract: advance:true means auto-advance-IF-ELIGIBLE (empty F2 / + // skipped F6), never a bypass of the phase gate once substantive decisions + // exist. The only gates that close their phase on selection are + // reviewer_schema_linking (F4) and reviewer_memory_promote (F8). + const adv = advance ? advanceIfReady(ctx, session) : { advanced: false }; const parts = [`Registrate ${toAdd.length} decisioni: ${toAdd.map((d) => d.type).join(", ")}.`]; - if (adv.advanced) parts.push("Fase avanzata automaticamente — nessun gate aggiuntivo necessario."); + parts.push(adv.advanced + ? "Fase avanzata automaticamente." + : 'La fase resta aperta: chiudila con reviewer_confirm kind:"phase" quando pronta.'); return textResult(parts.join(" ")); } catch (fatal) { const msg = (fatal.stderr || fatal.message || String(fatal)).toString().trim(); diff --git a/harness/.pi/skills/tht-sessione/SKILL.md b/harness/.pi/skills/tht-sessione/SKILL.md index d32079ad..77431a13 100644 --- a/harness/.pi/skills/tht-sessione/SKILL.md +++ b/harness/.pi/skills/tht-sessione/SKILL.md @@ -29,15 +29,17 @@ A phase advances ONLY when a `phase_approved:phase:N` decision is recorded for t phase — written by `reviewer_confirm kind:"phase"`. (F7 is two-step: `kind:"sql"` records `sql_approved`, then `kind:"phase"` advances.) A `reviewer_decide`/ `reviewer_select` choice records its OWN decision but does NOT advance the phase. `advance:true` -auto-advances only F2 (empty memory) and F6 (skipped/empty) — never a phase that recorded -substantive decisions. +on `reviewer_decide` auto-advances only F2 (empty memory) and F6 (skipped/empty) — never a +phase that recorded substantive decisions. Exactly three gates close their phase themselves, +because there the human interaction IS the phase approval: `rewrite_question` (F3), +`reviewer_schema_linking` with `advance:true` (F4), and `reviewer_memory_promote` (F8). | Phase | Artifact out | Advance / close by | |-------|--------------|--------------------| | F1 chiarimento | — | `reviewer_confirm kind:"phase"` | | F2 memoria | — | `advance:true` only if nothing recorded; else `reviewer_confirm kind:"phase"` | | 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. | +| F4 schema_linking | `schema_linking.json` | `reviewer_schema_linking` with `advance:true` closes the phase itself (the curation IS the approval; `reviewer_confirm kind:"phase"` only as fallback if it reports an error). 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"` | | F7 sql_finale | `sql_final.sql` | `kind:"sql"` records `sql_approved`, then `reviewer_confirm kind:"phase"` | @@ -56,9 +58,10 @@ substantive decisions. `kind:"phase"` to advance), the deliberate "this phase is done" gate. The `advance:true` flag on `reviewer_decide` is a shortcut that auto-advances ONLY F2 when the memory phase recorded nothing and F6 when it is skipped/empty; everywhere - else it is a silent no-op, so never rely on it to advance. Do NOT add a `reviewer_confirm` - that merely echoes a decision already recorded by a choice — the phase gate is a separate, - deliberate step, not an echo of a decision. + else it is a silent no-op, so never rely on it to advance. The self-closing gates are the + three listed above (F3 `rewrite_question`, F4 `reviewer_schema_linking` `advance:true`, + F8 `reviewer_memory_promote`) — after one of those, do NOT add a `reviewer_confirm` that + merely echoes it; the phase is already closed. 3. **Single pick vs multi-answer.** For a single-pick clarification or decision, use `reviewer_select` and attach a `decision` payload (`{type, subject, detail?, rationale?}`) to each concrete option: picking it persists that decision directly —