From 712da73ebf2403081d8864ae3bfe529e4025e5ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Val=20Alexander=20=F0=9F=91=91?= Date: Mon, 2 Nov 2026 18:28:39 -0600 Subject: [PATCH 1/3] test(conversation): protect native question drafts and YOLO boundaries --- apps/desktop/e2e/human-requests.spec.ts | 75 ++++++++++++++++++- .../test/human-requests-runner.test.ts | 26 +++++++ .../2026-11-01-cf3-question-regressions.md | 49 ++++++++++++ docs/status.md | 2 + 4 files changed, 150 insertions(+), 2 deletions(-) create mode 100644 docs/reviews/2026-11-01-cf3-question-regressions.md diff --git a/apps/desktop/e2e/human-requests.spec.ts b/apps/desktop/e2e/human-requests.spec.ts index cb19d474..c5035e44 100644 --- a/apps/desktop/e2e/human-requests.spec.ts +++ b/apps/desktop/e2e/human-requests.spec.ts @@ -10,13 +10,14 @@ import { launch, seedHome, tid, items } from "./support"; // It is a scripted fixture, not live-model evidence. Its log lives outside MODEX_HOME so the secret check // below can read every file Modex wrote. -function fixture(dir: string): string { +function fixture(dir: string, questionGate?: string): string { const cli = path.join(dir, "codex-questions.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 questionGate = ${JSON.stringify(questionGate ?? null)}; const emit = (o) => process.stdout.write(JSON.stringify(o) + '\\n'); const questions = [ { id: 'q-env', header: 'Environment', question: 'Which environment?', isOther: false, isSecret: false, options: [{ label: 'staging', description: 'Safe to try' }, { label: 'production', description: 'Live traffic' }] }, @@ -49,7 +50,14 @@ require('node:readline').createInterface({ input: process.stdin }).on('line', (l open = { thread: p.threadId, turn: id, request: 900 + (++asked), done: false }; emit({ id: msg.id, result: { turn: { id } } }); emit({ method: 'turn/started', params: { threadId: p.threadId, turn: { id } } }); - emit({ id: open.request, method: 'item/tool/requestUserInput', params: { threadId: p.threadId, turnId: id, itemId: 'ask-' + asked, isBlocking: true, autoResolutionMs: null, questions } }); + const request = { id: open.request, method: 'item/tool/requestUserInput', params: { threadId: p.threadId, turnId: id, itemId: 'ask-' + asked, isBlocking: true, autoResolutionMs: null, questions } }; + if (questionGate) { + const pending = open; + const timer = setInterval(() => { + if (pending.done || open !== pending) return clearInterval(timer); + if (fs.existsSync(questionGate)) { clearInterval(timer); emit(request); } + }, 10); + } else emit(request); return; } const finish = (status, text) => { @@ -87,6 +95,69 @@ async function send(page: Page, text: string): Promise { } const currentRow = (page: Page) => page.locator('[data-testid="thread-row"][aria-current="true"]'); +test("a native question preserves the existing composer draft, staged attachment and arrival focus", async () => { + const { home } = seedHome({ default_backend: "codex", default_mode: "agent" }); + const logs = fs.mkdtempSync(path.join(os.tmpdir(), "modex-e2e-question-draft-")); + const gate = path.join(logs, "ask-now"); + const statePath = path.join(home, "app/state.json"); + const state = JSON.parse(fs.readFileSync(statePath, "utf8")); + state.settings.codex_bin = fixture(logs, gate); + fs.writeFileSync(statePath, JSON.stringify(state)); + let app: ElectronApplication | undefined; + try { + const opened = await launch(home); app = opened.app; + const page = opened.page; + const threadId = await page.evaluate(async () => { + const state = await window.modex!.invoke("state:get", undefined); + return (await window.modex!.invoke("thread:create", { projectId: state.projects[0]!.id, backend: "codex", mode: "agent", auto: false })).id; + }); + // Direct creation returns to its caller; reload to hydrate the renderer from main. + await page.reload(); + await page.locator(`[data-testid="thread-row"][data-thread-id="${threadId}"]`).click(); + await expect(currentRow(page)).toHaveAttribute("data-status", "idle"); + const draft = " keep this draft\nwith whitespace 😀 "; + const composer = tid(page, "composer-input"); + await composer.fill(draft); + await page.evaluate(() => { + const data = new DataTransfer(); + data.items.add(new File(["question draft attachment bytes"], "draft.txt", { type: "text/plain" })); + document.querySelector('[data-testid="composer-input"]')!.dispatchEvent(new ClipboardEvent("paste", { clipboardData: data, bubbles: true, cancelable: true })); + }); + await expect(tid(page, "composer-attachments")).toContainText("draft.txt"); + const recovery = () => JSON.parse(fs.readFileSync(path.join(home, "app/update-recovery.json"), "utf8")).drafts.threads.find((row: { threadId: string }) => row.threadId === threadId); + await expect.poll(() => recovery()?.attachments.length).toBe(1); + const saved = recovery(); + expect(saved.text).toBe(draft); + await composer.focus(); + // A turn arriving through main must not consume the window's separate unsent draft. + expect(await page.evaluate(threadId => window.modex!.invoke("thread:send", { threadId, text: "deploy it" }), threadId)).toEqual({ ok: true }); + await expect(currentRow(page)).toHaveAttribute("data-status", "running"); + fs.writeFileSync(gate, "ready"); + const card = items(page, "question").first(); + await expect(tid(card, "question")).toHaveCount(2); + await expect(composer).toBeFocused(); + await expect(composer).toHaveValue(draft); + expect(recovery()).toEqual(saved); + expect(replies(logs)).toEqual([]); + await card.getByRole("radio", { name: "staging" }).check(); + await card.getByLabel("Token").fill("question-only-secret"); + await card.getByLabel("Token").press("Enter"); + await expect(currentRow(page)).toHaveAttribute("data-status", "idle"); + await expect(composer).toHaveValue(draft); + await expect(tid(page, "composer-attachments")).toContainText("draft.txt"); + expect(recovery()).toEqual(saved); + await page.reload(); + await expect(composer).toHaveValue(draft); + await expect(tid(page, "composer-attachments")).toContainText("draft.txt"); + expect(recovery()).toEqual(saved); + expect(fs.readFileSync(path.join(home, "attachments", saved.attachments[0].rel), "utf8")).toBe("question draft attachment bytes"); + const transcript = JSON.parse(fs.readFileSync(path.join(home, "app/threads", `${threadId}.json`), "utf8")); + expect(transcript.filter((item: { kind: string }) => item.kind === "user").map((item: { text: string }) => item.text)).toEqual(["deploy it"]); + expect(replies(logs)).toHaveLength(1); + expect(everything(path.join(home, "app"))).not.toContain("question-only-secret"); + } finally { await app?.close(); fs.rmSync(logs, { recursive: true, force: true }); } +}); + test("Codex questions: the card answers by its schema, the CLI hears once, the secret stays out, Stop withdraws", async () => { const { home } = seedHome({ default_backend: "codex", default_mode: "agent" }); const logs = fs.mkdtempSync(path.join(os.tmpdir(), "modex-e2e-codex-questions-")); diff --git a/apps/desktop/test/human-requests-runner.test.ts b/apps/desktop/test/human-requests-runner.test.ts index 28a6fea9..5dfd65d9 100644 --- a/apps/desktop/test/human-requests-runner.test.ts +++ b/apps/desktop/test/human-requests-runner.test.ts @@ -197,6 +197,32 @@ test("Codex: a native question is a card answered by its schema; the CLI gets th } finally { await h.runner.dispose(); await codex.dispose(); } }); +for (const timing of ["before the question", "while the question is pending"] as const) { + test(`Codex: YOLO enabled ${timing} never answers a native question`, async () => { + const proc = new FakeProcess(); + const replies = codexAsking(proc, { questions }); + const codex = new CodexBackend("codex", fakeSpawn(proc).spawn); + const h = await harness(codex); + try { + h.runner.updateThread(h.thread.id, { approvals: timing === "before the question" ? "yolo" : "ask" }); + const sent = h.runner.send(h.thread.id, "deploy it"); + await waitFor(() => h.runner.items(h.thread.id).some(i => i.kind === "question")); + if (timing === "while the question is pending") h.runner.updateThread(h.thread.id, { approvals: "yolo" }); + await new Promise(resolve => setImmediate(resolve)); + const card = h.runner.items(h.thread.id).find(i => i.kind === "question") as ThreadItem & { kind: "question" }; + assert.equal(h.runner.status(h.thread.id), "waiting"); + assert.equal(card.answers, undefined); + assert.equal(card.withdrawn, undefined); + assert.deepEqual(replies, [], "YOLO must not send a native answer"); + const answers = { "q-env": ["staging"], "q-token": ["explicit-secret"] }; + assert.deepEqual(h.runner.answerQuestion(h.thread.id, card.id, answers), { ok: true }); + assert.deepEqual(h.runner.answerQuestion(h.thread.id, card.id, answers), { ok: false, reason: "already-resolved" }); + await sent; + assert.deepEqual(replies, [{ answers: { "q-env": { answers: ["staging"] }, "q-token": { answers: ["explicit-secret"] } } }]); + } finally { h.runner.stop(h.thread.id); await h.runner.dispose(); await codex.dispose(); } + }); +} + test("Codex: Stop during a question withdraws it; the CLI is told it was withdrawn, not given an answer", async () => { const proc = new FakeProcess(); const replies = codexAsking(proc, { questions }); diff --git a/docs/reviews/2026-11-01-cf3-question-regressions.md b/docs/reviews/2026-11-01-cf3-question-regressions.md new file mode 100644 index 00000000..7bbcf43d --- /dev/null +++ b/docs/reviews/2026-11-01-cf3-question-regressions.md @@ -0,0 +1,49 @@ +# CF-3 native-question regression work + +Status: local work in progress; not delivered or accepted. Original base: `9ecccc9bcea42ec99cd76092b1fe3bec15d05775`; integrated base: `7cfb29c7b0bfa452c9285f9b85a63a704cda857f`. This receipt concerns #276 and does not close its acceptance matrix. + +## Changes and scope + +Two runner tests exercise the real CodexBackend protocol fixture with YOLO enabled before a native question arrives and enabled while the question is pending. They require a waiting question with no answer, withdrawal or native reply until an explicit answer is supplied. A duplicate answer is refused, and the native process receives exactly one answer. + +The Electron regression creates an idle Codex thread, stages a multiline composer draft and a pasted file, then starts a separate text-only turn through the real main-process `thread:send` channel. A fixture-only gate holds the native question until that turn is running. Assertions cover composer focus when the question arrives, unchanged draft recovery through answering and renderer reload, exact staged attachment identity and bytes, no extra user message, one native reply and no secret answer in app storage. This is a synthetic CLI fixture, not live-provider evidence. + +The test initially attempted to stage the attachment while the thread was running. Independent review identified that the UI correctly refuses such staging. The revised setup stages it while idle; a second review found no further actionable defects by inspection. The recovered-host execution and further setup correction are recorded below. + +## Initial validation before host recovery + +| Check | Result | Limit | +| --- | --- | --- | +| Full build | Passed | Base plus local test changes; no production source changes | +| Workspace typecheck | Passed | E2E files are outside the configured TypeScript projects | +| Changed Electron spec, separate strict TypeScript check | Passed | Checks the spec and bridge types, not GUI behavior | +| Native-question fixture gate probe | Passed | Direct CLI protocol only; no Electron or composer | +| Human-request runner file | 34 passed | Includes both new YOLO cases | +| Temporary automatic-answer mutation | Both new cases failed | Compiled runner restored byte-for-byte; both cases passed again | +| Desktop TypeScript unit phase | 2,094 passed, 20 skipped, 2 failed | Notes watcher failures; full command exited 1 | +| Two Notes files in isolation | Same two failures | No change to these tests or their production source | +| Native JavaScript unit phase, separately invoked | 21 passed, 6 skipped | Does not make the failed full command green | +| New Electron test | Initial setup: launch failed before assertions; corrected setup: not executed | Electron launch exited with SIGABRT | +| Playwright test discovery and whitespace check | Passed | Parsing/discovery only, not UI execution | + +The Notes failures occur in the watched-project and watched-folder cache assertions. An independent Node `fs.watch` probe reports asynchronous `EMFILE` for both recursive and nonrecursive watchers. NotesStore responds by removing its watcher and therefore cannot reuse the watcher-backed read cache those tests assert. Ordinary descriptor opens succeed, and raising only the diagnostic child process's file limit to 65,536 does not fix the watcher error. The source of that host limitation is unresolved; no other session's process was terminated. + +The same Electron executable also aborts in a standalone GUI `--version` invocation, before Modex starts, while Node mode returns successfully. That reproduces a GUI startup failure outside this test, but does not establish its underlying cause. The first Playwright result directory was copied outside the worktree and checked file-by-file by SHA-256 before any later test run. + +A direct probe evaluated the spec's actual fixture generator, started its generated CLI, and observed the turn start without a question while the gate was closed. Opening the gate produced exactly one native question with the two expected schema fields. One explicit answer produced one logged reply and completed the turn. This validates the new fixture's protocol sequencing only; the corrected Electron case has not executed. + +Logs and the first-failure archive are retained under `/Users/buns/Documents/Codex/artifacts/modex-land-and-prune/` with the prefix `cf3-question-regressions-`. The external checkpoint records commands, process outcomes and review findings. + +## Remaining gates + +Deliver the verified candidate through signed commits and protected CI. Focus after answering and approval interactions are not covered by this new case. Human desktop/phone acceptance, VoiceOver decisions and TestFlight acceptance retain their existing separate gates. No issue checkbox is changed by this receipt. + +## Host recovery and integrated verification + +The host now delivers filesystem watcher events without errors and runs Electron GUI startup. The usual commit-signing identity was loaded successfully. The corrected idle-staging case initially failed because direct `thread:create` returns the new thread to its caller without adding it to renderer state. Reloading before selecting the thread hydrates that state through the normal startup path. This is a test setup correction, not a production change; the failing log and trace are retained under `cf3-question-regressions-recovered-first-results`. + +All four human-request Electron cases passed after the correction. The worktree then fast-forwarded to main `7cfb29c`; the status-document conflict retained both upstream content and the CF-3 entry. The four cases passed again against the rebuilt integrated source (17.5 seconds). Independent read-only review found no actionable issue in either changed test file. + +Two temporary fault injections exercised the new case. Clearing the composer after question arrival failed its exact-text assertion. Corrupting the staged attachment caused normal reload recovery to refuse the changed file with “A saved attachment is missing or changed; all files were preserved,” and the test failed instead of accepting missing composer state. Both injections were removed; the restored four-case run passed. These are controlled test faults, not reports of production data loss. + +Integrated build, workspace typecheck and the separate strict E2E typecheck passed. Full `pnpm test` passed: worktree 120, core 16, desktop TypeScript 2,117 with 20 skips, desktop native/helper 21 with six skips, site 10 and Store 16 (2,300 passes, 26 skips, no failures). Both formerly failing Notes watcher cases passed. Full `pnpm test:e2e` passed: 481 desktop tests passed, seven skipped (23.2 minutes), and seven site tests passed (6.8 seconds). The command exited zero without a retry. Historical failures above remain preserved and do not describe the recovered host's current result. diff --git a/docs/status.md b/docs/status.md index 92b4f7ae..186475e6 100644 --- a/docs/status.md +++ b/docs/status.md @@ -269,6 +269,8 @@ Claude account pool review repairs passed build, full unit validation and the ac - **Current Code Review repository identity.** A reproduced C22 gap kept the inbox's repository name in the PR header and links after the overview returned a new name. The candidate adopts the overview locator after the existing PR-key check and retains it across an unavailable refresh. The Electron regression failed before the fix and passed afterward; independent review found no blockers, and the full unit retry passed (2,277 passes, 26 skips). Full E2E passed (479 desktop tests, 7 skips; 7 site tests). [Evidence](reviews/2026-11-01-cr4-current-locator.md). +- **CF-3 question regressions (#276).** Local regression work adds native-question YOLO timing cases and a composer draft/attachment preservation fixture. Current-main build, typechecks, full unit validation (2,300 passes, 26 skips), four focused Electron cases and full E2E (481 desktop passes, seven skips; seven site passes) pass after host recovery and a fixture setup correction. Post-answer focus, approval interactions and human/native acceptance remain open. See the [validation receipt](reviews/2026-11-01-cf3-question-regressions.md). + - **Deferred Markdown for long threads (hosted cold-open budget).** Hosted CI's approved 500 ms cold-open gate failed on runner variance alone: 33 hosted runs on 2026-10-30 had a 377.7 ms median and a 781.9 ms failure on application code identical to a 436.1 ms pass. Val chose on 2026-10-30 to defer offscreen answers. Turns before the newest eight show their Markdown as written until it nears the view, then parse once; scroll anchoring keeps the reader's place. Locally the five-launch median fell from about 190 to 88 ms, and 3× CPU throttling cut it from 806–870 to 312–340 ms. The gate is unchanged. The first hosted run had a 129.9 ms median (481 desktop E2E passes). It landed through #463 (`32f1cb4`) on 2026-10-31 UTC and is unreleased. [Evidence](reviews/2026-10-30-deferred-markdown.md). Desktop CI memory diagnostics landed through #458 after #456’s cold transcript budget failure. The workflow reuses the existing resource sampler and retains its log as a separate CI artifact; it does not establish the cause of the earlier failure or change the 500 ms gate. The #458 queue passed desktop and site E2E and retained its memory log. See [the diagnostic review](reviews/2026-10-30-desktop-ci-memory.md). From 24735bba7a24bca025dd75005bbc29e2a0e76fc3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Val=20Alexander=20=F0=9F=91=91?= Date: Mon, 2 Nov 2026 18:53:34 -0600 Subject: [PATCH 2/3] fix: retain keyboard focus through native question answers --- apps/desktop/e2e/human-requests.spec.ts | 29 ++++++++++++++++--- .../components/transcript/QuestionCard.tsx | 22 +++++++++++--- docs/reviews/2026-11-02-cf3-question-focus.md | 25 ++++++++++++++++ docs/status.md | 2 ++ 4 files changed, 70 insertions(+), 8 deletions(-) create mode 100644 docs/reviews/2026-11-02-cf3-question-focus.md diff --git a/apps/desktop/e2e/human-requests.spec.ts b/apps/desktop/e2e/human-requests.spec.ts index c5035e44..9ca91e0e 100644 --- a/apps/desktop/e2e/human-requests.spec.ts +++ b/apps/desktop/e2e/human-requests.spec.ts @@ -10,7 +10,7 @@ import { launch, seedHome, tid, items } from "./support"; // It is a scripted fixture, not live-model evidence. Its log lives outside MODEX_HOME so the secret check // below can read every file Modex wrote. -function fixture(dir: string, questionGate?: string): string { +function fixture(dir: string, questionGate?: string, finishGate?: string): string { const cli = path.join(dir, "codex-questions.cjs"); fs.writeFileSync(cli, `#!${process.execPath} const fs = require('node:fs'); @@ -18,6 +18,7 @@ if (process.argv.includes('--version')) { console.log('codex-cli 0.163.0'); proc 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 questionGate = ${JSON.stringify(questionGate ?? null)}; +const finishGate = ${JSON.stringify(finishGate ?? null)}; const emit = (o) => process.stdout.write(JSON.stringify(o) + '\\n'); const questions = [ { id: 'q-env', header: 'Environment', question: 'Which environment?', isOther: false, isSecret: false, options: [{ label: 'staging', description: 'Safe to try' }, { label: 'production', description: 'Live traffic' }] }, @@ -69,7 +70,12 @@ require('node:readline').createInterface({ input: process.stdin }).on('line', (l 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) finish('completed', 'Deploying to ' + msg.result.answers['q-env'].answers[0] + '.'); + if (msg.result) { + const complete = () => finish('completed', 'Deploying to ' + msg.result.answers['q-env'].answers[0] + '.'); + if (finishGate) { + const timer = setInterval(() => { if (fs.existsSync(finishGate)) { clearInterval(timer); complete(); } }, 10); + } else complete(); + } return; } if (msg.id !== undefined && msg.method) emit({ id: msg.id, error: { code: -32601, message: 'fixture does not implement ' + msg.method } }); @@ -95,13 +101,14 @@ async function send(page: Page, text: string): Promise { } const currentRow = (page: Page) => page.locator('[data-testid="thread-row"][aria-current="true"]'); -test("a native question preserves the existing composer draft, staged attachment and arrival focus", async () => { +for (const moveFocus of [false, true]) test(`a native question preserves the existing composer draft, staged attachment and ${moveFocus ? "moved" : "answer"} focus`, async () => { const { home } = seedHome({ default_backend: "codex", default_mode: "agent" }); const logs = fs.mkdtempSync(path.join(os.tmpdir(), "modex-e2e-question-draft-")); const gate = path.join(logs, "ask-now"); + const finishGate = path.join(logs, "finish-now"); const statePath = path.join(home, "app/state.json"); const state = JSON.parse(fs.readFileSync(statePath, "utf8")); - state.settings.codex_bin = fixture(logs, gate); + state.settings.codex_bin = fixture(logs, gate, finishGate); fs.writeFileSync(statePath, JSON.stringify(state)); let app: ElectronApplication | undefined; try { @@ -140,9 +147,23 @@ test("a native question preserves the existing composer draft, staged attachment expect(recovery()).toEqual(saved); expect(replies(logs)).toEqual([]); await card.getByRole("radio", { name: "staging" }).check(); + // Invalid keyboard submission must leave a usable focus target inside the card. + await card.getByLabel("Token").focus(); + await card.getByLabel("Token").press("Enter"); + await expect(tid(card, "question-error")).toHaveText("Token needs an answer."); + await expect(card.getByLabel("Token")).toBeFocused(); + expect(replies(logs)).toEqual([]); await card.getByLabel("Token").fill("question-only-secret"); + if (moveFocus) await page.evaluate(() => { + // Bubble after React's Enter handler, before main's asynchronous answer comes back. + document.addEventListener("keydown", (event) => { event.preventDefault(); (document.querySelector('[data-testid="composer-input"]') as HTMLTextAreaElement).focus(); }, { once: true }); + }); await card.getByLabel("Token").press("Enter"); + await expect(tid(card, "question-answer")).toHaveText(["staging", "(hidden)"]); + await expect(moveFocus ? composer : tid(card, "question-answer").first()).toBeFocused(); + fs.writeFileSync(finishGate, "ready"); await expect(currentRow(page)).toHaveAttribute("data-status", "idle"); + await expect(moveFocus ? composer : tid(page, "turn-label")).toBeFocused(); await expect(composer).toHaveValue(draft); await expect(tid(page, "composer-attachments")).toContainText("draft.txt"); expect(recovery()).toEqual(saved); diff --git a/apps/desktop/src/renderer/components/transcript/QuestionCard.tsx b/apps/desktop/src/renderer/components/transcript/QuestionCard.tsx index ba461635..4f770af8 100644 --- a/apps/desktop/src/renderer/components/transcript/QuestionCard.tsx +++ b/apps/desktop/src/renderer/components/transcript/QuestionCard.tsx @@ -1,4 +1,4 @@ -import { createContext, useContext, useEffect, useState } from "react"; +import { createContext, useContext, useEffect, useLayoutEffect, useRef, useState } from "react"; import type { AnswerResult, ThreadItem } from "../../../shared/types"; import { answerNote, type Question } from "../../../shared/human-requests"; @@ -33,6 +33,8 @@ const OTHER = "\u0000other"; * answers against the questions before anything reaches the CLI, and the card says why when it refuses. */ export function QuestionCard({ item, onAnswer }: { item: QuestionEntry; onAnswer: (itemId: string, answers: Record) => Promise }) { + const card = useRef(null); + const focusOrigin = useRef(null); const drafts = useContext(QuestionDraftContext); const [draft, setDraft] = useState(() => drafts?.get(item.id) ?? { choice: {}, text: {} }); const { choice, text } = draft; @@ -50,9 +52,21 @@ export function QuestionCard({ item, onAnswer }: { item: QuestionEntry; onAnswer setDraft({ choice: {}, text: {} }); } }, [open, drafts, item.id]); + useLayoutEffect(() => { + const origin = focusOrigin.current; + if (!origin || (open && sending)) return; + focusOrigin.current = null; + // Disabling/removing the submitting control drops focus to body. Restore it only + // if the person has not moved elsewhere while main validates their answer. + if (document.activeElement !== document.body && document.activeElement !== origin) return; + if (open) origin.focus({ preventScroll: true }); + else card.current?.querySelector('[data-testid="question-answer"], [data-testid="question-delivery"], [data-testid="question-closed"]')?.focus({ preventScroll: true }); + }, [open, sending]); const typed = (q: Question) => !q.options || choice[q.id] === OTHER; const value = (q: Question) => (typed(q) ? text[q.id] ?? "" : choice[q.id] ?? ""); const submit = async () => { + if (sending) return; + focusOrigin.current = card.current?.contains(document.activeElement) ? document.activeElement as HTMLElement : null; setSending(true); setError(null); try { @@ -65,7 +79,7 @@ export function QuestionCard({ item, onAnswer }: { item: QuestionEntry; onAnswer } }; return ( -
+
? {item.questions.length === 1 ? "The agent has a question" : `The agent has ${item.questions.length} questions`} @@ -74,7 +88,7 @@ export function QuestionCard({ item, onAnswer }: { item: QuestionEntry; onAnswer
{q.header || q.text}
{q.header &&
{q.text}
} - {item.answers ?
{(item.answers[q.id] ?? []).join(", ")}
: open && ( + {item.answers ?
{(item.answers[q.id] ?? []).join(", ")}
: open && ( <> {q.options && (
@@ -103,7 +117,7 @@ export function QuestionCard({ item, onAnswer }: { item: QuestionEntry; onAnswer
))} {item.answers && answerNote(item) &&
{answerNote(item)}
} - {item.delivery === "uncertain" || item.delivery === "sending" ?
{item.delivery === "uncertain" ? "Delivery uncertain — not resent. Check the agent before answering again." : "Sending answer…"}
: item.withdrawn ?
{CLOSED[item.withdrawn]}
: open && ( + {item.delivery === "uncertain" || item.delivery === "sending" ?
{item.delivery === "uncertain" ? "Delivery uncertain — not resent. Check the agent before answering again." : "Sending answer…"}
: item.withdrawn ?
{CLOSED[item.withdrawn]}
: open && (
{error && {error}} diff --git a/docs/reviews/2026-11-02-cf3-question-focus.md b/docs/reviews/2026-11-02-cf3-question-focus.md new file mode 100644 index 00000000..b48f5a1a --- /dev/null +++ b/docs/reviews/2026-11-02-cf3-question-focus.md @@ -0,0 +1,25 @@ +# CF-3 native-question keyboard focus regression + +This local follow-up to PR #487 starts at `712da73ebf2403081d8864ae3bfe529e4025e5ce`, preserving its native-question composer draft and attachment fixture. It addresses keyboard focus only; it is not human, VoiceOver, live-model, phone or release acceptance, and #276 remains open. + +## Reproduction and repair + +The first Electron regression submitted an incomplete native question with Enter. Main refused the answer and the card displayed its validation error, but disabling the Token input dropped focus to the page body. The original source also failed a separately gated accepted-answer check: after Enter removed the input, the displayed answer was not focused. Both failures were observed against the real renderer, IPC, runner and scripted Codex app-server fixture. + +The card now remembers its submitting control, restores that control after validation fails, and focuses the first answer or closure status when controls are removed. It checks the current active element before restoring focus so a person who moves to the composer keeps focus there. Answer/status elements are programmatically focusable without adding Tab stops. The existing turn-fold handoff continues to move focus to the turn toggle. + +The fixture holds turn completion until the answer-focus assertion finishes. Both variants preserve the exact whitespace-bearing draft, staged attachment, persisted recovery and attachment bytes through answer and reload. The moved-focus variant changes focus after the real Enter handler and before its asynchronous reply, then checks that the composer remains focused. Existing approval and Stop keyboard tests are unchanged. + +## Validation evidence + +Artifacts are retained outside the worktree under `/Users/buns/Documents/Codex/artifacts/modex-land-and-prune/question-focus-*`. + +- `question-focus-red.log` and `question-focus-red-results/`: original invalid-submission focus failure. +- `question-focus-accepted-baseline-red.log` and `question-focus-accepted-baseline-red-results/`: original accepted-submission focus failure with the corrected finish gate. +- `question-focus-green-initial.log` and `question-focus-accepted-red-results/`: intermediate test expectation error; the assertion ran after the existing fold handoff. This is not evidence of an additional product bug. +- `question-focus-moved-fixture-failure.log` and its results directory: intermediate fixture error; Enter's default action inserted a newline after the fixture moved focus. The fixture now cancels that default action before moving focus. +- `question-focus-green.log`: all 13 focused native-question and existing keyboard cases passed in 33.5 seconds. + +The full `pnpm build && pnpm test && pnpm typecheck` chain exited 0. Unit validation passed 2,300 tests with 26 skips: root 120; core 16; desktop 2,117 plus 20 skips; native checks 21 plus six skips; site 10; Store 16. `question-focus-validation-tail.log` retains the suite totals and typecheck completion; the initial build output is in the tool transcript. Build warnings were the existing bundle-size notices. Full E2E is queued behind another session’s Electron run and remains pending. + +The changed Electron spec also passed its separate strict check: `pnpm --filter @modex/desktop exec tsc --noEmit --strict --noUncheckedIndexedAccess --target ES2022 --module ESNext --moduleResolution Bundler --skipLibCheck --types node e2e/human-requests.spec.ts src/renderer/bridge.ts` exited 0 (`question-focus-e2e-typecheck.log`). Workspace typechecks do not include E2E specs. diff --git a/docs/status.md b/docs/status.md index 186475e6..e6b7a8a6 100644 --- a/docs/status.md +++ b/docs/status.md @@ -269,6 +269,8 @@ Claude account pool review repairs passed build, full unit validation and the ac - **Current Code Review repository identity.** A reproduced C22 gap kept the inbox's repository name in the PR header and links after the overview returned a new name. The candidate adopts the overview locator after the existing PR-key check and retains it across an unavailable refresh. The Electron regression failed before the fix and passed afterward; independent review found no blockers, and the full unit retry passed (2,277 passes, 26 skips). Full E2E passed (479 desktop tests, 7 skips; 7 site tests). [Evidence](reviews/2026-11-01-cr4-current-locator.md). +- **CF-3 native-question keyboard focus (#276).** Local follow-up to #487 reproduces focus loss after invalid and accepted Enter submissions and repairs the handoff without stealing focus from the composer. Build, typechecks, 2,300 unit checks (26 skipped), and thirteen focused Electron cases pass, including exact draft/attachment preservation and existing approval/Stop checks; full E2E is pending. Human/native acceptance remains open. See the [validation receipt](reviews/2026-11-02-cf3-question-focus.md). + - **CF-3 question regressions (#276).** Local regression work adds native-question YOLO timing cases and a composer draft/attachment preservation fixture. Current-main build, typechecks, full unit validation (2,300 passes, 26 skips), four focused Electron cases and full E2E (481 desktop passes, seven skips; seven site passes) pass after host recovery and a fixture setup correction. Post-answer focus, approval interactions and human/native acceptance remain open. See the [validation receipt](reviews/2026-11-01-cf3-question-regressions.md). - **Deferred Markdown for long threads (hosted cold-open budget).** Hosted CI's approved 500 ms cold-open gate failed on runner variance alone: 33 hosted runs on 2026-10-30 had a 377.7 ms median and a 781.9 ms failure on application code identical to a 436.1 ms pass. Val chose on 2026-10-30 to defer offscreen answers. Turns before the newest eight show their Markdown as written until it nears the view, then parse once; scroll anchoring keeps the reader's place. Locally the five-launch median fell from about 190 to 88 ms, and 3× CPU throttling cut it from 806–870 to 312–340 ms. The gate is unchanged. The first hosted run had a 129.9 ms median (481 desktop E2E passes). It landed through #463 (`32f1cb4`) on 2026-10-31 UTC and is unreleased. [Evidence](reviews/2026-10-30-deferred-markdown.md). From 69309836f830d0392c4d333663b11bf1b42d670f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Val=20Alexander=20=F0=9F=91=91?= Date: Mon, 2 Nov 2026 19:27:25 -0600 Subject: [PATCH 3/3] docs: record native question focus validation --- docs/reviews/2026-11-02-cf3-question-focus.md | 2 +- docs/status.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/reviews/2026-11-02-cf3-question-focus.md b/docs/reviews/2026-11-02-cf3-question-focus.md index b48f5a1a..0ebd6df9 100644 --- a/docs/reviews/2026-11-02-cf3-question-focus.md +++ b/docs/reviews/2026-11-02-cf3-question-focus.md @@ -20,6 +20,6 @@ Artifacts are retained outside the worktree under `/Users/buns/Documents/Codex/a - `question-focus-moved-fixture-failure.log` and its results directory: intermediate fixture error; Enter's default action inserted a newline after the fixture moved focus. The fixture now cancels that default action before moving focus. - `question-focus-green.log`: all 13 focused native-question and existing keyboard cases passed in 33.5 seconds. -The full `pnpm build && pnpm test && pnpm typecheck` chain exited 0. Unit validation passed 2,300 tests with 26 skips: root 120; core 16; desktop 2,117 plus 20 skips; native checks 21 plus six skips; site 10; Store 16. `question-focus-validation-tail.log` retains the suite totals and typecheck completion; the initial build output is in the tool transcript. Build warnings were the existing bundle-size notices. Full E2E is queued behind another session’s Electron run and remains pending. +The full `pnpm build && pnpm test && pnpm typecheck` chain exited 0. Unit validation passed 2,300 tests with 26 skips: root 120; core 16; desktop 2,117 plus 20 skips; native checks 21 plus six skips; site 10; Store 16. `question-focus-validation-tail.log` retains the suite totals and typecheck completion; the initial build output is in the tool transcript. Build warnings were the existing bundle-size notices. Full `pnpm test:e2e` passed on signed candidate `24735bba7a24bca025dd75005bbc29e2a0e76fc3` (tree `00e589c1182f45ffdfa3f0f53299b0d6aac1a6ec`): 482 desktop tests passed with seven skips in 22.0 minutes, and seven site tests passed in 10.3 seconds. The command exited 0 without a retry; `question-focus-full-e2e.log` retains its complete output. The source and worktree stayed unchanged throughout that run. The run started after the other observed GUI suites exited; final process inventory showed independently started Electron processes from other owners, so exclusive GUI ownership throughout the entire run is not claimed. The changed Electron spec also passed its separate strict check: `pnpm --filter @modex/desktop exec tsc --noEmit --strict --noUncheckedIndexedAccess --target ES2022 --module ESNext --moduleResolution Bundler --skipLibCheck --types node e2e/human-requests.spec.ts src/renderer/bridge.ts` exited 0 (`question-focus-e2e-typecheck.log`). Workspace typechecks do not include E2E specs. diff --git a/docs/status.md b/docs/status.md index e6b7a8a6..fc46c5bd 100644 --- a/docs/status.md +++ b/docs/status.md @@ -269,7 +269,7 @@ Claude account pool review repairs passed build, full unit validation and the ac - **Current Code Review repository identity.** A reproduced C22 gap kept the inbox's repository name in the PR header and links after the overview returned a new name. The candidate adopts the overview locator after the existing PR-key check and retains it across an unavailable refresh. The Electron regression failed before the fix and passed afterward; independent review found no blockers, and the full unit retry passed (2,277 passes, 26 skips). Full E2E passed (479 desktop tests, 7 skips; 7 site tests). [Evidence](reviews/2026-11-01-cr4-current-locator.md). -- **CF-3 native-question keyboard focus (#276).** Local follow-up to #487 reproduces focus loss after invalid and accepted Enter submissions and repairs the handoff without stealing focus from the composer. Build, typechecks, 2,300 unit checks (26 skipped), and thirteen focused Electron cases pass, including exact draft/attachment preservation and existing approval/Stop checks; full E2E is pending. Human/native acceptance remains open. See the [validation receipt](reviews/2026-11-02-cf3-question-focus.md). +- **CF-3 native-question keyboard focus (#276).** Local follow-up to #487 reproduces focus loss after invalid and accepted Enter submissions and repairs the handoff without stealing focus from the composer. Build, typechecks, 2,300 unit checks (26 skipped), thirteen focused Electron cases, and full E2E (482 desktop passes, seven skips; seven site passes) pass on signed candidate `24735bb`, including exact draft/attachment preservation and existing approval/Stop checks. Human/native acceptance remains open. See the [validation receipt](reviews/2026-11-02-cf3-question-focus.md). - **CF-3 question regressions (#276).** Local regression work adds native-question YOLO timing cases and a composer draft/attachment preservation fixture. Current-main build, typechecks, full unit validation (2,300 passes, 26 skips), four focused Electron cases and full E2E (481 desktop passes, seven skips; seven site passes) pass after host recovery and a fixture setup correction. Post-answer focus, approval interactions and human/native acceptance remain open. See the [validation receipt](reviews/2026-11-01-cf3-question-regressions.md).