docs(spec): reviewer gate UX fixes — multiselect hatches, gate forward button, Altro text, empty-memory auto-advance
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Claude Opus 4.8
@@ -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 `<ReservedControls>` — `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
|
||||
<ReservedControls
|
||||
reserved={descriptor.reserved}
|
||||
onControl={(c) => 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): <text>. 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.
|
||||
```
|
||||
Reference in New Issue
Block a user