docs(plan): reviewer gate UX fixes implementation plan; align spec Part 4 to ctx.ui.notify
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Claude Opus 4.8
@@ -0,0 +1,575 @@
|
||||
# Reviewer gate UX fixes — Implementation Plan
|
||||
|
||||
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
|
||||
|
||||
**Goal:** Fix four reviewer-gate UX defects — the multiselect lacks Back/Exit/Other, the phase gate renders no forward button (and "Other" silently approves), "Altro — specifica" never captures text, and the empty memory phase shows a pointless empty checklist.
|
||||
|
||||
**Architecture:** Four surgical changes across the frontend widgets and the Pi gate. Keep logic in pure, unit-testable functions (the repo's pattern: `builders.js` and `resolveSelectOutcome` are unit-tested; the tool `execute()` glue that shells `tht` is L2/live). Add a pure `resolveConfirmOutcome` (twin of `resolveSelectOutcome`) and a pure `shouldSkipEmptyDecide` predicate so the new gate behavior is testable in node.
|
||||
|
||||
**Tech Stack:** React 18 + Vite + vitest + @testing-library/react (frontend); Node.js gate extension tested with `node:test` + a fake Pi runtime (harness); `tht` Python CLI unchanged.
|
||||
|
||||
## Global Constraints
|
||||
|
||||
- **Design source of truth:** [docs/superpowers/specs/2026-07-02-reviewer-gate-ux-fixes-design.md](../specs/2026-07-02-reviewer-gate-ux-fixes-design.md).
|
||||
- **No new auto-advance phases.** `phase.py` `_AUTO_ADVANCE_PHASES` stays `{2,6}`; do NOT touch `phase.py` or `workflow.yaml`. F1/F3/F5/F7 keep their `reviewer_confirm kind:"phase"` gate.
|
||||
- **Reserved wire contract:** after this work the only reserved responses on the wire are `control:"back"`, `control:"exit"`, and `control:"freetext"` (with `text`). The bare `control:"other"` must no longer be emitted by any widget.
|
||||
- **UI chrome (React button labels) is English** (`"Go back"`, `"Exit"`, `"Other — specify"`). **Gate notifications / `textResult` strings are Italian** (the workspace `psd` language), matching existing gate strings (e.g. `"Uso: /torna …"`, the `reLoop` notice). The reviewer-facing forward button label is **"Conferma e prosegui"** (decided).
|
||||
- **Match each file's existing indentation:** `tht-gate.js` uses TABS; `gate/builders.js` and the frontend use 2-space. No reformatting of untouched lines.
|
||||
- **Typecheck the frontend** after any `.tsx` change: `cd frontend && npx tsc -b`. No ESLint on any layer.
|
||||
- **Test commands:** frontend `cd frontend && npx vitest run <file>`; gate `node --test harness/.pi/extensions/gate/__tests__/<file>`.
|
||||
|
||||
---
|
||||
|
||||
## File structure
|
||||
|
||||
**Frontend (`frontend/src/widgets/`)**
|
||||
- `MultiselectWidget.tsx` — render `<ReservedControls>` (Task 1).
|
||||
- `ReservedControls.tsx` — "Other — specify" reveals a textarea and emits `control:"freetext"` + text; `onControl` gains an optional `text` arg (Task 2).
|
||||
- `SelectWidget.tsx`, `ArtifactGateWidget.tsx` — update the `onControl` call site to forward `text` (Task 2).
|
||||
- `ReservedControls.test.tsx` (new), `MultiselectWidget.test.tsx` (extend) — tests.
|
||||
|
||||
**Harness gate (`harness/.pi/extensions/`)**
|
||||
- `gate/builders.js` — `buildArtifactGate` derives `options` from `action.kind` (Task 3).
|
||||
- `tht-gate.js` — add exported pure `resolveConfirmOutcome` + rewrite `reviewer_confirm` guard to require an explicit approve and treat freetext as actionable (Task 4); add exported pure `shouldSkipEmptyDecide` + short-circuit the empty memory decide (Task 5).
|
||||
- `gate/__tests__/builders.test.js` (extend), `gate/__tests__/gate_confirm_outcome.test.js` (new), `gate/__tests__/gate_decide_empty.test.js` (new) — tests.
|
||||
|
||||
**Skill (`harness/.pi/skills/tht-sessione/`)**
|
||||
- `SKILL.md` — document the empty-memory short-circuit in Phase 2 (Task 6).
|
||||
|
||||
---
|
||||
|
||||
## Task 1: Multiselect gets Back/Exit/Other controls
|
||||
|
||||
**Files:**
|
||||
- Modify: `frontend/src/widgets/MultiselectWidget.tsx`
|
||||
- Test: `frontend/src/widgets/MultiselectWidget.test.tsx`
|
||||
|
||||
**Interfaces:**
|
||||
- Consumes: `ReservedControls` from `./ReservedControls` (existing; signature `{ reserved?: string[]; onControl: (c: string) => void }` at this point — Task 2 widens it).
|
||||
- Produces: nothing new; the multiselect now emits `{ id, control }` for reserved controls, matching `SelectWidget`.
|
||||
|
||||
- [ ] **Step 1: Write the failing test** — append to `frontend/src/widgets/MultiselectWidget.test.tsx`:
|
||||
|
||||
```tsx
|
||||
test("renders reserved controls and emits control on click", async () => {
|
||||
const onRespond = vi.fn();
|
||||
render(
|
||||
<MultiselectWidget
|
||||
descriptor={{
|
||||
id: "u1",
|
||||
widget: "multiselect",
|
||||
options: [{ id: "a", label: "Alpha" }],
|
||||
reserved: ["back", "exit", "other"],
|
||||
}}
|
||||
onRespond={onRespond}
|
||||
/>
|
||||
);
|
||||
await userEvent.click(screen.getByRole("button", { name: /go back/i }));
|
||||
expect(onRespond).toHaveBeenCalledWith({ id: "u1", control: "back" });
|
||||
await userEvent.click(screen.getByRole("button", { name: /^exit$/i }));
|
||||
expect(onRespond).toHaveBeenCalledWith({ id: "u1", control: "exit" });
|
||||
});
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Run the test to verify it fails**
|
||||
|
||||
Run: `cd frontend && npx vitest run src/widgets/MultiselectWidget.test.tsx -t "renders reserved controls"`
|
||||
Expected: FAIL — no button named "Go back" (ReservedControls not rendered).
|
||||
|
||||
- [ ] **Step 3: Implement — render `<ReservedControls>` in `MultiselectWidget.tsx`.** Add the import at the top (after line 2):
|
||||
|
||||
```tsx
|
||||
import { ReservedControls } from "./ReservedControls";
|
||||
```
|
||||
|
||||
Then insert the control block immediately AFTER the closing `</button>` of the Confirm button (currently line 56), before the closing `</div>`:
|
||||
|
||||
```tsx
|
||||
<ReservedControls
|
||||
reserved={descriptor.reserved}
|
||||
onControl={(c) => onRespond({ id: descriptor.id, control: c })}
|
||||
/>
|
||||
```
|
||||
|
||||
- [ ] **Step 4: Run the test to verify it passes**
|
||||
|
||||
Run: `cd frontend && npx vitest run src/widgets/MultiselectWidget.test.tsx`
|
||||
Expected: PASS (all existing tests + the new one).
|
||||
|
||||
- [ ] **Step 5: Typecheck**
|
||||
|
||||
Run: `cd frontend && npx tsc -b`
|
||||
Expected: no errors.
|
||||
|
||||
- [ ] **Step 6: Commit**
|
||||
|
||||
```bash
|
||||
git add frontend/src/widgets/MultiselectWidget.tsx frontend/src/widgets/MultiselectWidget.test.tsx
|
||||
git commit -m "fix(frontend): render Back/Exit/Other controls on the multiselect widget"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Task 2: "Other — specify" opens a text field and emits `control:"freetext"`
|
||||
|
||||
**Files:**
|
||||
- Modify: `frontend/src/widgets/ReservedControls.tsx`
|
||||
- Modify: `frontend/src/widgets/SelectWidget.tsx` (line 16 call site)
|
||||
- Modify: `frontend/src/widgets/ArtifactGateWidget.tsx` (lines 54-57 call site)
|
||||
- Modify: `frontend/src/widgets/MultiselectWidget.tsx` (the call site added in Task 1)
|
||||
- Test: `frontend/src/widgets/ReservedControls.test.tsx` (new)
|
||||
|
||||
**Interfaces:**
|
||||
- Produces: `ReservedControls` `onControl` signature becomes `(control: string, text?: string) => void`. On "other", the component collects text and calls `onControl("freetext", text)` — it never emits the bare `"other"` control. `back`/`exit` fire immediately with no text.
|
||||
- Consumes (all three widgets): the call site becomes
|
||||
`onControl={(c, t) => onRespond({ id: descriptor.id, control: c, ...(t !== undefined ? { text: t } : {}) })}`.
|
||||
|
||||
- [ ] **Step 1: Write the failing test** — create `frontend/src/widgets/ReservedControls.test.tsx`:
|
||||
|
||||
```tsx
|
||||
import { render, screen } from "@testing-library/react";
|
||||
import userEvent from "@testing-library/user-event";
|
||||
import { ReservedControls } from "./ReservedControls";
|
||||
|
||||
test("back and exit fire immediately with no text", async () => {
|
||||
const onControl = vi.fn();
|
||||
render(<ReservedControls reserved={["back", "exit"]} onControl={onControl} />);
|
||||
await userEvent.click(screen.getByRole("button", { name: /go back/i }));
|
||||
expect(onControl).toHaveBeenCalledWith("back");
|
||||
await userEvent.click(screen.getByRole("button", { name: /^exit$/i }));
|
||||
expect(onControl).toHaveBeenCalledWith("exit");
|
||||
});
|
||||
|
||||
test("other reveals a textarea and emits freetext with the typed text", async () => {
|
||||
const onControl = vi.fn();
|
||||
render(<ReservedControls reserved={["other"]} onControl={onControl} />);
|
||||
await userEvent.click(screen.getByRole("button", { name: /other — specify/i }));
|
||||
// clicking Other does NOT emit a control yet — it reveals the input
|
||||
expect(onControl).not.toHaveBeenCalled();
|
||||
await userEvent.type(screen.getByRole("textbox"), "usa la tabella X");
|
||||
await userEvent.click(screen.getByRole("button", { name: /send/i }));
|
||||
expect(onControl).toHaveBeenCalledWith("freetext", "usa la tabella X");
|
||||
});
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Run the test to verify it fails**
|
||||
|
||||
Run: `cd frontend && npx vitest run src/widgets/ReservedControls.test.tsx`
|
||||
Expected: FAIL — clicking "Other — specify" currently calls `onControl("other")` immediately; no textbox appears.
|
||||
|
||||
- [ ] **Step 3: Implement — rewrite `frontend/src/widgets/ReservedControls.tsx`:**
|
||||
|
||||
```tsx
|
||||
import { useState } from "react";
|
||||
|
||||
const LABELS: Record<string, string> = { back: "Go back", exit: "Exit", other: "Other — specify" };
|
||||
|
||||
export function ReservedControls({
|
||||
reserved,
|
||||
onControl,
|
||||
}: {
|
||||
reserved?: string[];
|
||||
onControl: (c: string, text?: string) => void;
|
||||
}) {
|
||||
const [otherOpen, setOtherOpen] = useState(false);
|
||||
const [text, setText] = useState("");
|
||||
if (!reserved?.length) return null;
|
||||
return (
|
||||
<div className="flex flex-col gap-2 pt-2">
|
||||
<div className="flex gap-2">
|
||||
{reserved.map((c) => (
|
||||
<button
|
||||
key={c}
|
||||
className="text-sm border rounded px-2 py-1"
|
||||
onClick={() => (c === "other" ? setOtherOpen(true) : onControl(c))}
|
||||
>
|
||||
{LABELS[c] ?? c}
|
||||
</button>
|
||||
))}
|
||||
</div>
|
||||
{otherOpen && (
|
||||
<div className="flex flex-col gap-1">
|
||||
<textarea
|
||||
className="w-full border rounded p-2"
|
||||
value={text}
|
||||
onChange={(e) => setText(e.target.value)}
|
||||
/>
|
||||
<button className="text-sm border rounded px-2 py-1 self-start" onClick={() => onControl("freetext", text)}>
|
||||
Send
|
||||
</button>
|
||||
</div>
|
||||
)}
|
||||
</div>
|
||||
);
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 4: Update the three call sites** so the widgets forward the optional text.
|
||||
|
||||
`SelectWidget.tsx` line 16 — replace with:
|
||||
|
||||
```tsx
|
||||
<ReservedControls reserved={descriptor.reserved} onControl={(c, t) => onRespond({ id: descriptor.id, control: c, ...(t !== undefined ? { text: t } : {}) })} />
|
||||
```
|
||||
|
||||
`ArtifactGateWidget.tsx` lines 54-57 — replace the `<ReservedControls .../>` block with:
|
||||
|
||||
```tsx
|
||||
<ReservedControls
|
||||
reserved={descriptor.reserved}
|
||||
onControl={(c, t) => onRespond({ id: descriptor.id, control: c, ...(t !== undefined ? { text: t } : {}) })}
|
||||
/>
|
||||
```
|
||||
|
||||
`MultiselectWidget.tsx` — the block added in Task 1 — replace with:
|
||||
|
||||
```tsx
|
||||
<ReservedControls
|
||||
reserved={descriptor.reserved}
|
||||
onControl={(c, t) => onRespond({ id: descriptor.id, control: c, ...(t !== undefined ? { text: t } : {}) })}
|
||||
/>
|
||||
```
|
||||
|
||||
- [ ] **Step 5: Run tests + typecheck**
|
||||
|
||||
Run: `cd frontend && npx vitest run src/widgets/ && npx tsc -b`
|
||||
Expected: PASS — including the existing `ArtifactGateWidget.test.tsx` "reserved control responds with control field" test (back still emits `{ id, control: "back" }`, no `text` key).
|
||||
|
||||
- [ ] **Step 6: Commit**
|
||||
|
||||
```bash
|
||||
git add frontend/src/widgets/ReservedControls.tsx frontend/src/widgets/ReservedControls.test.tsx frontend/src/widgets/SelectWidget.tsx frontend/src/widgets/ArtifactGateWidget.tsx frontend/src/widgets/MultiselectWidget.tsx
|
||||
git commit -m "fix(frontend): 'Other — specify' opens a text field and emits control:freetext"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Task 3: `buildArtifactGate` derives merito `options` from `action.kind`
|
||||
|
||||
**Files:**
|
||||
- Modify: `harness/.pi/extensions/gate/builders.js` (`buildArtifactGate`, lines 102-126)
|
||||
- Test: `harness/.pi/extensions/gate/__tests__/builders.test.js` (extend)
|
||||
|
||||
**Interfaces:**
|
||||
- Produces: `buildArtifactGate({ id, phase, title, artifact, action })` now returns a descriptor with an `options` array:
|
||||
- `action.kind === "approve_reject"` → `[{ id: "approve", label: "Conferma e prosegui", recommended: true }, { id: "reject", label: "Rifiuta" }]`
|
||||
- `action.kind === "confirm"` → `[{ id: "approve", label: "Conferma e prosegui", recommended: true }]`
|
||||
- `action.kind === "view_only"` → `[]`
|
||||
- Consumed by: `ArtifactGateWidget` (`descriptor.options?.map`) and, indirectly, by `resolveConfirmOutcome` in Task 4 (which reads `choices:["approve"|"reject"]`).
|
||||
|
||||
- [ ] **Step 1: Write the failing test** — append to `harness/.pi/extensions/gate/__tests__/builders.test.js`:
|
||||
|
||||
```js
|
||||
test("buildArtifactGate derives approve/reject options from action.kind", () => {
|
||||
const w = buildArtifactGate({
|
||||
id: "u1",
|
||||
phase: "F1",
|
||||
title: "Chiudi fase",
|
||||
artifact: { kind: "phase", data: {} },
|
||||
action: { kind: "approve_reject" },
|
||||
});
|
||||
assert.deepEqual(w.options, [
|
||||
{ id: "approve", label: "Conferma e prosegui", recommended: true },
|
||||
{ id: "reject", label: "Rifiuta" },
|
||||
]);
|
||||
});
|
||||
|
||||
test("buildArtifactGate: confirm -> single approve; view_only -> no options", () => {
|
||||
const confirm = buildArtifactGate({
|
||||
id: "u1", phase: "F1", title: "t", artifact: { kind: "phase", data: {} }, action: { kind: "confirm" },
|
||||
});
|
||||
assert.deepEqual(confirm.options, [{ id: "approve", label: "Conferma e prosegui", recommended: true }]);
|
||||
const viewOnly = buildArtifactGate({
|
||||
id: "u1", phase: "F1", title: "t", artifact: { kind: "phase", data: {} }, action: { kind: "view_only" },
|
||||
});
|
||||
assert.deepEqual(viewOnly.options, []);
|
||||
});
|
||||
```
|
||||
|
||||
(If `buildArtifactGate` is not already imported at the top of `builders.test.js`, add it to the existing `require("../builders.js")` destructure.)
|
||||
|
||||
- [ ] **Step 2: Run the test to verify it fails**
|
||||
|
||||
Run: `node --test harness/.pi/extensions/gate/__tests__/builders.test.js`
|
||||
Expected: FAIL — `w.options` is `undefined`.
|
||||
|
||||
- [ ] **Step 3: Implement — add option derivation in `buildArtifactGate`.** Before the `return { ... }` (line 115), add:
|
||||
|
||||
```js
|
||||
const ACTION_OPTIONS = {
|
||||
approve_reject: [
|
||||
{ id: "approve", label: "Conferma e prosegui", recommended: true },
|
||||
{ id: "reject", label: "Rifiuta" },
|
||||
],
|
||||
confirm: [{ id: "approve", label: "Conferma e prosegui", recommended: true }],
|
||||
view_only: [],
|
||||
};
|
||||
```
|
||||
|
||||
Then add `options: ACTION_OPTIONS[action.kind],` to the returned object (e.g. right after the `action,` line):
|
||||
|
||||
```js
|
||||
action,
|
||||
options: ACTION_OPTIONS[action.kind],
|
||||
reserved: RESERVED,
|
||||
```
|
||||
|
||||
- [ ] **Step 4: Run the test to verify it passes**
|
||||
|
||||
Run: `node --test harness/.pi/extensions/gate/__tests__/builders.test.js`
|
||||
Expected: PASS (all existing builder tests + the two new ones).
|
||||
|
||||
- [ ] **Step 5: Commit**
|
||||
|
||||
```bash
|
||||
git add harness/.pi/extensions/gate/builders.js harness/.pi/extensions/gate/__tests__/builders.test.js
|
||||
git commit -m "fix(gate): buildArtifactGate emits approve/reject options so the gate renders a forward button"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Task 4: Gate requires explicit approve; free-text is actionable (not accidental approve)
|
||||
|
||||
**Files:**
|
||||
- Modify: `harness/.pi/extensions/tht-gate.js` (add `resolveConfirmOutcome`; rewrite the `reviewer_confirm` `execute` guard, lines 585-594)
|
||||
- Test: `harness/.pi/extensions/gate/__tests__/gate_confirm_outcome.test.js` (new)
|
||||
|
||||
**Interfaces:**
|
||||
- Produces (exported pure fn): `resolveConfirmOutcome(resp)` →
|
||||
- `{ kind: "freetext", text }` when `resp.control === "freetext"`
|
||||
- `{ kind: "back" }` / `{ kind: "exit" }` for those controls
|
||||
- `{ kind: "approve" }` when `selectedChoice(resp) === "approve"`
|
||||
- `{ kind: "reject" }` when `selectedChoice(resp) === "reject"`
|
||||
- `{ kind: "unknown", choice }` otherwise
|
||||
- Consumes: `selectedChoice` (already exported in `tht-gate.js`).
|
||||
- Behavior change: `reviewer_confirm` advances ONLY on `kind: "approve"`; `unknown` re-presents the widget (never auto-approves); `freetext` returns actionable feedback, distinct from `reject`.
|
||||
|
||||
- [ ] **Step 1: Write the failing test** — create `harness/.pi/extensions/gate/__tests__/gate_confirm_outcome.test.js`:
|
||||
|
||||
```js
|
||||
const test = require("node:test");
|
||||
const assert = require("node:assert");
|
||||
const { resolveConfirmOutcome } = require("../../tht-gate.js");
|
||||
|
||||
test("explicit approve choice resolves to approve", () => {
|
||||
assert.deepEqual(resolveConfirmOutcome({ id: "u1", choices: ["approve"] }), { kind: "approve" });
|
||||
});
|
||||
|
||||
test("reject choice resolves to reject", () => {
|
||||
assert.deepEqual(resolveConfirmOutcome({ id: "u1", choices: ["reject"] }), { kind: "reject" });
|
||||
});
|
||||
|
||||
test("freetext control is actionable, carries text, and is NOT approve", () => {
|
||||
const out = resolveConfirmOutcome({ id: "u1", control: "freetext", text: "aggiungi la tabella X" });
|
||||
assert.equal(out.kind, "freetext");
|
||||
assert.equal(out.text, "aggiungi la tabella X");
|
||||
});
|
||||
|
||||
test("back/exit controls resolve to their kinds", () => {
|
||||
assert.equal(resolveConfirmOutcome({ control: "back" }).kind, "back");
|
||||
assert.equal(resolveConfirmOutcome({ control: "exit" }).kind, "exit");
|
||||
});
|
||||
|
||||
test("an unrecognized response never approves (kind: unknown)", () => {
|
||||
assert.equal(resolveConfirmOutcome({ id: "u1", control: "other" }).kind, "unknown");
|
||||
assert.equal(resolveConfirmOutcome({ id: "u1", choices: [] }).kind, "unknown");
|
||||
});
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Run the test to verify it fails**
|
||||
|
||||
Run: `node --test harness/.pi/extensions/gate/__tests__/gate_confirm_outcome.test.js`
|
||||
Expected: FAIL — `resolveConfirmOutcome` is not exported / not a function.
|
||||
|
||||
- [ ] **Step 3: Implement the pure function** in `tht-gate.js`, immediately after `resolveSelectOutcome` (ends at line 254). Use TAB indentation to match the file:
|
||||
|
||||
```js
|
||||
// Classifies a reviewer_confirm response. Advance happens ONLY on an explicit
|
||||
// "approve" choice; a bare/unknown response resolves to {kind:"unknown"} and is
|
||||
// re-presented (never an accidental approve). freetext is actionable feedback,
|
||||
// distinct from an explicit "reject".
|
||||
export function resolveConfirmOutcome(resp) {
|
||||
if (resp?.control === "freetext") return { kind: "freetext", text: resp.text };
|
||||
if (resp?.control === "back") return { kind: "back" };
|
||||
if (resp?.control === "exit") return { kind: "exit" };
|
||||
const choice = selectedChoice(resp);
|
||||
if (choice === "approve") return { kind: "approve" };
|
||||
if (choice === "reject") return { kind: "reject" };
|
||||
return { kind: "unknown", choice };
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 4: Rewrite the `reviewer_confirm` guard.** Replace the current emit + guard block (the `const resp = await emitAndWait(ctx, widget);` line through the `control === "exit"` branch, lines 585-594) with an outcome loop:
|
||||
|
||||
```js
|
||||
let outcome;
|
||||
for (;;) {
|
||||
const resp = await emitAndWait(ctx, widget);
|
||||
outcome = resolveConfirmOutcome(resp);
|
||||
if (outcome.kind !== "unknown") break;
|
||||
await ctx.ui.notify("Scegli «Conferma e prosegui» o «Rifiuta».", "warning");
|
||||
}
|
||||
if (outcome.kind === "freetext")
|
||||
return textResult(
|
||||
`Altro (reviewer): ${outcome.text}. Valuta e agisci, poi ri-presenta il gate.`,
|
||||
);
|
||||
if (outcome.kind === "reject")
|
||||
return textResult("Rifiutato: rivedi e riprova.");
|
||||
if (outcome.kind === "back")
|
||||
return textResult("Il reviewer vuole tornare indietro.");
|
||||
if (outcome.kind === "exit")
|
||||
return textResult("Il reviewer vuole uscire.");
|
||||
// outcome.kind === "approve" -> execute the privileged action via the CLI.
|
||||
```
|
||||
|
||||
The four `if (kind === "phase"|"cte_plan"|"cte_result"|"sql")` branches that follow (lines 596-667) are UNCHANGED — they are now reached only after an explicit approve.
|
||||
|
||||
- [ ] **Step 5: Run the test to verify it passes**
|
||||
|
||||
Run: `node --test harness/.pi/extensions/gate/__tests__/gate_confirm_outcome.test.js`
|
||||
Expected: PASS.
|
||||
|
||||
- [ ] **Step 6: Sanity-check the extension still parses**
|
||||
|
||||
Run: `node --check harness/.pi/extensions/tht-gate.js`
|
||||
Expected: no output (syntax OK).
|
||||
|
||||
- [ ] **Step 7: Commit**
|
||||
|
||||
```bash
|
||||
git add harness/.pi/extensions/tht-gate.js harness/.pi/extensions/gate/__tests__/gate_confirm_outcome.test.js
|
||||
git commit -m "fix(gate): reviewer_confirm requires explicit approve; free-text is actionable, not accidental approve"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Task 5: Empty memory phase auto-advances with a message (no empty widget)
|
||||
|
||||
**Files:**
|
||||
- Modify: `harness/.pi/extensions/tht-gate.js` (add `shouldSkipEmptyDecide`; short-circuit in `reviewer_decide` `execute`, before `emitAndWait`, around line 522)
|
||||
- Test: `harness/.pi/extensions/gate/__tests__/gate_decide_empty.test.js` (new)
|
||||
|
||||
**Interfaces:**
|
||||
- Produces (exported pure fn): `shouldSkipEmptyDecide({ meritCount, allowEmpty, advance })` → `true` iff `meritCount === 0 && allowEmpty && advance`.
|
||||
- Behavior: when true, `reviewer_decide` does NOT emit the multiselect. It calls `ctx.ui.notify("Nessuna memory riutilizzabile per questa domanda — passo alla fase successiva.", "info")`, then `advanceIfReady(ctx, session)`, and returns a `textResult`. `advanceIfReady` (unchanged) only advances an auto-eligible phase (F2/F6 with zero substantive decisions), so this cannot advance a phase that recorded decisions.
|
||||
|
||||
- [ ] **Step 1: Write the failing test** — create `harness/.pi/extensions/gate/__tests__/gate_decide_empty.test.js`:
|
||||
|
||||
```js
|
||||
const test = require("node:test");
|
||||
const assert = require("node:assert");
|
||||
const { shouldSkipEmptyDecide } = require("../../tht-gate.js");
|
||||
|
||||
test("skips the widget only when empty AND allow_empty AND advance", () => {
|
||||
assert.equal(shouldSkipEmptyDecide({ meritCount: 0, allowEmpty: true, advance: true }), true);
|
||||
});
|
||||
|
||||
test("does not skip when there are merito options", () => {
|
||||
assert.equal(shouldSkipEmptyDecide({ meritCount: 2, allowEmpty: true, advance: true }), false);
|
||||
});
|
||||
|
||||
test("does not skip when allow_empty is false or advance is false", () => {
|
||||
assert.equal(shouldSkipEmptyDecide({ meritCount: 0, allowEmpty: false, advance: true }), false);
|
||||
assert.equal(shouldSkipEmptyDecide({ meritCount: 0, allowEmpty: true, advance: false }), false);
|
||||
});
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Run the test to verify it fails**
|
||||
|
||||
Run: `node --test harness/.pi/extensions/gate/__tests__/gate_decide_empty.test.js`
|
||||
Expected: FAIL — `shouldSkipEmptyDecide` is not exported.
|
||||
|
||||
- [ ] **Step 3: Implement the pure predicate** in `tht-gate.js`, right after `resolveConfirmOutcome` (from Task 4). TAB indentation:
|
||||
|
||||
```js
|
||||
// True when a reviewer_decide has no merito options but is allowed to close empty
|
||||
// and advance (the empty memory phase F2). The gate then shows an info notice and
|
||||
// auto-advances instead of presenting an empty checklist.
|
||||
export function shouldSkipEmptyDecide({ meritCount, allowEmpty, advance }) {
|
||||
return meritCount === 0 && !!allowEmpty && !!advance;
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 4: Wire the short-circuit into `reviewer_decide`.** In its `execute` (starts line 508), the merito options are built inline at lines 518-520. Refactor to compute them once, then short-circuit BEFORE `buildMultiselectRequest`. Replace the `const widget = buildMultiselectRequest({ ... });` block (lines 513-522) with:
|
||||
|
||||
```js
|
||||
const meritOptions = opts
|
||||
.filter((o) => !isReserved(o.label))
|
||||
.map((o) => ({ id: o.id, label: o.label }));
|
||||
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.",
|
||||
"info",
|
||||
);
|
||||
advanceIfReady(ctx, session);
|
||||
return textResult(
|
||||
"Fase memoria vuota: nessuna decisione da registrare, avanzamento automatico alla fase successiva.",
|
||||
);
|
||||
}
|
||||
const widget = buildMultiselectRequest({
|
||||
id: `u${Date.now()}`,
|
||||
phase,
|
||||
title,
|
||||
allowEmpty: params.allow_empty ?? false,
|
||||
options: meritOptions,
|
||||
recommended: opts.find((o) => o.recommended)?.id ?? null,
|
||||
});
|
||||
```
|
||||
|
||||
- [ ] **Step 5: Run the test + syntax check**
|
||||
|
||||
Run: `node --test harness/.pi/extensions/gate/__tests__/gate_decide_empty.test.js && node --check harness/.pi/extensions/tht-gate.js`
|
||||
Expected: PASS, then no syntax output.
|
||||
|
||||
- [ ] **Step 6: Run the full gate suite to confirm no regression**
|
||||
|
||||
Run: `node --test harness/.pi/extensions/gate/__tests__/`
|
||||
Expected: all tests PASS.
|
||||
|
||||
- [ ] **Step 7: Commit**
|
||||
|
||||
```bash
|
||||
git add harness/.pi/extensions/tht-gate.js harness/.pi/extensions/gate/__tests__/gate_decide_empty.test.js
|
||||
git commit -m "fix(gate): empty memory phase shows a notice and auto-advances instead of an empty checklist"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Task 6: Document the empty-memory short-circuit in SKILL.md
|
||||
|
||||
**Files:**
|
||||
- Modify: `harness/.pi/skills/tht-sessione/SKILL.md` (Phase 2, lines 162-175)
|
||||
|
||||
**Interfaces:** none (documentation). The model's Phase-2 call is unchanged — it still issues one `reviewer_decide(multi:true, advance:true, allow_empty:true)`; the gate now handles the empty case.
|
||||
|
||||
- [ ] **Step 1: Edit Phase 2 step 3** — after the sentence ending "…the phase advances — no separate gate." (line 172), append:
|
||||
|
||||
```markdown
|
||||
When the memory search returned **zero** candidates, still issue the single
|
||||
`reviewer_decide(advance:true, allow_empty:true)` with an empty merito list: the gate
|
||||
detects the empty+advance case, shows the reviewer an info notice ("Nessuna memory
|
||||
riutilizzabile … passo alla fase successiva") and auto-advances F2 — it does NOT present
|
||||
an empty checklist, and you do NOT add a separate `reviewer_confirm kind:"phase"`.
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Verify the surrounding text stays consistent** — re-read Phase 2 step 4 (lines 173-175): the "truly empty memory phase … auto-advances via `advance:true`" sentence still holds (the short-circuit is the mechanism). No change needed there.
|
||||
|
||||
- [ ] **Step 3: Commit**
|
||||
|
||||
```bash
|
||||
git add harness/.pi/skills/tht-sessione/SKILL.md
|
||||
git commit -m "docs(skill): document empty-memory auto-advance (no empty checklist) in Phase 2"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Final verification
|
||||
|
||||
- [ ] **Frontend:** `cd frontend && npx vitest run src/widgets/ && npx tsc -b` — all widget tests pass, no type errors.
|
||||
- [ ] **Gate:** `node --test harness/.pi/extensions/gate/__tests__/` — all gate tests pass.
|
||||
- [ ] **Deferred live check (needs VPN + `pi` on PATH — see PROJECT_STATE):** run a real session through the full stack and confirm at the F1 close gate a **"Conferma e prosegui"** button appears and advances; "Altro — specifica" opens a text box whose text reaches the model; the empty F2 shows the "Nessuna memory…" notice and advances with no checklist; the F1/F4 multiselect shows Back/Exit/Other. This exercises the `tht-gate.js` `execute()` glue that the node tests do not cover.
|
||||
|
||||
## Self-review notes (spec coverage)
|
||||
|
||||
- Spec Part 1 → Task 1. Spec Part 2 → Tasks 3 (options) + 4 (explicit-approve guard). Spec Part 3 → Task 2 (frontend "other"→freetext) + Task 4 (gate freetext actionable). Spec Part 4 → Task 5 (short-circuit) + Task 6 (SKILL). No spec requirement is unassigned.
|
||||
- The spec's Part 4 mentioned `buildInfoRequest`; the concrete primitive that reaches the browser is `ctx.ui.notify(text, "info")` → `session-bridge.ts:26-27` → client `{type:"info"}`. The plan uses `ctx.ui.notify` (the spec is corrected to match).
|
||||
@@ -150,9 +150,9 @@ branch returns the actionable "Altro (reviewer): …" string, distinct from the
|
||||
model already calls `reviewer_decide(allow_empty:true, advance:true)` in Phase 2 with zero
|
||||
merito options when memory is empty. In the handler (`tht-gate.js:508-546`), before emitting
|
||||
the widget: if the merito option list is empty AND `allow_empty` AND `advance`, do NOT emit the
|
||||
multiselect — emit an `info` widget (`buildInfoRequest`, `builders.js:130-145`) with
|
||||
"Nessuna memory riutilizzabile per questa domanda — passo alla fase successiva." and call
|
||||
`advanceIfReady`. Return a textResult reporting the auto-advance. This keeps the harness as the
|
||||
multiselect — show the reviewer an info notice via `ctx.ui.notify("Nessuna memory
|
||||
riutilizzabile per questa domanda — passo alla fase successiva.", "info")` (bridged to the
|
||||
client `{type:"info"}` event by `session-bridge.ts:26-27`) and call `advanceIfReady`. Return a textResult reporting the auto-advance. This keeps the harness as the
|
||||
advancer (respects the anti-bypass hook), reuses `advanceIfReady` (which only advances an
|
||||
auto-eligible phase — F2/F6 with zero substantive decisions), needs no new tool, and leaves the
|
||||
model's Phase-2 call unchanged.
|
||||
|
||||
Reference in New Issue
Block a user