fix: harden phase summary open questions
This commit is contained in:
@@ -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`
|
||||
|
||||
|
||||
@@ -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(
|
||||
<ArtifactView
|
||||
artifact={{
|
||||
kind: "phase",
|
||||
data: {
|
||||
schema_version: 2,
|
||||
phase: { id: "F6", num: 6, name: "Piano CTE" },
|
||||
summary: "Piano completato.",
|
||||
open_questions: [
|
||||
{
|
||||
label: "pazienti_finale restituisce 0 righe",
|
||||
question: "Verificare se la condizione temporale è troppo restrittiva",
|
||||
},
|
||||
],
|
||||
},
|
||||
}}
|
||||
/>,
|
||||
);
|
||||
|
||||
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(
|
||||
<ArtifactView
|
||||
|
||||
@@ -81,6 +81,22 @@ test("renders open_questions as a bullet list", () => {
|
||||
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(<PhaseSummaryViewer phase={malformed} />);
|
||||
|
||||
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,
|
||||
|
||||
@@ -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 (
|
||||
<div className="flex flex-col gap-4">
|
||||
<h3 className="thot-label">
|
||||
@@ -101,11 +114,11 @@ export function PhaseSummaryViewer({ phase }: { phase: PhaseSummaryV2 }) {
|
||||
</div>
|
||||
)}
|
||||
|
||||
{phase.open_questions && phase.open_questions.length > 0 && (
|
||||
{openQuestions.length > 0 && (
|
||||
<div>
|
||||
<h4 className="thot-label mb-1">Open questions</h4>
|
||||
<ul className="list-disc space-y-1 pl-5 text-sm text-foreground/90 marker:text-muted-foreground">
|
||||
{phase.open_questions.map((q, i) => (
|
||||
{openQuestions.map((q, i) => (
|
||||
<li key={i}>{q}</li>
|
||||
))}
|
||||
</ul>
|
||||
|
||||
@@ -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<string, unknown>;
|
||||
for (const field of ["question", "label"]) {
|
||||
const text = record[field];
|
||||
if (typeof text === "string" && text.trim()) return text.trim();
|
||||
}
|
||||
return null;
|
||||
}
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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 };
|
||||
}
|
||||
|
||||
|
||||
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user