fix: clarify memory selection semantics
This commit is contained in:
@@ -0,0 +1,115 @@
|
||||
# Memory Selection Clarity 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:** Make Phase 2 memory checkboxes mean only “apply this memory now”, with recommended memories preselected and unselected memories left undecided.
|
||||
|
||||
**Architecture:** The Pi gate converts each `recommended:true` Phase 2 option into the existing multiselect descriptor’s `selected` flag. The shared React multiselect keeps its generic behavior, while Phase 2 supplies explicit Italian selection and confirmation copy through descriptor fields. Only selected IDs continue to create decisions; omitted IDs remain non-persisting.
|
||||
|
||||
**Tech Stack:** Pi extension JavaScript (`node:test`), React 18/TypeScript, Vitest.
|
||||
|
||||
## Global Constraints
|
||||
|
||||
- UI copy is English except workspace/document content; this Phase 2 reviewer prompt is workspace-language Italian and stays Italian.
|
||||
- No unselected memory may create `memory_rejected` or any other decision.
|
||||
- Keep the generic multiselect compatible with existing widgets.
|
||||
|
||||
---
|
||||
|
||||
### Task 1: Carry recommendations into checked Phase 2 options
|
||||
|
||||
**Files:**
|
||||
- Modify: `harness/.pi/extensions/tht-gate.js:686-750`
|
||||
- Test: `harness/.pi/extensions/gate/__tests__/gate_reviewer_decide.test.js`
|
||||
|
||||
**Interfaces:**
|
||||
- Consumes: `reviewer_decide.options[].recommended?: boolean`.
|
||||
- Produces: a multiselect descriptor whose recommended option IDs occur in `selected`, and whose Phase 2 title/action communicate application only.
|
||||
|
||||
- [ ] **Step 1: Write the failing test**
|
||||
|
||||
Exercise the gate with one recommended and one optional decision, capture its UI request, and assert `selected` is the recommended ID, the title asks to select memories to apply, and `confirm_label` is `Applica le memory selezionate`.
|
||||
|
||||
- [ ] **Step 2: Run test to verify it fails**
|
||||
|
||||
Run: `node --test harness/.pi/extensions/gate/__tests__/gate_reviewer_decide.test.js`
|
||||
|
||||
Expected: FAIL because the descriptor has no `selected` recommendation mapping or memory-specific copy.
|
||||
|
||||
- [ ] **Step 3: Write minimal implementation**
|
||||
|
||||
In `reviewer_decide`, map each recommended option to `selected: true` before calling `buildMultiselectRequest`; add optional `selection_label` and `confirm_label` fields to the descriptor only for the F2 memory title.
|
||||
|
||||
- [ ] **Step 4: Run the gate tests**
|
||||
|
||||
Run: `node --test harness/.pi/extensions/gate/__tests__/*.test.js`
|
||||
|
||||
Expected: PASS.
|
||||
|
||||
### Task 2: Render Phase 2 action copy without changing generic multiselect behavior
|
||||
|
||||
**Files:**
|
||||
- Modify: `frontend/src/api/types.ts`
|
||||
- Modify: `frontend/src/widgets/MultiselectWidget.tsx`
|
||||
- Test: `frontend/src/widgets/MultiselectWidget.test.tsx`
|
||||
|
||||
**Interfaces:**
|
||||
- Consumes: optional `WidgetDescriptor.selection_label?: string` and `WidgetDescriptor.confirm_label?: string`.
|
||||
- Produces: supplied labels in the count and confirm button; absent labels retain `selected` and `Confirm`.
|
||||
|
||||
- [ ] **Step 1: Write the failing test**
|
||||
|
||||
Render a descriptor with one checked recommended memory and the custom labels. Assert the checkbox is checked, the count reads `1 / 2 memory da applicare`, and confirmation uses `Applica le memory selezionate` while emitting only checked IDs.
|
||||
|
||||
- [ ] **Step 2: Run test to verify it fails**
|
||||
|
||||
Run: `cd frontend && npx vitest run src/widgets/MultiselectWidget.test.tsx`
|
||||
|
||||
Expected: FAIL because the descriptor type and component ignore the custom labels.
|
||||
|
||||
- [ ] **Step 3: Write minimal implementation**
|
||||
|
||||
Add the two optional descriptor fields and use them with the existing generic strings as fallbacks. Do not alter checkbox response semantics.
|
||||
|
||||
- [ ] **Step 4: Run widget test and typecheck**
|
||||
|
||||
Run: `cd frontend && npx vitest run src/widgets/MultiselectWidget.test.tsx && npx tsc -b`
|
||||
|
||||
Expected: PASS.
|
||||
|
||||
### Task 3: Align the Phase 2 model instructions with implementation
|
||||
|
||||
**Files:**
|
||||
- Modify: `harness/.pi/skills/tht-sessione/memoria.md`
|
||||
- Modify: `harness/.pi/skills/tht-sessione/SKILL.md`
|
||||
|
||||
**Interfaces:**
|
||||
- Consumes: selected options are applied; omitted options are not applied now.
|
||||
- Produces: instructions that never ask the model to create a negative “do not use” option or a persistent rejection for unchecked memories.
|
||||
|
||||
- [ ] **Step 1: Update the memory checklist contract**
|
||||
|
||||
State that every row describes a candidate memory declaratively, `recommended:true` preselects only memories proposed for use, and unchecked candidates are not applied now and may be considered again after a reopen.
|
||||
|
||||
- [ ] **Step 2: Verify no obsolete rejection instruction remains**
|
||||
|
||||
Run: `rg -n "deselected.*memory_rejected|deselezionate.*memory_rejected|mem_id" harness/.pi/skills/tht-sessione`
|
||||
|
||||
Expected: no matches.
|
||||
|
||||
### Task 4: Full verification
|
||||
|
||||
**Files:**
|
||||
- Verify only.
|
||||
|
||||
- [ ] **Step 1: Run focused gate and frontend checks**
|
||||
|
||||
Run: `node --test harness/.pi/extensions/gate/__tests__/*.test.js && cd frontend && npx vitest run src/widgets/MultiselectWidget.test.tsx && npx tsc -b`
|
||||
|
||||
Expected: all commands exit 0.
|
||||
|
||||
- [ ] **Step 2: Review the diff**
|
||||
|
||||
Run: `git diff --check && git diff -- harness/.pi/extensions/tht-gate.js frontend/src/api/types.ts frontend/src/widgets/MultiselectWidget.tsx harness/.pi/skills/tht-sessione`
|
||||
|
||||
Expected: no whitespace errors; changes only implement the clarified memory-selection contract.
|
||||
@@ -36,6 +36,8 @@ export interface WidgetDescriptor {
|
||||
options?: WidgetOption[];
|
||||
reserved?: string[];
|
||||
allow_empty?: boolean;
|
||||
selection_label?: string;
|
||||
confirm_label?: string;
|
||||
artifact?: { kind: string; content?: string; [k: string]: unknown };
|
||||
level?: "info" | "warning" | "error";
|
||||
text?: string;
|
||||
|
||||
@@ -22,6 +22,30 @@ test("confirms the checked ids including initially selected", async () => {
|
||||
expect(onRespond).toHaveBeenCalledWith({ id: "u1", kind: "multiselect", choices: ["t1", "t2"] });
|
||||
});
|
||||
|
||||
test("uses memory-specific selection copy without changing selected ids", async () => {
|
||||
const onRespond = vi.fn();
|
||||
render(
|
||||
<MultiselectWidget
|
||||
descriptor={{
|
||||
id: "u-memory",
|
||||
widget: "multiselect",
|
||||
selection_label: "memory da applicare",
|
||||
confirm_label: "Applica le memory selezionate",
|
||||
options: [
|
||||
{ id: "recommended", label: "Memory raccomandata", selected: true },
|
||||
{ id: "optional", label: "Memory opzionale", selected: false },
|
||||
],
|
||||
}}
|
||||
onRespond={onRespond}
|
||||
/>
|
||||
);
|
||||
|
||||
expect(screen.getByRole("checkbox", { name: /memory raccomandata/i })).toBeChecked();
|
||||
expect(screen.getByText("1 / 2 memory da applicare")).toBeInTheDocument();
|
||||
await userEvent.click(screen.getByRole("button", { name: "Applica le memory selezionate" }));
|
||||
expect(onRespond).toHaveBeenCalledWith({ id: "u-memory", kind: "multiselect", choices: ["recommended"] });
|
||||
});
|
||||
|
||||
test("select-all checks all options", async () => {
|
||||
const onRespond = vi.fn();
|
||||
render(
|
||||
|
||||
@@ -39,7 +39,7 @@ export function MultiselectWidget({ descriptor, onRespond }: WidgetProps) {
|
||||
{allChecked ? "Deselect all" : "Select all"}
|
||||
</button>
|
||||
<span className="text-xs tabular-nums text-muted-foreground">
|
||||
{checked.size} / {options.length} selected
|
||||
{checked.size} / {options.length} {descriptor.selection_label ?? "selected"}
|
||||
</span>
|
||||
</div>
|
||||
<div className="flex flex-col gap-1 pt-2">
|
||||
@@ -67,7 +67,7 @@ export function MultiselectWidget({ descriptor, onRespond }: WidgetProps) {
|
||||
disabled={isDisabled}
|
||||
onClick={() => onRespond({ id: descriptor.id, kind: "multiselect", choices: Array.from(checked) })}
|
||||
>
|
||||
Confirm
|
||||
{descriptor.confirm_label ?? "Confirm"}
|
||||
</button>
|
||||
</div>
|
||||
<ReservedControls
|
||||
|
||||
@@ -0,0 +1,18 @@
|
||||
const test = require("node:test");
|
||||
const assert = require("node:assert");
|
||||
const { memorySelectionWidgetProps } = require("../../tht-gate.js");
|
||||
|
||||
test("F2 preseleziona solo le memory raccomandate e parla di applicazione", () => {
|
||||
assert.deepEqual(
|
||||
memorySelectionWidgetProps([
|
||||
{ id: "recommended", label: "Memory A", recommended: true },
|
||||
{ id: "optional", label: "Memory B" },
|
||||
]),
|
||||
{
|
||||
title: "Seleziona le memory da applicare alla domanda",
|
||||
selected: ["recommended"],
|
||||
selectionLabel: "memory da applicare",
|
||||
confirmLabel: "Applica le memory selezionate",
|
||||
},
|
||||
);
|
||||
});
|
||||
@@ -7,6 +7,13 @@ const { createFakePi } = require("./fake_pi_runtime.js");
|
||||
const installGate = require("../../tht-gate.js").default ?? require("../../tht-gate.js");
|
||||
|
||||
test("con THT_SESSION il kickoff usa l'id fornito e NON crea la sessione", async () => {
|
||||
const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "tht-gate-no-pack-"));
|
||||
const bin = path.join(tmp, "bin");
|
||||
fs.mkdirSync(bin);
|
||||
const tht = path.join(bin, "tht");
|
||||
fs.writeFileSync(tht, "#!/bin/sh\nexit 1\n", { mode: 0o755 });
|
||||
const oldPath = process.env.PATH;
|
||||
process.env.PATH = bin + ":" + oldPath;
|
||||
process.env.THT_SESSION = "2026-06-27-100000-test";
|
||||
try {
|
||||
const { pi, ctx } = createFakePi();
|
||||
@@ -19,13 +26,15 @@ test("con THT_SESSION il kickoff usa l'id fornito e NON crea la sessione", async
|
||||
assert.doesNotMatch(text, /tht session new/);
|
||||
assert.match(text, /retrieval pack non era ancora disponibile/i);
|
||||
assert.match(text, /tht search pack/);
|
||||
assert.doesNotMatch(text, /<retrieval-pack>/);
|
||||
assert.doesNotMatch(text, /<retrieval-pack>\s*# Retrieval pack/);
|
||||
assert.match(text, /<tht-sessione-skill>/);
|
||||
assert.match(text, /# Thoth session workflow \(phases 1-8\)/);
|
||||
assert.match(text, /non esplorare il repository/i);
|
||||
assert.doesNotMatch(text, /Carica la skill leggendo/);
|
||||
} finally {
|
||||
process.env.PATH = oldPath;
|
||||
delete process.env.THT_SESSION;
|
||||
fs.rmSync(tmp, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
|
||||
@@ -61,6 +61,7 @@ function buildSelectRequest({
|
||||
// `allowEmpty:false` with zero options is a broken widget and throws.
|
||||
function buildMultiselectRequest({
|
||||
id, phase, title, options, content = null, selected = [], allowEmpty = false,
|
||||
selectionLabel, confirmLabel,
|
||||
}) {
|
||||
requireString(title, "title", "multiselect");
|
||||
const opts = requireArray(options, "options", "multiselect");
|
||||
@@ -77,10 +78,12 @@ function buildMultiselectRequest({
|
||||
widget: "multiselect",
|
||||
title,
|
||||
allow_empty: allowEmpty,
|
||||
options: [...opts],
|
||||
options: opts.map((option) => ({ ...option, selected: selected.includes(option.id) })),
|
||||
selected: [...selected],
|
||||
content,
|
||||
reserved: RESERVED,
|
||||
...(selectionLabel ? { selection_label: selectionLabel } : {}),
|
||||
...(confirmLabel ? { confirm_label: confirmLabel } : {}),
|
||||
};
|
||||
}
|
||||
|
||||
|
||||
@@ -386,6 +386,17 @@ export function shouldSkipEmptyDecide({ meritCount, allowEmpty, advance }) {
|
||||
return meritCount === 0 && !!allowEmpty && !!advance;
|
||||
}
|
||||
|
||||
// Phase 2 uses a single positive checkbox meaning: apply this memory now.
|
||||
// Candidates not checked by the reviewer remain undecided and may be shown again.
|
||||
export function memorySelectionWidgetProps(options) {
|
||||
return {
|
||||
title: "Seleziona le memory da applicare alla domanda",
|
||||
selected: options.filter((option) => option.recommended).map((option) => option.id),
|
||||
selectionLabel: "memory da applicare",
|
||||
confirmLabel: "Applica le memory selezionate",
|
||||
};
|
||||
}
|
||||
|
||||
// --- F8 memory-promotion gate: pure candidate->widget mapping (L1-tested) ------
|
||||
// The candidates come from `tht memory promote --preview --json` (deterministic,
|
||||
// reviewer-approved decisions only); the model never authors them.
|
||||
@@ -730,10 +741,10 @@ export default function (pi) {
|
||||
const widget = buildMultiselectRequest({
|
||||
id: `u${Date.now()}`,
|
||||
phase,
|
||||
title,
|
||||
title: phase === "F2" ? memorySelectionWidgetProps(opts).title : title,
|
||||
allowEmpty: params.allow_empty ?? false,
|
||||
options: meritOptions,
|
||||
recommended: opts.find((o) => o.recommended)?.id ?? null,
|
||||
...(phase === "F2" ? memorySelectionWidgetProps(opts) : {}),
|
||||
});
|
||||
const resp = await emitAndWait(ctx, widget);
|
||||
if (resp.control === "freetext") {
|
||||
|
||||
@@ -215,21 +215,21 @@ Prerequisite: Phase 1 closed.
|
||||
allow_empty:true)`. Rules: at most **5** candidates; ONLY the 3 reusable types
|
||||
(`concept_clarified`, `table_promoted`, `table_excluded`) — query-specific
|
||||
decisions (`question_rewritten`, `sql_approved`, …) are NOT transferable, never
|
||||
propose them. Each option carries `mem_id:"mem-<id>"` plus `type`/`subject`/
|
||||
`rationale`. Selected options are applied (register the decision citing the
|
||||
`mem_id` in the rationale); deselected ones are recorded as `memory_rejected` by
|
||||
the gate (so the next `tht memory search --session` won't re-propose them). The
|
||||
checklist starts pre-selected with the recommended memories. With `allow_empty:true`
|
||||
an empty selection is accepted (no memory applied; deselected still recorded) and
|
||||
the phase advances — no separate gate.
|
||||
propose them. Each option carries `type`/`subject`/`rationale`; cite the source
|
||||
memory id (`mem-<id>`) in its rationale when applying it. Every option describes a
|
||||
candidate memory; never create an opposite "do not use" option. Only
|
||||
`recommended:true` options start checked. A
|
||||
deselected candidate is **not applied now**, not rejected, and may be considered
|
||||
again if Phase 2 is reopened. With `allow_empty:true` an empty selection is accepted
|
||||
(no memory applied) and the phase advances — no separate gate.
|
||||
When the memory search returned **zero** candidates, still issue the single
|
||||
`reviewer_decide(multi:true, 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"`.
|
||||
4. Closing: if memories were applied or rejected (substantive decisions), `advance:true`
|
||||
no-ops — close with `reviewer_confirm kind:"phase"`. Only a truly empty memory phase
|
||||
(nothing applied, nothing rejected) auto-advances via `advance:true`.
|
||||
4. Closing: if one or more memories were applied (substantive decisions), `advance:true`
|
||||
no-ops — close with `reviewer_confirm kind:"phase"`. If none is applied, F2
|
||||
auto-advances via `advance:true`.
|
||||
5. Memories are promoted at the END of the workflow (Phase 8, the
|
||||
`reviewer_memory_promote` gate) — never promote from here, never run
|
||||
`tht memory promote`/`save-one` yourself (the gate blocks them).
|
||||
|
||||
@@ -24,14 +24,13 @@ Rules:
|
||||
(e.g. `question_rewritten`, `sql_approved`) are NOT to be proposed: they don't
|
||||
transfer to other questions.
|
||||
- All candidate memories go in **a single** `reviewer_decide(multi:true,
|
||||
advance:true, allow_empty:true)`: each selected option is applied (register the
|
||||
decision with the appropriate type, citing the memory id in the rationale), each
|
||||
deselected option is **recorded as `memory_rejected` by the gate** (so the next
|
||||
`tht memory search --session` won't re-propose it). To enable this, EVERY memory
|
||||
option MUST carry the field `mem_id:"mem-<id>"` (besides `type`/`subject`/
|
||||
`rationale`). The checklist starts pre-selected with the recommended memories.
|
||||
With `allow_empty:true` an **empty selection is accepted** (no memory applied; the
|
||||
deselected ones are still recorded as rejected) and the phase advances — no
|
||||
advance:true, allow_empty:true)`: every option describes a candidate memory, never
|
||||
an opposite action such as "do not use it". Each selected option is applied
|
||||
(register the decision with the appropriate type, citing the memory id in the
|
||||
rationale). Mark `recommended:true` ONLY on memories proposed for use: those, and
|
||||
only those, start checked. An unchecked memory is **not applied now**; it is not a
|
||||
rejection and may be considered again if Phase 2 is reopened. With `allow_empty:true`
|
||||
an **empty selection is accepted** (no memory applied) and the phase advances — no
|
||||
separate gate.
|
||||
- For "inspect": show the memory's full JSON record in the prose before presenting
|
||||
the checklist, if the reviewer asks.
|
||||
|
||||
Reference in New Issue
Block a user