diff --git a/docs/probe-teach-split.md b/docs/probe-teach-split.md index d1b1c96..7f54530 100644 --- a/docs/probe-teach-split.md +++ b/docs/probe-teach-split.md @@ -21,7 +21,8 @@ philosophy so they share a view of the whole process). field it must not use. No plan tools, no jobs, no demos. - **The teacher** runs everything from planning onward: full tool set (teach quiz with nodeId, plan_init, jobs, demos). Constructed the moment the - transcript carries a `probe_done`. + transcript carries a concluded `probe_done`: a non-failed/non-aborted + assistant call followed by a matching successful tool result. - **The swap happens between probing and planning** — the planning phase needs full knowledge of how teaching is done, so the teacher agent starts at planning. @@ -43,6 +44,12 @@ The prober concludes explicitly: - **Refuses while a question is open** ("resolve the open question first") — same doctrine as nodeId validation. - **Refuses a second conclusion** (idempotent: the probe concludes once). + Conclusion requires a later `probe_done` result matching the call's ID + and tool name. The calling generation must not have failed or aborted; + the result must not be `isError` or explicitly contain `details.ok:false`. + Clean acknowledgments without an explicit `ok:true` remain valid. Bare, + unmatched, errored, or refused calls keep probing open; saved attempts + are never deleted or replay-executed by this detection rule. - The assessment is the tool call's input, so it streams to the learner live and is durable in the transcript by the tool result itself. A dedicated assessment card/viewer comes later (UI). @@ -53,7 +60,7 @@ The prober concludes explicitly: - **Side effect: it queues the handoff.** When the prober's turn ends, the wake pipeline (the same machinery that delivers sub-agent results and abort steers) fires a server-side turn with a handoff steer — the teacher - is constructed (probe_done is in the transcript) and authors the plan. + is constructed (probe_done has successfully concluded) and authors the plan. The learner experiences assessment → plan as one continuous flow. If the wake is lost (crash window), the next client message still constructs the teacher — the transcript is the source of truth. diff --git a/src/agent/tools/probe.ts b/src/agent/tools/probe.ts index 11e3cd4..7cd9b84 100644 --- a/src/agent/tools/probe.ts +++ b/src/agent/tools/probe.ts @@ -42,8 +42,9 @@ function probeConcluded(messages: readonly LhMessage[]): boolean { * live), so the agent sees the fixed PROBE_DONE_ACK_TEXT — never an * echo. details = {ok:true} only. * - * The first probe_done in the transcript is also the construction seam: - * from that message on, every request builds the TEACHER agent. + * The first concluded probe_done (non-failed call plus matching successful + * result) is also the construction seam: from that result on, every request + * builds the TEACHER agent. Bare calls and refused results keep probing. */ export function probeDoneTool(ctx: ProbeDoneContext): Record> { return { diff --git a/src/core/native-messages.ts b/src/core/native-messages.ts index 60f51ce..c9d5a46 100644 --- a/src/core/native-messages.ts +++ b/src/core/native-messages.ts @@ -669,15 +669,35 @@ export function danglingQuestionOf( return null; } -/** The probe→teach construction seam (chat-handler): a probe_done call in - * the transcript — any state — means the probe is concluded: teach. */ -export function hasProbeDone(messages: readonly LhMessage[]): boolean { +/** Successful probe_done result MESSAGE ids, in transcript order. A call + * from a failed/aborted generation grants no completion authority. Its + * later result must match both call id and tool name, must not be an + * error, and must not explicitly refuse with details.ok:false. Older + * clean acknowledgments without ok:true still count. No history changes. */ +export function probeDoneResultIds(messages: readonly LhMessage[]): Set { + const calls = new Set(); + const results = new Set(); for (const m of messages) { - if ((m as { role?: string }).role !== "assistant") continue; - const assistant = m as AssistantMessage; - if (assistant.content.some((b) => b.type === "toolCall" && b.name === "probe_done")) return true; + if (m.role === "assistant") { + if (m.stopReason === "error" || m.stopReason === "aborted") continue; + for (const block of m.content) { + if (block.type === "toolCall" && block.name === "probe_done") calls.add(block.id); + } + } else if ( + m.role === "toolResult" && m.toolName === "probe_done" && + calls.has(m.toolCallId) && !m.isError && + (m.details as { ok?: unknown } | null | undefined)?.ok !== false + ) { + results.add(m.id); + } } - return false; + return results; +} + +/** The probe→teach construction seam (chat-handler and the tool guard): + * only a concluded call plus a matching successful result means teach. */ +export function hasProbeDone(messages: readonly LhMessage[]): boolean { + return probeDoneResultIds(messages).size > 0; } /** Every toolResult id in the record — answer-event validation (the call diff --git a/src/core/phases.ts b/src/core/phases.ts index 4a6e0f4..0ecd870 100644 --- a/src/core/phases.ts +++ b/src/core/phases.ts @@ -33,7 +33,7 @@ * Both are strictly sequential; the two derivations agree on every settled * record (a call's part projects into the same view boundary). * - * Pure: no imports, no runtime deps — safe on server, client, and tests. + * Pure: only browser-safe core helpers — safe on server, client, and tests. */ /** The three phases of a session, as the picker / header / dividers see them. */ @@ -80,7 +80,10 @@ function advance( // plan → teach only via the first nodeId-tagged quiz CALL (the call, // not its answer — an errored-and-retried call still proves teaching // began, and the retry lands in the same group anyway). - if (phase === "probe" && part.type === "tool-probe_done" && part.state === "output-available") { + if ( + phase === "probe" && part.type === "tool-probe_done" && part.state === "output-available" && + (part.output as { ok?: unknown } | null | undefined)?.ok !== false + ) { phase = "plan"; } // plan → teach via the first nodeId-tagged quizzing CALL — quiz OR @@ -153,7 +156,7 @@ export function phaseTimeline(messages: readonly PhaseMessage[]): Phase3[] { // ─── the native fold (Wave D: record-side phase over LhMessage[]) ─────────── -import type { LhMessage } from "./native-messages.ts"; +import { probeDoneResultIds, type LhMessage } from "./native-messages.ts"; /** The phase at `upToIndex` (inclusive; default: the last message) of a * NATIVE record. probe→plan advances at the probe_done call's clean @@ -163,18 +166,12 @@ import type { LhMessage } from "./native-messages.ts"; export function phaseAt3Lh(messages: readonly LhMessage[], upToIndex?: number): Phase3 { let phase: Phase3 = "probe"; const last = Math.min(upToIndex ?? messages.length - 1, messages.length - 1); + const concluded = probeDoneResultIds(messages.slice(0, last + 1)); for (let i = 0; i <= last; i++) { const m = messages[i]!; const role = (m as { role?: string }).role; if (role === "toolResult") { - const result = m as { toolName?: string; isError?: boolean }; - if ( - phase === "probe" && - result.toolName === "probe_done" && - !result.isError - ) { - phase = "plan"; - } + if (phase === "probe" && concluded.has(m.id)) phase = "plan"; } else if (role === "assistant") { const content = (m as { content?: ReadonlyArray<{ type?: string; name?: string; arguments?: unknown }> }).content ?? []; if (phase === "plan") { diff --git a/tests/phases.test.ts b/tests/phases.test.ts index ad03da0..78ed427 100644 --- a/tests/phases.test.ts +++ b/tests/phases.test.ts @@ -103,6 +103,16 @@ test("phaseAt3: a mere probe_done CALL (input-available, not yet concluded) keep assert.equal(still, "probe"); }); +test("phaseAt3 and phaseTimeline: explicit probe refusals never advance the UI phase", () => { + const refused = assistantTool("probe_done", "output-available", { ok: false, error: "question open" }); + const errored = assistantTool("probe_done", "output-error", { ok: true }); + const tagged = assistantQuiz({ nodeId: "n1" }); + const clean = assistantTool("probe_done", "output-available", { ok: true }); + const messages = [refused, errored, tagged, clean, tagged]; + assert.equal(phaseAt3(messages, 2), "probe"); + assert.deepEqual(phaseTimeline(messages), ["probe", "probe", "probe", "plan", "teach"]); +}); + test("phaseAt3: legacy transcripts without probe_done read as probe (no backwards compat — data wiped)", () => { const legacy = phaseAt3([user("teach me lacan"), assistantTool("plan_init", "output-available", PLAN_OK)]); assert.equal(legacy, "probe", "plan_init alone never flips a phase post-split"); @@ -158,23 +168,31 @@ function assistantQuizCall(arguments_: Record): LhMessage { } as unknown as LhMessage; } +function assistantProbeCall(): LhMessage { + return { + ...assistantQuizCall({}), + content: [{ type: "toolCall", id: "call_probe_done", name: "probe_done", arguments: {} }], + } as LhMessage; +} + test("phaseAt3Lh (native fold): probe→plan at the clean probe_done result; plan→teach at the first tagged quiz CALL", () => { const done = toolResult("probe_done"); + const call = assistantProbeCall(); const tagged = assistantQuizCall({ question: "q", nodeId: "gravity-basics", correctAnswer: "a" }); const untagged = assistantQuizCall({ question: "q", correctAnswer: "a" }); - assert.equal(phaseAt3Lh([done, untagged]), "plan", "an untagged quiz call never starts teaching"); - assert.equal(phaseAt3Lh([done, tagged]), "teach", "the tagged quiz CALL is the seam"); + assert.equal(phaseAt3Lh([call, done, untagged]), "plan", "an untagged quiz call never starts teaching"); + assert.equal(phaseAt3Lh([call, done, tagged]), "teach", "the tagged quiz CALL is the seam"); assert.equal(phaseAt3Lh([tagged]), "probe", "the chain never skips (tag before probe_done)"); assert.equal( - phaseAt3Lh([toolResult("probe_done", true), tagged]), + phaseAt3Lh([call, toolResult("probe_done", true), tagged]), "probe", "an ERRORED probe_done concludes nothing", ); // The two folds agree on the same settled transcript (the view projection // of the tagged call carries its arguments as the part input). assert.equal( - phaseAt3Lh([done, tagged]), + phaseAt3Lh([call, done, tagged]), phaseAt3([ { parts: [{ type: "tool-probe_done", state: "output-available" } as never] }, { parts: [{ type: "tool-quiz", input: { nodeId: "gravity-basics" } } as never] }, @@ -188,6 +206,7 @@ test("phaseAt3Lh (native fold): probe→plan at the clean probe_done result; pla test("phaseAt3 + phaseAt3Lh: a tagged explain CALL is the plan→teach seam too", () => { const done = toolResult("probe_done"); + const call = assistantProbeCall(); const explainCall = (args: Record): LhMessage => ({ id: "a_explain", @@ -199,10 +218,10 @@ test("phaseAt3 + phaseAt3Lh: a tagged explain CALL is the plan→teach seam too" const tagged = explainCall({ question: "q", correctAnswer: "a", explanation: "e", nodeId: "gravity-basics" }); const untagged = explainCall({ question: "q", correctAnswer: "a", explanation: "e" }); - assert.equal(phaseAt3Lh([done, untagged]), "plan", "an untagged explain never starts teaching"); - assert.equal(phaseAt3Lh([done, tagged]), "teach", "the tagged explain CALL is the seam"); + assert.equal(phaseAt3Lh([call, done, untagged]), "plan", "an untagged explain never starts teaching"); + assert.equal(phaseAt3Lh([call, done, tagged]), "teach", "the tagged explain CALL is the seam"); assert.equal( - phaseAt3Lh([done, tagged]), + phaseAt3Lh([call, done, tagged]), phaseAt3([ { parts: [{ type: "tool-probe_done", state: "output-available" } as never] }, { parts: [{ type: "tool-explain", input: { nodeId: "gravity-basics" } } as never] }, diff --git a/tests/probe-done.test.ts b/tests/probe-done.test.ts index c880e18..0d7d5fe 100644 --- a/tests/probe-done.test.ts +++ b/tests/probe-done.test.ts @@ -13,12 +13,13 @@ import * as path from "node:path"; import { after, describe, it } from "node:test"; import { probeDoneTool } from "../src/agent/tools/probe.ts"; import type { SteerMessage } from "../src/core/contracts.ts"; -import type { LhMessage } from "../src/core/native-messages.ts"; +import { hasProbeDone, type LhMessage } from "../src/core/native-messages.ts"; +import { phaseAt3Lh } from "../src/core/phases.ts"; import { HANDOFF_TEXT } from "../src/core/contracts.ts"; import { chatStore } from "../src/store/chats.ts"; import { PATHS } from "../config.ts"; -const dataDir = path.join(PATHS.root, "data"); +const dataDir = PATHS.data; fs.mkdirSync(dataDir, { recursive: true }); const createdChats: string[] = []; @@ -142,3 +143,61 @@ describe("probe_done: the probe→planning seam", () => { assert.equal(res.ok, true, "ok:true even when the queue is lost — the next client message constructs the teacher off the transcript seam"); }); }); + +/** A real record result, distinct from the calling assistant message. */ +function probeResult(overrides: Partial> = {}): LhMessage { + return { + id: "probe-result", role: "toolResult", toolCallId: "call_probe", toolName: "probe_done", + content: [{ type: "text", text: "ok" }], isError: false, timestamp: 1, + ...overrides, + }; +} + +const probeCall = () => assistantWithCall("probe", "probe_done", { assessment: ASSESSMENT }); +const failedProbeCall = (stopReason: "error" | "aborted"): LhMessage => ({ ...probeCall(), stopReason } as LhMessage); +const incompleteProbes: Array<[string, () => LhMessage[]]> = [ + ["unmatched call", () => [probeCall()]], + ["errored result", () => [probeCall(), probeResult({ isError: true })]], + ["explicit refusal without isError", () => [probeCall(), probeResult({ details: { ok: false, error: "refused" } })]], + ["result without a call", () => [probeResult()]], + ["wrong call id", () => [probeCall(), probeResult({ toolCallId: "other" })]], + ["wrong result tool name", () => [probeCall(), probeResult({ toolName: "plan_init" })]], + ["wrong call tool name", () => [assistantWithCall("probe", "plan_init", {}), probeResult()]], + ["result before the call", () => [probeResult(), probeCall()]], + ["failed calling generation", () => [failedProbeCall("error"), probeResult({ details: { ok: true } })]], + ["aborted calling generation", () => [failedProbeCall("aborted"), probeResult({ details: { ok: true } })]], +]; + +describe("probe completion: matched successful results only (PR130 foundation)", () => { + for (const [name, messages] of incompleteProbes) { + it(`${name} keeps probing open and allows a fresh conclusion`, async () => { + const chatId = newChat(`Probe recovery: ${name}`); + const before = messages(); + const snapshot = structuredClone(before); + chatStore.saveMessages(chatId, before); + assert.equal(hasProbeDone(before), false); + assert.equal(phaseAt3Lh(before), "probe"); + const delivered: SteerMessage[] = []; + const tools = probeDoneTool({ chatId, chatStore, deliver: (s) => delivered.push(s) }); + assert.equal((await execute(tools, { assessment: ASSESSMENT })).ok, true); + assert.equal(delivered.length, 1, "only the fresh invocation queues a handoff"); + assert.deepEqual(before, snapshot, "the scans never mutate history"); + assert.deepEqual(chatStore.getChat(chatId)!.messages, snapshot, "the tool does not delete or replay saved attempts"); + }); + } + + for (const [name, result] of [ + ["current acknowledgment", probeResult({ details: { ok: true } })], + ["older clean acknowledgment without details", probeResult()], + ["older clean acknowledgment without ok", probeResult({ details: { assessment: ASSESSMENT } })], + ] as const) { + it(`${name} concludes exactly at the matching result`, () => { + const messages = [probeCall(), result]; + const before = structuredClone(messages); + assert.equal(hasProbeDone(messages), true); + assert.equal(phaseAt3Lh(messages, 0), "probe", "a future result cannot conclude an earlier prefix"); + assert.equal(phaseAt3Lh(messages, 1), "plan"); + assert.deepEqual(messages, before); + }); + } +}); diff --git a/tests/retry-failed-turn.test.ts b/tests/retry-failed-turn.test.ts index cdc364b..01edeb5 100644 --- a/tests/retry-failed-turn.test.ts +++ b/tests/retry-failed-turn.test.ts @@ -17,6 +17,7 @@ import type { AgentBuildOptions, TeachingAgent } from "../src/agent/teacher-agen import type { PiEngine } from "../src/agent/models.ts"; import type { LhMessage } from "../src/core/native-messages.ts"; import type { QuestionData } from "../src/core/question-core.ts"; +import type { SteerMessage } from "../src/core/contracts.ts"; // Set the storage/auth seams BEFORE any app/config-loading imports. assert.equal(process.env.NODE_ENV, "test", "Run with NODE_ENV=test; these tests must not invoke real providers."); @@ -36,7 +37,8 @@ const { jobStore } = await import("../src/store/jobs.ts"); const { subscribe } = await import("../src/core/events.ts"); const { buildAnswer, normalizeOptions, resolveCorrect } = await import("../src/core/question-core.ts"); const { ChatInputError, endsInFailedTurn, failedResponseAtEnd, inputAppends, parseChatInput, retryDrops } = await import("../src/core/input-events.ts"); -const { INTERRUPTED_TEXT, asAgentMessages, newHandoffMessage, newPlanBlockMessage, newUserMessage, reconstructRequestCause, toLlmMessages } = await import("../src/core/native-messages.ts"); +const { INTERRUPTED_TEXT, asAgentMessages, hasProbeDone, newHandoffMessage, newPlanBlockMessage, newUserMessage, reconstructRequestCause, toLlmMessages } = await import("../src/core/native-messages.ts"); +const { phaseAt3Lh } = await import("../src/core/phases.ts"); const { newCompactionMarker } = await import("../src/core/compaction.ts"); after(() => { @@ -287,6 +289,47 @@ describe("failed-response Retry (issue #110)", () => { assert.deepEqual(contexts.at(-1), ["user"]); }); + it("a retained failed probe retries with the prober, concludes once, then selects the teacher", async () => { + const input = { assessment: { strands: [{ strand: "planet order", assessment: "Names known; ordering needs work." }] } }; + const { engine, histories } = scripted([ + () => fauxAssistantMessage([fauxToolCall("probe_done", input, { id: "failed-probe" })], { stopReason: "error", errorMessage: "cut off" }), + () => fauxAssistantMessage([fauxToolCall("probe_done", input, { id: "fresh-probe" })], { stopReason: "toolUse" }), + () => fauxAssistantMessage("Assessment ready."), + () => fauxAssistantMessage("Now planning the lesson."), + ]); + const chat = createConfiguredChat("Test: failed probe recovery"); + const delivered: SteerMessage[] = []; + const selections: boolean[] = []; + const factory: AgentFactory = (opts) => { + selections.push(opts.teaching); + if (opts.teaching) return createTeacherAgent(undefined, { ...opts, engine }); + return createProberAgent({ + chatId: chat.id, chatStore, jobStore, deliver: (s) => delivered.push(s), spawn: () => {}, + }, { ...opts, engine }); + }; + await turn(engine, chat.id, { type: "text", text: "teach me planet order" }, factory); + const before = chatStore.getChat(chat.id)!.messages; + assert.ok(endsInFailedTurn(before)); + assert.equal(hasProbeDone(before), false); + assert.equal(phaseAt3Lh(before), "probe"); + assert.equal(delivered.length, 0, "the stored failed call never ran"); + + assert.equal((await turn(engine, chat.id, { type: "retry" }, factory)).status, 200); + const recovered = chatStore.getChat(chat.id)!.messages; + assertPreserved(before, recovered); + assert.deepEqual(selections, [false, false], "Retry must still build the prober"); + assert.equal(hasProbeDone(recovered), true); + assert.equal(phaseAt3Lh(recovered), "plan"); + assert.equal(delivered.length, 1, "the full retained-store guard accepts the fresh conclusion"); + assert.deepEqual(recovered.filter((m) => m.role === "toolResult").map((m) => m.role === "toolResult" && m.toolCallId), ["fresh-probe"]); + assert.doesNotMatch(histories[1]!, /failed-probe/, "the failed call is omitted only from temporary Retry history"); + + assert.equal((await turn(engine, chat.id, { type: "text", text: "go on" }, factory)).status, 200); + assert.deepEqual(selections, [false, false, true]); + assert.equal(delivered.length, 1, "later turns do not replay either saved call"); + assertPreserved(recovered, chatStore.getChat(chat.id)!.messages); + }); + for (const kind of ["prober", "teacher", "practice"] as const) { it(`uses the same retained-history retry boundary for the ${kind}`, async () => { const { engine, contexts } = scripted([() => fauxAssistantMessage("Recovered response.")]);