feat(harness): F6/F7 close on their last approval — no echo phase gate
Where completeness is machine-detectable, the reviewer's last substantive approval now closes the phase itself (same pattern as F3/F4/F8): - F6: approving the LAST CTE of the plan (kind:"cte_result" with next_cte now empty) advances the phase; a non-final CTE keeps the phase open and names the next one. - F7: kind:"sql" records sql_approved — which IS F7's only advance prerequisite — and advances immediately. Two reviewer interactions per session removed, both pure echoes. The summary phase gate remains only where completeness is a human judgment (F1, F2 with recorded memories, F5). SKILL.md states the rule and the five self-closing gates; L1 tests cover last/non-last CTE and sql close. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Claude Fable 5
@@ -0,0 +1,106 @@
|
||||
// F6/F7 auto-close: where completeness is machine-detectable the LAST substantive
|
||||
// approval closes the phase itself — approving the final CTE of the plan (F6) and
|
||||
// approving the SQL (F7) must run `tht phase advance` without a separate
|
||||
// reviewer_confirm kind:"phase"; a non-final CTE must NOT advance.
|
||||
//
|
||||
// HARNESS NOTE: the gate's ESM `import { execFileSync }` is snapshotted from the CJS
|
||||
// namespace at first require — later reassignments of cp.execFileSync are NOT seen.
|
||||
// Install ONE dispatcher before requiring the gate; tests swap its `current` handler.
|
||||
const test = require("node:test");
|
||||
const assert = require("node:assert");
|
||||
const cp = require("node:child_process");
|
||||
const { createRequire } = require("node:module");
|
||||
const path = require("node:path");
|
||||
|
||||
const GATE = path.join(__dirname, "..", "..", "tht-gate.js");
|
||||
if (typeof globalThis.require === "undefined") {
|
||||
globalThis.require = createRequire(GATE);
|
||||
}
|
||||
|
||||
const shell = { current: () => "" };
|
||||
cp.execFileSync = (file, args, opts) => shell.current(file, args, opts);
|
||||
|
||||
const META = JSON.stringify({
|
||||
max_phase: 8,
|
||||
phases: [
|
||||
{ num: 6, id: "F6" },
|
||||
{ num: 7, id: "F7" },
|
||||
{ num: 8, id: "F8" },
|
||||
],
|
||||
});
|
||||
|
||||
// `cte next` walks the queue one entry per `decision add cte_approved`.
|
||||
function useShell({ phase, cteQueue = [] }) {
|
||||
const calls = [];
|
||||
const queue = [...cteQueue];
|
||||
shell.current = (file, args) => {
|
||||
calls.push(args.join(" "));
|
||||
if (args[0] === "phase" && args[1] === "meta") return META;
|
||||
if (args[0] === "phase" && args[1] === "show") return `Fase corrente: ${phase}\n`;
|
||||
if (args[0] === "cte" && args[1] === "next") return queue.length ? `${queue[0]}\n` : "";
|
||||
if (args[0] === "decision" && args[1] === "add" && args.includes("cte_approved")) {
|
||||
queue.shift();
|
||||
return "";
|
||||
}
|
||||
return "";
|
||||
};
|
||||
return calls;
|
||||
}
|
||||
|
||||
async function runConfirm(kind, artifactKind) {
|
||||
const gate = require(GATE);
|
||||
const { createFakePi } = require("./fake_pi_runtime.js");
|
||||
const { pi, ctx, tools } = createFakePi();
|
||||
ctx.cwd = "/nonexistent-thothii-test-cwd";
|
||||
gate.default(pi);
|
||||
// Reset the module-level phase-meta cache captured by a previous test's handler.
|
||||
await pi.emit("session_start", {});
|
||||
ctx.ui.input = async (title) => {
|
||||
const d = JSON.parse(title);
|
||||
return JSON.stringify({ id: d.id, kind: "artifact-gate", choices: ["approve"] });
|
||||
};
|
||||
return tools.get("reviewer_confirm").def.execute(
|
||||
"call-1",
|
||||
{ session: "s1", kind, title: "t", artifact: { kind: artifactKind, data: "# md" } },
|
||||
null,
|
||||
null,
|
||||
ctx,
|
||||
);
|
||||
}
|
||||
|
||||
test("approving a NON-final CTE records it and does NOT advance the phase", async () => {
|
||||
const calls = useShell({ phase: 6, cteQueue: ["cte_a", "cte_b"] });
|
||||
const result = await runConfirm("cte_result", "cte_result");
|
||||
assert.match(result.content[0].text, /cte_a.*approvato/i);
|
||||
assert.match(result.content[0].text, /cte_b/);
|
||||
assert.ok(!calls.some((c) => c.startsWith("phase advance")), `no advance in: ${calls}`);
|
||||
});
|
||||
|
||||
test("approving the LAST CTE of the plan closes F6 automatically", async () => {
|
||||
const calls = useShell({ phase: 6, cteQueue: ["cte_finale"] });
|
||||
const result = await runConfirm("cte_result", "cte_result");
|
||||
assert.match(result.content[0].text, /cte_finale.*approvato/i);
|
||||
assert.match(result.content[0].text, /chiusa automaticamente/);
|
||||
assert.match(result.content[0].text, /NON presentare reviewer_confirm/);
|
||||
assert.ok(
|
||||
calls.some((c) => c === "phase advance --session s1"),
|
||||
`expected phase advance in: ${calls}`,
|
||||
);
|
||||
assert.ok(!calls.some((c) => c.startsWith("session finalize")), "F6 must not finalize");
|
||||
});
|
||||
|
||||
test("approving the SQL closes F7 automatically (no separate phase gate)", async () => {
|
||||
const calls = useShell({ phase: 7 });
|
||||
const result = await runConfirm("sql", "sql");
|
||||
assert.match(result.content[0].text, /SQL approvato/);
|
||||
assert.match(result.content[0].text, /chiusa automaticamente/);
|
||||
assert.ok(
|
||||
calls.some((c) => c.includes("decision add --session s1 --type sql_approved")),
|
||||
`expected sql_approved in: ${calls}`,
|
||||
);
|
||||
assert.ok(
|
||||
calls.some((c) => c === "phase advance --session s1"),
|
||||
`expected phase advance in: ${calls}`,
|
||||
);
|
||||
assert.ok(!calls.some((c) => c.startsWith("session finalize")), "F7 must not finalize");
|
||||
});
|
||||
@@ -1299,7 +1299,28 @@ export default function (pi) {
|
||||
"",
|
||||
);
|
||||
if (err) return err;
|
||||
return textResult(`CTE '${cteName}' approvato (sessione ${session}).`);
|
||||
// Deterministic completeness: when no CTE remains in the plan the reviewer
|
||||
// has approved everything F6 can ask — close the phase here instead of
|
||||
// echoing a reviewer_confirm kind:"phase" that decides nothing new.
|
||||
const remaining = tht(ctx, ["cte", "next", "--session", session]).trim();
|
||||
if (remaining) {
|
||||
return textResult(
|
||||
`CTE '${cteName}' approvato (sessione ${session}). Prossimo CTE del piano: '${remaining}'.`,
|
||||
);
|
||||
}
|
||||
const closed = advancePhaseAndFinalize(ctx, session, curNum);
|
||||
if (closed.err) return closed.err;
|
||||
if (closed.finalized) {
|
||||
lockActive = false;
|
||||
lastSteered = false;
|
||||
return textResult(
|
||||
`CTE '${cteName}' approvato — era l'ultimo del piano: fase chiusa e sessione finalizzata (${session}).`,
|
||||
);
|
||||
}
|
||||
return textResult(
|
||||
`CTE '${cteName}' approvato — era l'ultimo del piano: Fase ${curNum} chiusa automaticamente. ` +
|
||||
'NON presentare reviewer_confirm kind:"phase" per questa fase; prosegui con la successiva.',
|
||||
);
|
||||
}
|
||||
if (kind === "sql") {
|
||||
const err = relayIfThtFails(
|
||||
@@ -1317,7 +1338,19 @@ export default function (pi) {
|
||||
"",
|
||||
);
|
||||
if (err) return err;
|
||||
return textResult(`SQL approvato (sessione ${session}).`);
|
||||
// F7's only prerequisite IS sql_approved (workflow.yaml): the phase gate
|
||||
// after it decides nothing new — close the phase here.
|
||||
const closed = advancePhaseAndFinalize(ctx, session, curNum);
|
||||
if (closed.err) return closed.err;
|
||||
if (closed.finalized) {
|
||||
lockActive = false;
|
||||
lastSteered = false;
|
||||
return textResult(`SQL approvato — fase chiusa e sessione finalizzata (${session}).`);
|
||||
}
|
||||
return textResult(
|
||||
`SQL approvato — Fase ${curNum} chiusa automaticamente. ` +
|
||||
'NON presentare reviewer_confirm kind:"phase" per questa fase; prosegui con la successiva (datamart/memorie).',
|
||||
);
|
||||
}
|
||||
return textResult(`Approvato (kind=${kind}, sessione ${session}).`);
|
||||
} catch (fatal) {
|
||||
|
||||
@@ -26,13 +26,16 @@ about a domain term, ask the reviewer.
|
||||
## Phase map (advance cheat-sheet)
|
||||
|
||||
A phase advances ONLY when a `phase_approved:phase:N` decision is recorded for the current
|
||||
phase — written by `reviewer_confirm kind:"phase"`. (F7 is two-step: `kind:"sql"` records
|
||||
`sql_approved`, then `kind:"phase"` advances.) A `reviewer_decide`/
|
||||
phase. THE RULE: where completeness is machine-detectable, the LAST substantive approval
|
||||
closes the phase itself; a `reviewer_confirm kind:"phase"` summary gate exists only where
|
||||
completeness is a human judgment (F1, F2 with recorded memories, F5). A `reviewer_decide`/
|
||||
`reviewer_select` choice records its OWN decision but does NOT advance the phase. `advance:true`
|
||||
on `reviewer_decide` auto-advances only F2 (empty memory) and F6 (skipped/empty) — never a
|
||||
phase that recorded substantive decisions. Exactly three gates close their phase themselves,
|
||||
because there the human interaction IS the phase approval: `rewrite_question` (F3),
|
||||
`reviewer_schema_linking` with `advance:true` (F4), and `reviewer_memory_promote` (F8).
|
||||
phase that recorded substantive decisions. FIVE gates close their phase themselves, because
|
||||
there the human interaction IS the phase approval: `rewrite_question` (F3),
|
||||
`reviewer_schema_linking` with `advance:true` (F4), the LAST `reviewer_confirm
|
||||
kind:"cte_result"` of the plan (F6), `reviewer_confirm kind:"sql"` (F7), and
|
||||
`reviewer_memory_promote` (F8).
|
||||
|
||||
| Phase | Artifact out | Advance / close by |
|
||||
|-------|--------------|--------------------|
|
||||
@@ -41,8 +44,8 @@ because there the human interaction IS the phase approval: `rewrite_question` (F
|
||||
| F3 riscrittura | `question.md` | `rewrite_question` records approval and advances automatically |
|
||||
| F4 schema_linking | `schema_linking.json` | `reviewer_schema_linking` with `advance:true` closes the phase itself (the curation IS the approval; `reviewer_confirm kind:"phase"` only as fallback if it reports an error). Promoted columns are the reviewer-approved OUTPUT columns — project exactly those in the final SELECT. |
|
||||
| F5 sintesi | — | `reviewer_confirm kind:"phase"` (after `tht session check`) |
|
||||
| F6 cte | `cte_plan.json`, `ctes/`, `cte_tests.json` | approve each CTE `kind:"cte_result"`, then `reviewer_confirm kind:"phase"` |
|
||||
| F7 sql_finale | `sql_final.sql` | `kind:"sql"` records `sql_approved`, then `reviewer_confirm kind:"phase"` |
|
||||
| F6 cte | `cte_plan.json`, `ctes/`, `cte_tests.json` | approve each CTE with `kind:"cte_result"`; approving the LAST CTE of the plan closes the phase automatically (`kind:"phase"` only as fallback if the auto-close reports an error) |
|
||||
| F7 sql_finale | `sql_final.sql` | `kind:"sql"` records `sql_approved` AND closes the phase automatically (`kind:"phase"` only as fallback if it reports an error) |
|
||||
| F8 datamart | — | auto: the `reviewer_memory_promote` gate advances F8 and finalizes the session itself (`reviewer_confirm kind:"phase"` only as fallback if it reports an error) |
|
||||
|
||||
## Disciplines (hold in every phase)
|
||||
@@ -51,17 +54,17 @@ because there the human interaction IS the phase approval: `rewrite_question` (F
|
||||
command (you invoke `tht ...` via the shell tool). NEVER run `tht phase advance`
|
||||
or `tht decision add` from the shell — they are blocked by the gate's anti-bypass
|
||||
hook; the gate extension records every decision via the `reviewer_*` tools.
|
||||
2. **The choice records; the phase gate advances.** A `reviewer_decide`, or a
|
||||
2. **The choice records; the closing gate advances.** A `reviewer_decide`, or a
|
||||
`reviewer_select` whose chosen option carries a `decision`, PERSISTS that decision — it
|
||||
does NOT by itself advance the phase. To move to the next phase you MUST issue
|
||||
`reviewer_confirm kind:"phase"` (for F7, first `kind:"sql"` to record `sql_approved`, then
|
||||
`kind:"phase"` to advance), the deliberate "this phase is
|
||||
done" gate. The `advance:true` flag on `reviewer_decide` is a shortcut that auto-advances
|
||||
ONLY F2 when the memory phase recorded nothing and F6 when it is skipped/empty; everywhere
|
||||
else it is a silent no-op, so never rely on it to advance. The self-closing gates are the
|
||||
three listed above (F3 `rewrite_question`, F4 `reviewer_schema_linking` `advance:true`,
|
||||
F8 `reviewer_memory_promote`) — after one of those, do NOT add a `reviewer_confirm` that
|
||||
merely echoes it; the phase is already closed.
|
||||
does NOT by itself advance the phase. In F1, F2-with-memories and F5 the phase closes
|
||||
with the deliberate `reviewer_confirm kind:"phase"` summary gate. The `advance:true`
|
||||
flag on `reviewer_decide` is a shortcut that auto-advances ONLY F2 when the memory phase
|
||||
recorded nothing and F6 when it is skipped/empty; everywhere else it is a silent no-op,
|
||||
so never rely on it to advance. The self-closing gates are the five listed above
|
||||
(F3 `rewrite_question`, F4 `reviewer_schema_linking` `advance:true`, F6 last
|
||||
`kind:"cte_result"`, F7 `kind:"sql"`, F8 `reviewer_memory_promote`) — after one of
|
||||
those, do NOT add a `reviewer_confirm kind:"phase"` that merely echoes it; the phase is
|
||||
already closed.
|
||||
3. **Single pick vs multi-answer.** For a single-pick clarification or decision, use
|
||||
`reviewer_select` and attach a `decision` payload (`{type, subject, detail?,
|
||||
rationale?}`) to each concrete option: picking it persists that decision directly —
|
||||
|
||||
Reference in New Issue
Block a user