From e228c6f2fd18b8833806b5e8ee0b16e8b79d1a90 Mon Sep 17 00:00:00 2001 From: User Date: Tue, 14 Jul 2026 15:09:50 +0200 Subject: [PATCH] fix: harden phase summary open questions --- .../2026-07-14-workflow-ui-regressions.md | 18 ++++++------- frontend/src/viewers/ArtifactView.test.tsx | 25 +++++++++++++++++++ .../src/viewers/PhaseSummaryViewer.test.tsx | 16 ++++++++++++ frontend/src/viewers/PhaseSummaryViewer.tsx | 19 +++++++++++--- frontend/src/viewers/artifactV2.ts | 15 +++++++++++ .../gate/__tests__/artifact_contracts.test.js | 16 ++++++++++++ .../.pi/extensions/gate/artifact-contracts.js | 11 ++++++++ harness/.pi/skills/tht-sessione/SKILL.md | 2 ++ 8 files changed, 110 insertions(+), 12 deletions(-) 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 0ec4b0c2..dc395ebc 100644 --- a/docs/superpowers/plans/2026-07-14-workflow-ui-regressions.md +++ b/docs/superpowers/plans/2026-07-14-workflow-ui-regressions.md @@ -202,7 +202,7 @@ Expected: PASS. **Files:** - Modify: `harness/.pi/extensions/gate/artifact-contracts.js` -- Modify: `harness/.pi/extensions/gate/__tests__/artifact-contracts.test.js` +- Modify: `harness/.pi/extensions/gate/__tests__/artifact_contracts.test.js` - Modify: `harness/.pi/skills/tht-sessione/SKILL.md` - Modify: `frontend/src/viewers/artifactV2.ts` - Modify: `frontend/src/viewers/PhaseSummaryViewer.tsx` @@ -214,7 +214,7 @@ Expected: PASS. - Frontend legacy normalization: string entries pass through; objects prefer `question`, then `label`; all other values become safe text or are omitted. -- [ ] **Step 1: Write failing gate validation test** +- [x] **Step 1: Write failing gate validation test** Pass the exact observed payload shape: @@ -224,34 +224,34 @@ open_questions: [{ label: "pazienti_finale restituisce 0 righe", question: "Veri Expect `ok:false` and an error naming `open_questions[0]`. -- [ ] **Step 2: Run gate test and verify RED** +- [x] **Step 2: Run gate test and verify RED** -Run: `cd harness && node --test .pi/extensions/gate/__tests__/artifact-contracts.test.js` +Run: `cd harness && node --test .pi/extensions/gate/__tests__/artifact_contracts.test.js` Expected: payload is currently accepted. -- [ ] **Step 3: Implement strict gate validation** +- [x] **Step 3: Implement strict gate validation** Reject a non-array `open_questions` value and every non-string entry with an indexed error. Document the exact array-of-strings shape in the session skill. -- [ ] **Step 4: Write failing frontend resilience test** +- [x] **Step 4: Write failing frontend resilience test** Render a phase-summary artifact containing the observed object and assert the screen displays `Verificare i filtri` and does not show the ErrorBoundary fallback. -- [ ] **Step 5: Run frontend test and verify RED** +- [x] **Step 5: Run frontend test and verify RED** Run: `cd frontend && npx vitest run src/viewers/PhaseSummaryViewer.test.tsx src/viewers/ArtifactView.test.tsx` Expected: React reports an object child/rendering failure. -- [ ] **Step 6: Implement safe legacy normalization** +- [x] **Step 6: Implement safe legacy normalization** Add a small `phaseOpenQuestionText(value: unknown): string | null` helper and map/filter entries before rendering. Preserve the strict public TypeScript contract for new v2 payloads. -- [ ] **Step 7: Run targeted tests and typechecks** +- [x] **Step 7: Run targeted tests and typechecks** Run: `cd harness && npm test` diff --git a/frontend/src/viewers/ArtifactView.test.tsx b/frontend/src/viewers/ArtifactView.test.tsx index f86dd405..0f34be7a 100644 --- a/frontend/src/viewers/ArtifactView.test.tsx +++ b/frontend/src/viewers/ArtifactView.test.tsx @@ -129,6 +129,31 @@ test("phase v2 dispatches to PhaseSummaryViewer", () => { expect(screen.getByText("Riassunto della fase.")).toBeInTheDocument(); }); +test("phase v2 tolerates the malformed open question from the latest session", () => { + render( + , + ); + + expect( + screen.getByText("Verificare se la condizione temporale è troppo restrittiva"), + ).toBeInTheDocument(); +}); + test("phase v1 (no schema_version) still renders via legacy markdown/structured path", () => { render( { expect(screen.getByText("Serve confermare la finestra temporale?")).toBeInTheDocument(); }); +test("renders a legacy structured open question safely", () => { + const malformed = { + ...phase, + open_questions: [ + { + label: "pazienti_finale restituisce 0 righe", + question: "Verificare se i filtri sono troppo restrittivi", + }, + ], + } as unknown as PhaseSummaryV2; + + render(); + + expect(screen.getByText("Verificare se i filtri sono troppo restrittivi")).toBeInTheDocument(); +}); + test("renders a section with title only (no items key) without crashing", () => { const proseOnly: PhaseSummaryV2 = { schema_version: 2, diff --git a/frontend/src/viewers/PhaseSummaryViewer.tsx b/frontend/src/viewers/PhaseSummaryViewer.tsx index 6badb2cd..46029132 100644 --- a/frontend/src/viewers/PhaseSummaryViewer.tsx +++ b/frontend/src/viewers/PhaseSummaryViewer.tsx @@ -1,7 +1,14 @@ import { Badge } from "../components/ui/badge"; import { MarkdownView } from "./MarkdownView"; import { statusBadgeClass } from "./statusBadge"; -import type { PhaseCheck, PhaseSection, PhaseSectionItem, PhaseSummaryV2, PhaseTable } from "./artifactV2"; +import { + phaseOpenQuestionText, + type PhaseCheck, + type PhaseSection, + type PhaseSectionItem, + type PhaseSummaryV2, + type PhaseTable, +} from "./artifactV2"; const CODE_CHIP = "rounded-md bg-muted px-1.5 py-0.5 font-mono text-xs text-foreground/90"; @@ -69,6 +76,12 @@ function TableRecap({ table }: { table: PhaseTable }) { } export function PhaseSummaryViewer({ phase }: { phase: PhaseSummaryV2 }) { + const openQuestions = Array.isArray(phase.open_questions) + ? (phase.open_questions as unknown[]) + .map(phaseOpenQuestionText) + .filter((question): question is string => question !== null) + : []; + return (

@@ -101,11 +114,11 @@ export function PhaseSummaryViewer({ phase }: { phase: PhaseSummaryV2 }) {

)} - {phase.open_questions && phase.open_questions.length > 0 && ( + {openQuestions.length > 0 && (

Open questions

    - {phase.open_questions.map((q, i) => ( + {openQuestions.map((q, i) => (
  • {q}
    • ))}
diff --git a/frontend/src/viewers/artifactV2.ts b/frontend/src/viewers/artifactV2.ts index caca0e09..6f6a62d0 100644 --- a/frontend/src/viewers/artifactV2.ts +++ b/frontend/src/viewers/artifactV2.ts @@ -119,3 +119,18 @@ export interface PhaseSummaryV2 { tables?: PhaseTable[]; open_questions?: string[]; } + +/** Defensive compatibility for malformed v2 artifacts persisted before the gate + * enforced `open_questions: string[]`. New payloads stay strictly typed as strings. */ +export function phaseOpenQuestionText(value: unknown): string | null { + if (typeof value === "string") return value.trim() || null; + if (typeof value === "number" || typeof value === "boolean") return String(value); + if (!value || typeof value !== "object" || Array.isArray(value)) return null; + + const record = value as Record; + for (const field of ["question", "label"]) { + const text = record[field]; + if (typeof text === "string" && text.trim()) return text.trim(); + } + return null; +} diff --git a/harness/.pi/extensions/gate/__tests__/artifact_contracts.test.js b/harness/.pi/extensions/gate/__tests__/artifact_contracts.test.js index 7c023f82..6b604f5b 100644 --- a/harness/.pi/extensions/gate/__tests__/artifact_contracts.test.js +++ b/harness/.pi/extensions/gate/__tests__/artifact_contracts.test.js @@ -151,6 +151,22 @@ test("validatePhaseSummaryV2: rejects a section missing title or items", () => { assert.ok(out.errors.some((e) => /sections\[0\].*title/.test(e))); }); +test("validatePhaseSummaryV2: rejects structured open_questions instead of crashing the UI", () => { + const out = validatePhaseSummaryV2({ + schema_version: 2, + summary: "s", + open_questions: [ + { + label: "pazienti_finale restituisce 0 righe", + question: "Verificare se i filtri sono troppo restrittivi", + }, + ], + }); + + assert.equal(out.ok, false); + assert.ok(out.errors.some((error) => /open_questions\[0\]/.test(error))); +}); + 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/artifact-contracts.js b/harness/.pi/extensions/gate/artifact-contracts.js index 116b2b93..1ca6e89a 100644 --- a/harness/.pi/extensions/gate/artifact-contracts.js +++ b/harness/.pi/extensions/gate/artifact-contracts.js @@ -115,6 +115,17 @@ function validatePhaseSummaryV2(data) { if (data.tables !== undefined && !isArray(data.tables)) { errors.push("tables deve essere un array se presente."); } + if (data.open_questions !== undefined) { + if (!isArray(data.open_questions)) { + errors.push("open_questions deve essere un array di stringhe se presente."); + } else { + data.open_questions.forEach((question, i) => { + if (typeof question !== "string") { + errors.push(`open_questions[${i}] deve essere una stringa.`); + } + }); + } + } return { ok: errors.length === 0, errors }; } diff --git a/harness/.pi/skills/tht-sessione/SKILL.md b/harness/.pi/skills/tht-sessione/SKILL.md index 454a5780..1bc9625b 100644 --- a/harness/.pi/skills/tht-sessione/SKILL.md +++ b/harness/.pi/skills/tht-sessione/SKILL.md @@ -100,6 +100,8 @@ substantive decisions. "columns":[{"name":"cod_paz","value_filter":""}]}], "open_questions":[]} ``` + `open_questions` MUST be an array of plain strings (`string[]`). Never put objects + such as `{label, question}` in it; express each open question as one complete string. 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.