diff --git a/apps/desktop/e2e/plan-clarification.spec.ts b/apps/desktop/e2e/plan-clarification.spec.ts new file mode 100644 index 00000000..4763adf2 --- /dev/null +++ b/apps/desktop/e2e/plan-clarification.spec.ts @@ -0,0 +1,139 @@ +import { test, expect } from "@playwright/test"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { currentRow, items, launch, seedHome, tid } from "./support"; + +/** + * A plan clarification (docs/task-context.md, CF-7 #280): the CLI reports a plan, asks a clarifying question + * through the question card, and carries on once it is answered. The answer reaches the CLI once, and Save + * to Notes records it with the secret hidden. A clarification nobody answered says so and gains no answer. + * The executable below is a scripted codex-cli 0.163.0 app-server (`turn/plan/updated`, then + * `item/tool/requestUserInput`), not a provider. + */ +function fixture(dir: string): string { + const cli = path.join(dir, "codex-clarify.cjs"); + fs.writeFileSync(cli, `#!${process.execPath} +const fs = require('node:fs'); +if (process.argv.includes('--version')) { console.log('codex-cli 0.163.0'); process.exit(0); } +if (process.argv.includes('login') || process.argv.includes('auth')) { console.log('Logged in using ChatGPT'); process.exit(0); } +const log = ${JSON.stringify(path.join(dir, "codex-replies.jsonl"))}; +const emit = (o) => process.stdout.write(JSON.stringify(o) + '\\n'); +const questions = [ + { id: 'q-env', header: 'Environment', question: 'Which environment should the plan target?', isOther: false, isSecret: false, options: [{ label: 'staging', description: 'Safe to try' }, { label: 'production', description: 'Live traffic' }] }, + { id: 'q-token', header: 'Token', question: 'Paste the deploy token', isOther: true, isSecret: true, options: null }, +]; +let turn = 0, open = null; +const cwds = new Map(); +const plan = (thread, id, a, b) => emit({ method: 'turn/plan/updated', params: { threadId: thread, turnId: id, explanation: null, plan: [{ step: 'Pick the environment', status: a }, { step: 'Deploy', status: b }] } }); +require('node:readline').createInterface({ input: process.stdin }).on('line', (line) => { + const msg = JSON.parse(line); + const p = msg.params || {}; + if (msg.method === 'initialize') return emit({ id: msg.id, result: { userAgent: 'codex-clarify/0.163.0' } }); + if (msg.method === 'initialized') return; + if (msg.method === 'account/read') return emit({ id: msg.id, result: { account: { type: 'chatgpt' }, requiresOpenaiAuth: true } }); + if (msg.method === 'model/list') return emit({ id: msg.id, result: { data: [{ id: 'fixture', model: 'fixture', displayName: 'Fixture', isDefault: true }], nextCursor: null } }); + if (msg.method === 'thread/start' || msg.method === 'thread/resume') { + const id = p.threadId || 'thr-' + cwds.size; + cwds.set(id, p.cwd || ''); + return emit({ id: msg.id, result: { thread: { id } } }); + } + if (msg.method === 'turn/start' && /modex-title-/.test(cwds.get(p.threadId) || '')) { + const id = 'title-' + (++turn); + emit({ id: msg.id, result: { turn: { id } } }); + emit({ method: 'item/completed', params: { threadId: p.threadId, turnId: id, item: { id: 'name-' + turn, type: 'agentMessage', text: 'Deploy plan', phase: 'final_answer' } } }); + return emit({ method: 'turn/completed', params: { threadId: p.threadId, turn: { id, status: 'completed', error: null } } }); + } + if (msg.method === 'turn/start') { + const id = 'turn-' + (++turn); + open = { thread: p.threadId, turn: id, request: 900 + turn, done: false }; + emit({ id: msg.id, result: { turn: { id } } }); + emit({ method: 'turn/started', params: { threadId: p.threadId, turn: { id } } }); + plan(p.threadId, id, 'inProgress', 'pending'); + emit({ id: open.request, method: 'item/tool/requestUserInput', params: { threadId: p.threadId, turnId: id, itemId: 'ask-' + turn, isBlocking: true, autoResolutionMs: null, questions } }); + return; + } + const finish = (status, text) => { + if (!open || open.done) return; + open.done = true; + if (text) emit({ method: 'item/completed', params: { threadId: open.thread, turnId: open.turn, item: { id: 'say-' + turn, type: 'agentMessage', text, phase: 'final_answer' } } }); + emit({ method: 'turn/completed', params: { threadId: open.thread, turn: { id: open.turn, status, error: null } } }); + }; + if (msg.method === 'turn/interrupt') { emit({ id: msg.id, result: {} }); return finish('interrupted'); } + if (open && msg.id === open.request && !msg.method) { + fs.appendFileSync(log, JSON.stringify({ request: msg.id, result: msg.result, error: msg.error }) + '\\n'); + if (msg.result) { + plan(open.thread, open.turn, 'completed', 'completed'); + finish('completed', 'Planned the deploy to ' + msg.result.answers['q-env'].answers[0] + '.'); + } + return; + } + if (msg.id !== undefined && msg.method) emit({ id: msg.id, error: { code: -32601, message: 'fixture does not implement ' + msg.method } }); +}); +`, { mode: 0o755 }); + return cli; +} + +type Reply = { request: number; result?: { answers: Record }; error?: { code: number } }; +const replies = (dir: string): Reply[] => { + const file = path.join(dir, "codex-replies.jsonl"); + return fs.existsSync(file) ? fs.readFileSync(file, "utf8").trim().split("\n").filter(Boolean).map((line) => JSON.parse(line) as Reply) : []; +}; +const everything = (dir: string): string => fs.readdirSync(dir, { recursive: true, withFileTypes: true }) + .filter((entry) => entry.isFile()).map((entry) => fs.readFileSync(path.join(entry.parentPath, entry.name), "utf8")).join("\n"); + +test("a plan clarification is answered on its card, reaches the CLI once, and is saved to Notes with the secret hidden; an unanswered one says so", async () => { + const { home, repo } = seedHome({ default_backend: "codex", default_mode: "agent" }); + const logs = fs.mkdtempSync(path.join(os.tmpdir(), "modex-e2e-clarify-")); + const statePath = path.join(home, "app/state.json"); + const state = JSON.parse(fs.readFileSync(statePath, "utf8")); + state.settings.codex_bin = fixture(logs); + fs.writeFileSync(statePath, JSON.stringify(state)); + const { app, page } = await launch(home); + try { + await page.keyboard.press("Meta+n"); + await tid(page, "composer-input").fill("Plan the deploy"); + await page.keyboard.press("Meta+Enter"); + // The plan as reported, then the clarification on the existing question card. + await expect(tid(items(page, "plan"), "plan-step")).toHaveText(["In progress Pick the environment", "Not started Deploy"]); + const card = items(page, "question").first(); + await expect(currentRow(page)).toHaveAttribute("data-status", "waiting"); + await card.getByRole("radio", { name: "staging" }).check(); + await card.getByLabel("Token").fill("s3cr3t-deploy-token"); + await tid(card, "question-submit").click(); + await expect(items(page, "assistant").last()).toContainText("Planned the deploy to staging."); + await expect(currentRow(page)).toHaveAttribute("data-status", "idle"); + await expect(tid(items(page, "plan"), "plan-step")).toHaveText(["Reported done Pick the environment", "Reported done Deploy"]); + expect(replies(logs).map((reply) => reply.result)).toEqual([{ answers: { "q-env": { answers: ["staging"] }, "q-token": { answers: ["s3cr3t-deploy-token"] } } }]); + + // Save to Notes: the clarification is in the record, the secret is not. + await tid(page, "save-task-note").last().click(); + await expect(tid(page, "task-note-saved").last()).toContainText("Saved to Notes · Task:"); + const notes = async () => (await page.evaluate(() => window.modex!.invoke("space:list", undefined))) as { title: string; markdown: string }[]; + const first = (await notes()).find((note) => note.title.startsWith("Task:"))!; + expect(first.markdown).toContain("## Clarifications"); + expect(first.markdown).toContain("- Which environment should the plan target? — staging"); + expect(first.markdown).toContain("- Paste the deploy token — (hidden)"); + expect(first.markdown).not.toContain("s3cr3t-deploy-token"); + + // A second turn asks again, and Stop closes the question unanswered: the CLI gets an error, the record no answer. + await tid(page, "composer-input").fill("Plan it again"); + await page.keyboard.press("Meta+Enter"); + await expect(tid(items(page, "question").nth(1), "question-submit")).toBeVisible(); + await tid(page, "stop").click(); + await expect(currentRow(page)).toHaveAttribute("data-status", "idle"); + await expect.poll(() => replies(logs).length).toBe(2); + expect(replies(logs)[1]).toMatchObject({ error: { code: -32000 } }); + await tid(page, "save-task-note").last().click(); + await expect(tid(page, "task-note-saved").last()).toContainText("Saved to Notes · Task:"); + const second = (await notes()).filter((note) => note.title.startsWith("Task:")).find((note) => note.markdown.includes("Plan it again"))!; + expect(second.markdown).toContain("- Which environment should the plan target? — not answered: the turn was stopped"); + expect(second.markdown).toMatch(/## Not settled\n[\s\S]*Paste the deploy token — not answered: the turn was stopped/); + expect(second.markdown).not.toContain("— staging"); + expect(replies(logs)).toHaveLength(2); + expect(everything(home).includes("s3cr3t-deploy-token")).toBe(false); + } finally { + await app.close(); + for (const dir of [home, repo, logs]) fs.rmSync(dir, { recursive: true, force: true }); + } +}); diff --git a/apps/desktop/src/shared/task-context.ts b/apps/desktop/src/shared/task-context.ts index e41d6dd9..fb9f04b5 100644 --- a/apps/desktop/src/shared/task-context.ts +++ b/apps/desktop/src/shared/task-context.ts @@ -20,9 +20,22 @@ export interface TaskRecord { /** Changed files outside the project: counted, never named. */ outsideFiles: number; commands: TaskCommand[]; + /** Each question the CLI asked during the turn, one entry per question on its cards. */ + clarifications: TaskClarification[]; answer?: string; } +/** A question the CLI asked, with the answer as its card shows it. Nothing is filled in that the card does not hold. */ +export interface TaskClarification { + question: string; + /** The answers as shown, saved and exported (a secret one is "(hidden)"), or null when nobody answered. */ + answer: string[] | null; + /** Why an unanswered question closed (docs/human-requests.md). */ + closed?: "native" | "stopped" | "ended"; + /** Answered, but Modex stopped before it could confirm the CLI received the answer. */ + uncertain?: true; +} + const PATCH_TOOLS = new Set(["apply_patch", "Edit", "MultiEdit", "Write", "NotebookEdit"]); const SETTLED_WELL = new Set(["succeeded"]); @@ -34,7 +47,14 @@ export function taskRecord(items: readonly ThreadItem[], opts: { threadId: strin const files = new Set(); let outsideFiles = 0; const commands: TaskCommand[] = []; + const clarifications: TaskClarification[] = []; for (const item of rest) { + if (item.kind === "question") { + for (const question of item.questions) { + const answer = item.answers ? item.answers[question.id] ?? null : null; + clarifications.push({ question: question.text, answer, ...(!answer && item.withdrawn ? { closed: item.withdrawn } : {}), ...(answer && item.delivery === "uncertain" ? { uncertain: true as const } : {}) }); + } + } if (item.kind !== "tool") continue; if (PATCH_TOOLS.has(item.name)) { for (const file of patchFiles(item)) { @@ -50,7 +70,7 @@ export function taskRecord(items: readonly ThreadItem[], opts: { threadId: strin return { title: opts.title, threadId: opts.threadId, request: user?.kind === "user" ? user.text : "", ...(plan?.kind === "plan" ? { plan: { steps: plan.steps, from: plan.from, revision: plan.revision } } : {}), - files: [...files], outsideFiles, commands, + files: [...files], outsideFiles, commands, clarifications, ...(answer?.kind === "assistant" && answer.text.trim() ? { answer: answer.text.trim() } : {}), }; } @@ -62,6 +82,8 @@ const FROM_WORDS: Record = { "codex-plan": "Codex's plan", " export function taskMarkdown(record: TaskRecord): string { const lines: string[] = [`# ${record.title}`, ""]; if (record.request.trim()) lines.push(...record.request.trim().split("\n").map((line) => `> ${line}`), ""); + const clarifications = record.clarifications ?? []; + if (clarifications.length) lines.push("## Clarifications", "", ...clarifications.map((c) => `- ${clarificationLine(c)}`), ""); if (record.plan) { lines.push("## Plan", ""); for (const step of record.plan.steps) lines.push(`- [${step.status === "completed" ? "x" : " "}] ${step.text} (${STEP_WORDS[step.status]})`); @@ -82,12 +104,20 @@ export function taskMarkdown(record: TaskRecord): string { const loose = [ ...record.commands.filter((command) => !SETTLED_WELL.has(command.outcome)).map(commandLine), ...(record.plan?.steps.filter((step) => step.status !== "completed").map((step) => `${step.text} (${STEP_WORDS[step.status]})`) ?? []), + ...clarifications.filter((c) => !c.answer || c.uncertain).map(clarificationLine), ]; if (loose.length) lines.push("## Not settled", "", ...loose.map((line) => `- ${line}`), ""); lines.push("---", `From modex thread ${record.threadId}.`); return lines.join("\n") + "\n"; } +const CLOSED_WORDS: Record, string> = { native: "the agent withdrew it", stopped: "the turn was stopped", ended: "the turn ended first" }; + +function clarificationLine(c: TaskClarification): string { + if (!c.answer) return `${c.question} — not answered${c.closed ? `: ${CLOSED_WORDS[c.closed]}` : ""}`; + return `${c.question} — ${c.answer.join(", ")}${c.uncertain ? " (Modex could not confirm the agent received this answer)" : ""}`; +} + function commandLine(command: TaskCommand): string { const words: Record = { running: "running", succeeded: "succeeded", failed: "failed", denied: "denied", interrupted: "stopped", unknown: "status unknown" }; const exit = typeof command.exitCode === "number" ? `exit code ${command.exitCode}` : command.exitCode === null ? "no exit code" : "exit code not reported"; diff --git a/apps/desktop/test/task-context.test.ts b/apps/desktop/test/task-context.test.ts index 204b8c06..cb8b53c0 100644 --- a/apps/desktop/test/task-context.test.ts +++ b/apps/desktop/test/task-context.test.ts @@ -118,6 +118,37 @@ test("a task record summarises request, plan, files, checks and loose ends, with for (const absent of ["PRIVATE REASONING", "LOTS OF LOG", "/Users/val", "verified", "Verified"]) assert.ok(!md.includes(absent), `must not include: ${absent}`); }); +test("a task record lists each clarification with its answer, a secret hidden, and says when one was not answered or not confirmed; it invents none", () => { + const at = "2026-10-18T09:00:00.000Z"; + const q = (id: string, header: string, text: string, secret = false) => ({ id, header, text, options: null, allowOther: true, secret }); + const items: ThreadItem[] = [ + { id: "u1", kind: "user", text: "Deploy the fix", at }, + { id: "ask-1", kind: "question", blocking: true, questions: [q("env", "Environment", "Which environment?"), q("token", "Token", "Paste the deploy token", true)], + answers: { env: ["staging"], token: ["(hidden)"] }, answeredBy: "desktop", delivery: "sent", at }, + { id: "ask-2", kind: "question", blocking: true, questions: [q("branch", "Branch", "Which branch?")], withdrawn: "stopped", at }, + { id: "ask-3", kind: "question", blocking: true, questions: [q("notes", "Notes", "Add release notes?")], answers: { notes: ["yes"] }, answeredBy: "phone", delivery: "uncertain", at }, + { id: "ask-4", kind: "question", blocking: false, questions: [q("region", "Region", "Which region?")], withdrawn: "native", at }, + { id: "f", kind: "assistant", text: "Deployed to staging.", phase: "final", outcome: "succeeded", at }, + ] as ThreadItem[]; + const record = taskRecord(items, { threadId: "T", title: "Deploy", cwd: "/repo" }); + assert.deepEqual(record.clarifications, [ + { question: "Which environment?", answer: ["staging"] }, + { question: "Paste the deploy token", answer: ["(hidden)"] }, + { question: "Which branch?", answer: null, closed: "stopped" }, + { question: "Add release notes?", answer: ["yes"], uncertain: true }, + { question: "Which region?", answer: null, closed: "native" }, + ]); + const md = taskMarkdown(record); + for (const expected of ["## Clarifications", "- Which environment? — staging", "- Paste the deploy token — (hidden)", "- Which branch? — not answered: the turn was stopped", + "- Add release notes? — yes (Modex could not confirm the agent received this answer)", "- Which region? — not answered: the agent withdrew it"]) { + assert.ok(md.includes(expected), `missing: ${expected}\n${md}`); + } + assert.match(md, /## Not settled\n[\s\S]*Which branch\? — not answered[\s\S]*Add release notes\?/); + assert.ok(!/Which environment\?[^\n]*\n[\s\S]*## Not settled[\s\S]*Which environment/.test(md), "an answered, sent clarification is settled"); + // A turn with no questions has no section, and old records read as before. + assert.equal(taskMarkdown(taskRecord(items.filter((i) => i.kind !== "question"), { threadId: "T", title: "Deploy", cwd: "/repo" })).includes("Clarifications"), false); +}); + test("Save task to Notes writes the turn's record into its project's notes; saving again updates it, and a note edited since is kept", async () => { const { Store } = await import("../src/main/engine/store.js"); const { ThreadRunner } = await import("../src/main/engine/runner.js"); diff --git a/docs/status.md b/docs/status.md index 22a84dbb..b98a1ed7 100644 --- a/docs/status.md +++ b/docs/status.md @@ -114,8 +114,11 @@ and [landing verification](reviews/2026-10-11-landing-clarity.md). - **Fixes those checks found.** Opening a thread for the first time no longer resends and rewrites every item in it, which cost a 1,400-item transcript about 55 ms. An approval card in a narrow window wraps its text and buttons instead of pushing Deny out of view. - - **Still open:** CF-4's remaining Markdown checks (#277), CF-7's clarifications, live-provider - acceptance and human VoiceOver checks. + - **Clarifications in the task record (CF-7 #280, second part).** A question Codex asks while it + plans is answered on its card, and the turn's record and Save to Notes list it with its answer, + a secret hidden. One nobody answered reads "not answered" with why, and is never filled in. + - **Still open:** CF-4's remaining Markdown checks (#277), live-provider acceptance and human + VoiceOver checks. - **Every inline error can be copied, and most can be retried.** A shared `ErrorNotice` replaces bare error text across the terminal, Changes diff, Files, Canvas, browser and diff --git a/docs/task-context.md b/docs/task-context.md index 503f4ea9..e5660251 100644 --- a/docs/task-context.md +++ b/docs/task-context.md @@ -28,14 +28,32 @@ outcome is the turn's. ## The task record -`taskRecord` (`shared/task-context.ts`) summarises one turn: the request, the latest plan, the files -its patch tools changed, the commands it ran with outcome and exit code, the final answer, and what -is not settled. Not settled means commands that did not succeed and plan steps not reported done. +`taskRecord` (`shared/task-context.ts`) summarises one turn: the request, the questions the CLI asked +and their answers, the latest plan, the files its patch tools changed, the commands it ran with +outcome and exit code, the final answer, and what is not settled. Not settled means commands that did +not succeed, plan steps not reported done, and clarifications nobody answered or whose answer Modex +could not confirm the CLI received. `taskMarkdown` renders the record as portable Markdown that ends with a backlink to the thread. It leaves out reasoning, command output, and any path outside the project. Files inside the project are listed relative to the thread's working directory; files outside it are counted, not named. +## Clarifications + +A CLI that needs to ask before it plans or acts asks through the [question card](human-requests.md): +Codex's `item/tool/requestUserInput`, with a control for each question. The answer reaches the CLI +once, and the plan carries on from it. Claude Code 2.1.302 sends Modex no structured questions. + +The record lists every question the turn asked, in order, with the answer as its card shows it: + +- an answered question gives its answer, and a secret one reads "(hidden)"; +- an unanswered one reads "not answered", with why it closed: the agent withdrew it, the turn was + stopped, or the turn ended first; +- an answer Modex could not confirm the CLI received says so (see + [across replay and reconnect](human-requests.md#across-replay-and-reconnect)). + +Nothing is filled in that the card does not hold. + ## Save to Notes Under each finished turn, **Save to Notes** (`task:save-note`, `ThreadRunner.saveTaskNote`) writes the @@ -51,8 +69,6 @@ saves on its own, and an agent cannot use this. ## Not done yet -- Clarification through question cards waits for the human-request lifecycle - ([#276](https://github.com/TypeSafeAI/modex/issues/276)). - Verified steps, and marking earlier verification stale. Verification needs a check's observed result tied to the code revision the step changed. - An on-demand context map of relevant files and tests beyond those the turn changed.