From 91a68066173e42d564ed6924b12ec812fda93be6 Mon Sep 17 00:00:00 2001 From: mptyl Date: Thu, 2 Jul 2026 16:43:28 +0200 Subject: [PATCH] =?UTF-8?q?docs(spec):=20reviewer=20gate=20UX=20fixes=20?= =?UTF-8?q?=E2=80=94=20multiselect=20hatches,=20gate=20forward=20button,?= =?UTF-8?q?=20Altro=20text,=20empty-memory=20auto-advance?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 4.8 --- ...026-07-02-reviewer-gate-ux-fixes-design.md | 198 ++++++++++++++++++ 1 file changed, 198 insertions(+) create mode 100644 docs/superpowers/specs/2026-07-02-reviewer-gate-ux-fixes-design.md diff --git a/docs/superpowers/specs/2026-07-02-reviewer-gate-ux-fixes-design.md b/docs/superpowers/specs/2026-07-02-reviewer-gate-ux-fixes-design.md new file mode 100644 index 00000000..51bb6d20 --- /dev/null +++ b/docs/superpowers/specs/2026-07-02-reviewer-gate-ux-fixes-design.md @@ -0,0 +1,198 @@ +# Reviewer gate UX fixes — design + +> Date: 2026-07-02 · Status: approved (design) · Scope: frontend widgets + harness Pi gate (`tht-gate.js`, `builders.js`) + `SKILL.md`. No `tht` CLI, `phase.py`, or `workflow.yaml` semantic changes. + +## Problem + +Driving a live session surfaced four defects at the reviewer gates. Two user-reported +symptoms ("the multiselect has no Exit/Back"; "phase completion only offers Exit and +Other-specify, no way forward, and Other is ignored") decompose into four root causes, all +verified against source. + +1. **The multiselect widget renders no Exit/Back/Other controls.** `MultiselectWidget` + (`frontend/src/widgets/MultiselectWidget.tsx:30-58`) is the ONLY pick widget that never + renders `` — `SelectWidget` (`SelectWidget.tsx:16`) and + `ArtifactGateWidget` (`ArtifactGateWidget.tsx:54-57`) both do. The backend already sets + `reserved: ["back","exit","other"]` on the multiselect descriptor + (`builders.js:95`) and the `reviewer_decide` handler already maps `control:"back"|"exit"| + "freetext"` (`tht-gate.js:524-532`) — the field is just never consumed by the frontend. + +2. **The phase/artifact gate renders no merito buttons — and "Other" silently approves.** + `reviewer_confirm` builds `buildArtifactGate({..., action:{kind:"approve_reject"}})` + (`tht-gate.js:578-584`), but `buildArtifactGate` (`builders.js:102-126`) never populates + an `options` array. `ArtifactGateWidget` renders merito buttons only from + `descriptor.options?.map(...)` (`ArtifactGateWidget.tsx:44-52`) and ignores `action` + entirely → **zero Approva/Rifiuta buttons**; the reviewer sees only the reserved + [Go back][Exit][Other] controls. There is no forward button. Worse, in the handler + (`tht-gate.js:586-594`) the only guards are `control:"freetext"`/`choice:"reject"` → + reject, `control:"back"` → back, `control:"exit"` → exit; **`control:"other"` matches + none and falls through to the "approved" branch (`:596`), advancing the phase** — with no + text captured. This is the exact "only Exit and Other, no way forward, Other ignored" + symptom: "Other" is an accidental approve. + +3. **"Altro — specifica" never captures text.** The working free-text path is an *option* + carrying `opens:{widget:"freetext"}` (`altroOption`, `builders.js:38-44`) routed through + `LinkageHost` — but `LinkageHost` is wired ONLY in `ArtifactGateWidget` + (`ArtifactGateWidget.tsx:17-33`); `SelectWidget`/`MultiselectWidget` ignore `opens`. The + reserved "Other — specify" button (`ReservedControls.tsx:1-6`) sends a bare + `{control:"other"}` with no text and no child widget, and the harness maps only + `control:"freetext"` (`resolveSelectOutcome`, `tht-gate.js:245-254`; the three handlers) — + so "Other" collects nothing and is dropped (select/decide) or mis-approves (confirm, see #2). + +4. **The empty memory phase (F2) shows a pointless empty checklist.** `SKILL.md` Phase 2 + (`SKILL.md:162-175`) always presents `reviewer_decide(allow_empty:true, advance:true)`, + even when `tht memory search` returned zero candidates. `buildMultiselectRequest` accepts + `allowEmpty:true` with zero options (`builders.js:78-97`), so the reviewer sees an empty + checklist + a "Select all" over nothing + an enabled Confirm; only after that dead click + does `advanceIfReady` (`tht-gate.js:540`, `:193-204`) auto-advance F2 → F3. No "there are + no memories" message is ever shown. F2 already auto-advances; the empty widget is the only + friction. + +## Goals + +- Give `reviewer_decide` (multiselect) the same Back/Exit/Other escape hatches as the other + pick widgets. +- Make the phase/artifact gate render an explicit forward control ("Approva e continua") and + a "Rifiuta" — and stop "Other" from silently approving. +- Make "Altro — specifica" open a text field, carry the text to the model as **actionable + feedback** (recorded verbatim), consistently across pick widgets and the gate. +- When F2 memory is empty, skip the widget: show an info message and auto-advance. + +## Non-goals + +- **No new auto-advance phases.** F1/F3/F5/F7 keep their `reviewer_confirm kind:"phase"` + gate — each reviews something real (F1 "done clarifying", F3 `question.md`, F5 the + schema-linking readable view per Discipline 7, F7 `sql_final.sql`). `_AUTO_ADVANCE_PHASES` + stays `{2,6}` — the deliberate HITL contract (`SKILL.md:8-11`). +- No change to the `advance` semantics in `workflow.yaml` or the `auto_advance_eligible` + rule in `phase.py`. +- No widget-registry restructure and no new widget kind (info/freetext already exist). + +## Part 1 — Multiselect reserved controls (frontend only) + +`MultiselectWidget.tsx`: import `ReservedControls` and render it after the Confirm button, +mirroring `SelectWidget`/`ArtifactGateWidget`: + +```tsx + onRespond({ id: descriptor.id, control: c })} +/> +``` + +No backend change — `reviewer_decide` already emits `reserved` and handles the responses. +Result: options + Select-all + **[Go back] [Exit] [Other — specify]**. ("Other" is made +functional in Part 3.) + +**Tests** — vitest (`MultiselectWidget.test.tsx`, extend): the three reserved buttons render; +clicking each fires `onRespond({id, control})` with `back`/`exit`/`other`; the merito Confirm +still emits `{kind:"multiselect", choices}`. + +## Part 2 — Gate renders explicit merito options; "Other" no longer approves (harness + test) + +**Backend — derive `options` from `action.kind` in `buildArtifactGate`** (`builders.js`). +`ArtifactGateWidget` is unchanged (it already maps `descriptor.options`); the fix is to make +the builder populate them: + +- `action.kind:"approve_reject"` → `options:[{id:"approve", label:"Conferma e prosegui", + recommended:true}, {id:"reject", label:"Rifiuta"}]` +- `action.kind:"confirm"` → `options:[{id:"approve", label:"Conferma e prosegui", + recommended:true}]` +- `action.kind:"view_only"` → `options:[]` (no merito action; reserved controls only) + +The forward option is labelled "Conferma e prosegui" so the "advance to next phase" +affordance is unambiguous (the user's core complaint). `recommended` floats it to the top +(the frontend already renders `(consigliato)` on select; the artifact-gate button list keeps +`approve` first). + +**Backend — tighten the `reviewer_confirm` handler** (`tht-gate.js:585-594`): advance ONLY on +an explicit approve choice. Replace the permissive fall-through with: + +- `selectedChoice(resp) === "approve"` → run the privileged action (existing `kind` branches). +- `selectedChoice(resp) === "reject"` → "Rifiutato: rivedi e riprova." +- `control:"freetext"` → actionable feedback (Part 3), NOT reject. +- `control:"back"`/`"exit"` → unchanged. +- anything else (incl. a stray `control:"other"` that Part 3 will eliminate) → re-present the + widget via the no-limbo loop; never auto-approve. + +**Tests** — node `builders.test.js` (extend): `buildArtifactGate` with each `action.kind` +yields the documented `options`; `approve` is first and `recommended`. The `ArtifactGateWidget` +test already injects `options:[{id:"approve",...}]` — keep it aligned. Handler wiring is L2/live +per repo convention; the behavioral guarantee rides on `builders.test.js` + a deferred live +phase-gate check. + +## Part 3 — "Altro — specifica" captures text and is actionable (frontend + harness) + +**Frontend — make the reserved "other" control open a freetext child, uniformly.** Rather than +per-widget `opens` wiring, centralize: when a reserved control `"other"` is clicked, collect +text via `FreetextWidget` (reusing the `LinkageHost` merge shape) and submit +`{id, control:"freetext", text}` — the response the harness already understands +(`resolveSelectOutcome`/handlers map `control:"freetext"`). Concretely, `ReservedControls` +(or a small wrapper it delegates to) renders an inline `FreetextWidget` on "other" instead of +firing `onControl("other")` immediately. This fixes "Other" for select, multiselect, and the +gate in one place, and the bare `control:"other"` disappears from the wire. + +**Harness — treat gate free-text as actionable feedback, not rejection** (`tht-gate.js:586-589`). +Split the current combined guard: `choice:"reject"` → "Rifiutato: rivedi e riprova."; +`control:"freetext"` → return `Altro (reviewer): . Valuta e agisci, poi ri-presenta il +gate.` — matching the wording already used by `reviewer_decide`/`reviewer_select` +(`tht-gate.js:456,:526`) and Discipline 10 (record the reviewer's words verbatim in the +decision `rationale`). "Other" is present only where a real widget renders — with Part 4 the +empty-memory case shows no widget, so "Other" never appears on a no-op gate (approach A / C). + +**Tests** — vitest: clicking reserved "Other" reveals a text input; submitting sends +`{control:"freetext", text}` (not `control:"other"`). node: the `reviewer_confirm` freetext +branch returns the actionable "Altro (reviewer): …" string, distinct from the reject string. + +## Part 4 — Empty memory phase auto-advances with a message (harness + SKILL) + +**Chosen mechanism (option i): short-circuit the empty case inside `reviewer_decide`.** 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 +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. + +**SKILL.md Phase 2** (`SKILL.md:162-175`): document the short-circuit — when +`tht memory search` returns no candidates, still issue the single +`reviewer_decide(allow_empty:true, advance:true)` with empty merito options; the gate shows the +"no memories" info and auto-advances (no separate `reviewer_confirm kind:"phase"`). The +non-empty path is unchanged (selected applied, deselected recorded, closed by the phase gate). + +**Tests** — node: `reviewer_decide` with empty options + `allow_empty:true` + `advance:true` +emits an `info` descriptor and does not emit a `multiselect`, then calls the advance path +(assert via the fake ctx/`tht` shim used by the gate tests). Non-empty options → unchanged +(multiselect emitted, no info). No `phase.py` change, so no pytest. + +## Build order & testing + +Order: **Part 1 → Part 2 → Part 3 → Part 4.** Part 1 is a self-contained frontend fix. Part 2 +makes the gate usable (its forward button). Part 3 depends on Part 2 (both touch the +`reviewer_confirm` handler + the "other" wire) and removes the bare `control:"other"`. Part 4 +is orthogonal but lands last so the SKILL text reflects the finished gate behavior. + +TDD where cheap: vitest RED→GREEN on the widget changes (`cd frontend && npx vitest run`); +node RED→GREEN on `builders.test.js` and the gate handler tests +(`node --test` under `harness/.pi/extensions/gate/__tests__`). Typecheck the frontend +(`cd frontend && npx tsc -b`). The `tht-gate.js` glue is L2/live per repo convention; full +verification of the phase gate + empty-memory advance through the real stack is deferred (needs +VPN — see PROJECT_STATE open items). + +## Risks + +- **Gate glue is not unit-tested** (repo convention): the `reviewer_confirm` handler tightening + (Part 2) and the empty-memory short-circuit (Part 4) rely on `builders.test.js` + gate handler + tests + a deferred live check. Mitigation: keep handler logic minimal; push option-shaping into + the pure `builders.js` (which IS unit-tested). +- **Reserved "other" → freetext centralization (Part 3)** changes a shared component + (`ReservedControls`) used by every pick widget. Mitigation: cover all three widgets with a + vitest each; keep the non-"other" controls (back/exit) firing immediately as today. +- **Empty-memory short-circuit scope:** the `reviewer_decide` short-circuit triggers on *any* + empty+`allow_empty`+`advance` call, not F2 by name. This matches intent (only a trivially + empty auto-eligible phase advances — `advanceIfReady` enforces eligibility), but the test must + assert a non-empty call is untouched so normal decides never short-circuit. +```