diff --git a/coworker/engine.py b/coworker/engine.py index f16afe865..9651dc880 100644 --- a/coworker/engine.py +++ b/coworker/engine.py @@ -1837,7 +1837,17 @@ async def _handle_ask_user(self, tool_call: ToolCall) -> AsyncIterator[Event]: "error": "no response", } - status = "ok" if (result.get("answer") or result.get("answers")) else "denied" + # A skipped question answers None (OPE-153), so an all-skipped card is "denied" like an + # unanswered one — `answers` being non-empty isn't enough on its own. + replies = result.get("answers") + status = ( + "ok" + if ( + result.get("answer") + or (isinstance(replies, dict) and any(v is not None for v in replies.values())) + ) + else "denied" + ) if status == "ok": self._note_ask_replies(result, question) self.messages.append(_tool_result_message(tool_call, result)) @@ -1872,8 +1882,10 @@ def _note_ask_replies( self._reviewer_denials = 0 anchor = sum(1 for m in self.messages if m.get("role") == "user") answers = result.get("answers") + # Skipped questions answer None (OPE-153) — dropped here, else str(None) files "None" + # as something the user actually said. values = ( - [str(v) for v in answers.values()] + [str(v) for v in answers.values() if v is not None] if isinstance(answers, dict) else [str(result.get("answer") or "")] ) diff --git a/coworker/interactions.py b/coworker/interactions.py index f40943b88..371f33501 100644 --- a/coworker/interactions.py +++ b/coworker/interactions.py @@ -17,7 +17,7 @@ from typing import Optional from .inbox import KIND_APPROVAL, KIND_QUESTION -from .tools.ask import option_label +from .tools.ask import SKIP_SENTINEL, option_label @dataclass @@ -56,8 +56,15 @@ def buttons_for(item) -> list[Button]: if item.kind == KIND_QUESTION and getattr(item, "options", None): # One button per option; the resolution IS the chosen option's label (what the agent # gets). Rich {label, description, …} options button as their label. + # + # Skip trails the options (OPE-153) so the channel has the same way out the card has: + # without it, a reader whose answer isn't on the list can only pick a wrong option or + # leave the agent suspended. It grants nobody new authority — anyone who can click an + # option here could already decide the question — it only adds declining to what they + # can already do. Grouped questions stay button-less on purpose: they fall back to the + # "(Open the app to respond.)" text, and the app is where their per-question skip lives. return [ Button(option_label(opt), encode(item.id, option_label(opt))) for opt in item.options - ] + ] + [Button("Skip", encode(item.id, SKIP_SENTINEL))] return [] diff --git a/coworker/tools/ask.py b/coworker/tools/ask.py index a426232dd..7c7a3ac7b 100644 --- a/coworker/tools/ask.py +++ b/coworker/tools/ask.py @@ -22,6 +22,24 @@ # How many questions one grouped call may carry (stepper chips get unreadable past this). MAX_GROUPED_QUESTIONS = 4 +# OPE-153 — the resolution string a card sends when the user declines to answer. The resolution +# field otherwise carries the answer verbatim, so "skipped" needs a value no answer can collide +# with; the GUI writes the same constant (see SKIP in InboxItemCard.tsx — keep the two in step). +# A grouped card marks a per-question skip by using it as that question's value in the answer map, +# and a whole-card skip by filling every unanswered question with it. +SKIP_SENTINEL = "__ocw_skip__" + +# Told to the agent alongside a skipped answer. Only the no-re-ask half is absolute — it's the +# anti-loop guarantee the skip exists for. "Pick a default" is deliberately conditional: the same +# note fires for "you choose the chart colour" and for "Staging or Production?", and a model that +# follows instructions literally would deploy off a shrug. Proceeding without deciding is offered +# as a third path so that judgment is asked for, not left to whichever model happens to be driving. +SKIP_NOTE = ( + "The user chose not to answer. Do not put this question back to them. If you can proceed " + "safely, pick a sensible default and say which way you went. If the choice is consequential " + "or hard to undo, don't guess — say what you'd need and let them come back to it." +) + # An option is a plain string OR a rich object. `label` is what the user picks (and what comes # back as the answer); `description` renders under it; `recommended` adds the green tag (put the # recommended option first); `preview` is monospace text shown in the side pane (code, config, @@ -143,6 +161,12 @@ def ask_user( Grouped form (`questions`) returns `{"answers": {"
": "..."}}` — one entry per question. Don't ask what you can reasonably decide yourself; reserve this for choices that are actually the user's to make. + + The user may SKIP any question. A skipped answer comes back as `null` (never ""), with + `skipped` naming what was declined. Treat it as "you decide" where the choice is safe: + pick a sensible default, continue, and say which way you went. Where it isn't — anything + consequential or hard to undo — don't guess; say what you'd need instead. Either way, + never re-ask a skipped question. """ # Real handling lives in the engine (it needs the out-of-band Inbox round-trip). This body # only runs if no question_asker is wired (e.g. a headless surface). @@ -242,22 +266,56 @@ def question_item_fields(args: dict) -> dict | None: } +def question_key(entry) -> str: + """The answer-map key for one grouped question — its header, else the question text. The + same key the card writes when it resolves, so the two halves agree.""" + if not isinstance(entry, dict): + return "answer" + return str(entry.get("header") or entry.get("question") or "answer") + + def answer_result(item_questions: list, resolution: str | None) -> dict: """Shape the ask_user tool result from an Inbox item's resolution string. Grouped items resolve with a JSON object string keyed by header-or-question → `{"answers": {...}}`; - everything else returns the plain `{"answer": str}` shape.""" + everything else returns the plain `{"answer": str}` shape. + + A skipped question (OPE-153) answers `None` rather than "", so the agent can tell "the user + declined" from "the answer came back empty" — the two used to be the same value. Skips also + carry `skipped` (the question keys, or True for a lone question) and `SKIP_NOTE`.""" if item_questions: + # A text-only surface (mirrored channel) can only send the bare sentinel: that skips the + # whole card, so every question answers None. + if resolution == SKIP_SENTINEL: + keys = [question_key(q) for q in item_questions] + return { + "answers": dict.fromkeys(keys), + "skipped": keys, + "note": SKIP_NOTE, + } try: parsed = json.loads(resolution or "") except (ValueError, TypeError): parsed = None if isinstance(parsed, dict): - return {"answers": {str(k): str(v) for k, v in parsed.items()}} + answers: dict[str, str | None] = {} + skipped: list[str] = [] + for k, v in parsed.items(): + key = str(k) + if v is None or str(v) == SKIP_SENTINEL: + answers[key] = None + skipped.append(key) + else: + answers[key] = str(v) + result: dict = {"answers": answers} + if skipped: + result["skipped"] = skipped + result["note"] = SKIP_NOTE + return result if resolution: # Answered from a text-only surface (e.g. a mirrored channel): attribute the lone # answer to the first question rather than losing it. - first = item_questions[0] if isinstance(item_questions[0], dict) else {} - key = str(first.get("header") or first.get("question") or "answer") - return {"answers": {key: str(resolution)}} + return {"answers": {question_key(item_questions[0]): str(resolution)}} return {"answer": ""} + if resolution == SKIP_SENTINEL: + return {"answer": None, "skipped": True, "note": SKIP_NOTE} return {"answer": resolution or ""} diff --git a/surfaces/gui/e2e/ask-skip.spec.ts b/surfaces/gui/e2e/ask-skip.spec.ts new file mode 100644 index 000000000..516789250 --- /dev/null +++ b/surfaces/gui/e2e/ask-skip.spec.ts @@ -0,0 +1,163 @@ +import type { Page } from "@playwright/test"; +import { test, expect } from "./fixtures"; + +// OPE-153 — skipping a question. A question card used to have no exit: the only ways out were +// picking an option or typing an answer, so a user who couldn't answer had to invent one or +// abandon the session. Now every card carries Skip, and grouped cards carry both "Skip question" +// (this one, advance) and "Skip all" (resolve the rest). Seeded via a per-test inbox route +// override so the base fixtures' counts stay untouched. + +// Mirrors SKIP in InboxItemCard.tsx / SKIP_SENTINEL in coworker/tools/ask.py. +const SKIP = "__ocw_skip__"; + +const BASE = { + body: "", + state: "pending", + resolution: null as string | null, + inbox: "default", + created_at: "2026-08-31 08:00:00", + resolved_at: null as string | null, + session_title: "Investigate alerts", + session_agent: "ops", + session_workspace: "", + session_exists: true, +}; + +const SINGLE_ITEM = { + ...BASE, + id: "inb-question-single", + session_id: "ops-1", + kind: "question", + title: "Which environment should I deploy to?", + header: "Environment", + options: ["Staging", "Production"], + allow_text: true, + multi: false, + questions: [], +}; + +// The card a user could previously get stuck on: exhaustive options, no free-text escape. +const NO_TEXT_ITEM = { + ...SINGLE_ITEM, + id: "inb-question-notext", + allow_text: false, +}; + +const GROUPED_ITEM = { + ...BASE, + id: "inb-question-grouped", + session_id: "ops-1", + kind: "question", + title: "Chart style?", + header: "Chart style", + options: ["Bar", "Line"], + allow_text: false, + multi: false, + questions: [ + { question: "Chart style?", header: "Chart style", options: ["Bar", "Line"], allow_text: false, multi: false }, + { question: "Which distribution?", header: "Distribution", options: ["Stacked", "Grouped"], allow_text: false, multi: false }, + { question: "Which palette?", header: "Palette", options: ["Warm", "Cool"], allow_text: false, multi: false }, + ], +}; + +/** Replace the Inbox's seeded items for this test (resolve mutates the local copy). */ +async function seedInbox(page: Page, items: Record[]) { + const inbox = items.map((i) => ({ ...i })); + const json = (body: unknown) => ({ + status: 200, + contentType: "application/json", + body: JSON.stringify(body), + }); + await page.route(/\/v1\/inbox\/[^/]+\/resolve$/, (route) => { + const path = new URL(route.request().url()).pathname; + const id = decodeURIComponent(path.split("/").slice(-2)[0]); + const it = inbox.find((x) => x.id === id); + if (it) { + it.state = "resolved"; + it.resolution = route.request().postDataJSON().resolution; + } + return route.fulfill(json({ ok: true })); + }); + await page.route(/\/v1\/inbox(\?.*)?$/, (route) => + route.fulfill(json({ items: inbox.filter((i) => i.state === "pending") })), + ); + return inbox; +} + +async function openInbox(page: Page, expectTitle: string) { + await page.goto("/"); + await page.getByTestId("inbox-chip").click(); + await expect(page.getByText(expectTitle)).toBeVisible(); +} + +test("a single question can be skipped, and resolves as skipped rather than blank", async ({ + page, +}) => { + await seedInbox(page, [SINGLE_ITEM]); + await openInbox(page, "Which environment should I deploy to?"); + + // One control, labelled plainly — no stepper, so nothing to say "question" about. + const skip = page.getByTestId("question-skip"); + await expect(skip.getByRole("button", { name: "Skip", exact: true })).toBeVisible(); + await expect(skip.getByRole("button", { name: "Skip all" })).toHaveCount(0); + + const resolved = page.waitForRequest((r) => r.url().includes("/resolve") && r.method() === "POST"); + await skip.getByRole("button", { name: "Skip", exact: true }).click(); + expect((await resolved).postDataJSON().resolution).toBe(SKIP); + await expect(page.getByText("Nothing pending.")).toBeVisible(); +}); + +test("Skip is offered even when the card has no free-text escape", async ({ page }) => { + await seedInbox(page, [NO_TEXT_ITEM]); + await openInbox(page, "Which environment should I deploy to?"); + + // Exhaustive options and no "Or type your own answer…" — Skip is the ONLY way out. + await expect(page.getByPlaceholder("Or type your own answer…")).toHaveCount(0); + const resolved = page.waitForRequest((r) => r.url().includes("/resolve") && r.method() === "POST"); + await page.getByTestId("question-skip").getByRole("button", { name: "Skip", exact: true }).click(); + expect((await resolved).postDataJSON().resolution).toBe(SKIP); +}); + +test("one step of a grouped card can be skipped while the others are answered", async ({ + page, +}) => { + await seedInbox(page, [GROUPED_ITEM]); + await openInbox(page, "Chart style?"); + + const stepper = page.getByTestId("question-stepper"); + const skip = page.getByTestId("question-skip"); + + // Skipping step 1 advances exactly as answering would. + await skip.getByRole("button", { name: "Skip question" }).click(); + await expect(stepper).toContainText("2 of 3"); + + await page.getByRole("button", { name: "Stacked", exact: true }).click(); + await expect(stepper).toContainText("3 of 3"); + + // Last step: nothing left to "skip all", so only the per-question control remains. + await expect(skip.getByRole("button", { name: "Skip all" })).toHaveCount(0); + + const resolved = page.waitForRequest((r) => r.url().includes("/resolve") && r.method() === "POST"); + await page.getByRole("button", { name: "Warm", exact: true }).click(); + expect((await resolved).postDataJSON().resolution).toBe( + JSON.stringify({ "Chart style": SKIP, Distribution: "Stacked", Palette: "Warm" }), + ); +}); + +test("Skip all resolves the rest of a grouped card but keeps answers already given", async ({ + page, +}) => { + await seedInbox(page, [GROUPED_ITEM]); + await openInbox(page, "Chart style?"); + + await page.getByRole("button", { name: "Bar", exact: true }).click(); + await expect(page.getByTestId("question-stepper")).toContainText("2 of 3"); + + const resolved = page.waitForRequest((r) => r.url().includes("/resolve") && r.method() === "POST"); + await page.getByTestId("question-skip").getByRole("button", { name: "Skip all" }).click(); + // Step 1's real answer survives; only the two the user never reached are skipped. + expect((await resolved).postDataJSON().resolution).toBe( + JSON.stringify({ "Chart style": "Bar", Distribution: SKIP, Palette: SKIP }), + ); + await expect(page.getByText("Nothing pending.")).toBeVisible(); +}); diff --git a/surfaces/gui/src/components/InboxItemCard.tsx b/surfaces/gui/src/components/InboxItemCard.tsx index f5bc18dfe..a341f261b 100644 --- a/surfaces/gui/src/components/InboxItemCard.tsx +++ b/surfaces/gui/src/components/InboxItemCard.tsx @@ -28,6 +28,10 @@ const BTN_BORDERED = const BTN_ACCENT = "px-3 py-1.5 rounded-lg border border-accent text-accent text-[13px] font-semibold hover:bg-accentSoft"; const BTN_QUIET = "px-3 py-1.5 text-[13px] text-faint hover:text-danger"; +// Skip (OPE-153) is an escape, not a rejection — quiet like Deny, without the danger red. Uses +// `muted` rather than Deny's `faint`: a borderless text button is already secondary next to the +// option pills, and this one has to be FOUND (faint is only ~3:1 on the dark panel). +const BTN_SKIP = "px-3 py-1.5 text-[13px] text-muted hover:text-ink"; const OPT_BASE = "inline-flex items-center gap-1.5 px-3 py-1.5 rounded-full border text-[13px] transition-colors"; const OPT_OFF = "border-line bg-paper text-ink hover:border-accent hover:bg-accentSoft/50"; @@ -39,6 +43,11 @@ const ROW_BASE = "w-full text-left rounded-lg border px-3 py-2 transition-colors const ROW_OFF = "border-line bg-paper hover:border-accent hover:bg-accentSoft/50"; const ROW_ON = "border-accent bg-accentSoft"; +// The resolution a card sends when the user declines to answer (OPE-153). Mirrors +// SKIP_SENTINEL in coworker/tools/ask.py, which turns it back into a null answer — +// keep the two constants in step. +const SKIP = "__ocw_skip__"; + // -- question normalization --------------------------------------------------- interface NormOption { @@ -248,6 +257,25 @@ function QuestionCard({ else onResolve(item.id, JSON.stringify(all)); }; + // Skipping ONE question is just answering it with the sentinel — the stepper advances (or the + // card resolves on the last step) exactly as a real answer would. + const skipStep = () => submit(SKIP); + + // Skipping the WHOLE card resolves it now, marking every question the user never got to. + // Answers already given are kept: "skip all" means "don't ask me the rest", not "discard". + const skipAll = () => { + if (!grouped) { + onResolve(item.id, SKIP); + return; + } + const all = { ...answers }; + for (let i = step; i < specs.length; i++) { + const k = keyFor(specs[i]); + if (!(k in all)) all[k] = SKIP; + } + onResolve(item.id, JSON.stringify(all)); + }; + return ( <> {/* Stepper chips (grouped): "Chart style · 1 of 2 · Distribution ›" — ‹ steps back. */} @@ -286,6 +314,18 @@ function QuestionCard({ {chip} {/* key={step} resets selection/text/hover state when the stepper advances */} + {/* OPE-153: the way out. Always present — a card with exhaustive options and + allow_text:false is exactly where the user would otherwise be stuck. */} +
+ + {grouped && step + 1 < specs.length && ( + + )} +
); } diff --git a/surfaces/gui/src/locales/en.json b/surfaces/gui/src/locales/en.json index 25cbc2602..98ca39623 100644 --- a/surfaces/gui/src/locales/en.json +++ b/surfaces/gui/src/locales/en.json @@ -754,6 +754,9 @@ "send_count": "Send ({{count}})", "or_type_answer": "Or type your own answer…", "your_answer": "Your answer…", + "skip": "Skip", + "skip_question": "Skip question", + "skip_all": "Skip all", "grant": "Grant", "grant_no_folder": "Grant (no folder)", "no_folder_suggested": "No folder was suggested", diff --git a/surfaces/gui/src/locales/zh.json b/surfaces/gui/src/locales/zh.json index 65c3389d9..6b3fbf142 100644 --- a/surfaces/gui/src/locales/zh.json +++ b/surfaces/gui/src/locales/zh.json @@ -743,6 +743,9 @@ "send_count": "发送 ({{count}})", "or_type_answer": "或输入你自己的回答…", "your_answer": "你的回答…", + "skip": "跳过", + "skip_question": "跳过此问题", + "skip_all": "全部跳过", "grant": "授权", "grant_no_folder": "授权（无文件夹）", "no_folder_suggested": "未建议文件夹", diff --git a/tests/test_ask_user_upgrades.py b/tests/test_ask_user_upgrades.py index f5a19b859..f142d8080 100644 --- a/tests/test_ask_user_upgrades.py +++ b/tests/test_ask_user_upgrades.py @@ -11,6 +11,8 @@ from coworker.server.manager import SessionManager from coworker.tools.ask import ( MAX_GROUPED_QUESTIONS, + SKIP_NOTE, + SKIP_SENTINEL, answer_result, ask_user_tool, normalize_option, @@ -112,6 +114,50 @@ def test_answer_result_shapes(): assert answer_result(grouped, "") == {"answer": ""} # engine reads this as denied +# -- skipping (OPE-153) ------------------------------------------------------- + + +def test_skipped_single_question_answers_null_not_empty(): + """The whole point of the sentinel: "" (no answer came back) and a skip must not + look alike to the agent.""" + res = answer_result([], SKIP_SENTINEL) + assert res["answer"] is None + assert res["skipped"] is True + assert res["note"] == SKIP_NOTE + assert answer_result([], "") == {"answer": ""} # unchanged, still not a skip + + +def test_skipped_grouped_questions_name_what_was_declined(): + grouped = [{"question": "Chart style?", "header": "Chart"}, {"question": "Colors?"}] + # one step skipped, one answered + res = answer_result(grouped, json.dumps({"Chart": SKIP_SENTINEL, "Colors?": "Blue"})) + assert res["answers"] == {"Chart": None, "Colors?": "Blue"} + assert res["skipped"] == ["Chart"] + assert res["note"] == SKIP_NOTE + # whole card skipped from the card: every question carries the sentinel + res = answer_result(grouped, json.dumps({"Chart": SKIP_SENTINEL, "Colors?": SKIP_SENTINEL})) + assert res["answers"] == {"Chart": None, "Colors?": None} + assert res["skipped"] == ["Chart", "Colors?"] + # a JSON null means the same thing (defensive — a surface may serialize it that way) + assert answer_result(grouped, json.dumps({"Chart": None}))["skipped"] == ["Chart"] + + +def test_bare_sentinel_skips_a_whole_grouped_card(): + """A text-only surface can't send the answer map — the bare sentinel skips everything + rather than being filed as the first question's answer.""" + grouped = [{"question": "Chart style?", "header": "Chart"}, {"question": "Colors?"}] + res = answer_result(grouped, SKIP_SENTINEL) + assert res["answers"] == {"Chart": None, "Colors?": None} + assert res["skipped"] == ["Chart", "Colors?"] + + +def test_fully_answered_card_carries_no_skip_keys(): + """No skip → no `skipped`/`note` noise in the agent's tool result.""" + grouped = [{"question": "Chart style?", "header": "Chart"}] + assert answer_result(grouped, json.dumps({"Chart": "Bar"})) == {"answers": {"Chart": "Bar"}} + assert answer_result([], "staging") == {"answer": "staging"} + + # -- inbox persistence + back-compat ------------------------------------------ @@ -155,10 +201,30 @@ def test_buttons_use_rich_option_labels(tmp_path): options=["staging", {"label": "prod", "description": "the real one"}], ) btns = buttons_for(item) - assert [b.label for b in btns] == ["staging", "prod"] + assert [b.label for b in btns] == ["staging", "prod", "Skip"] assert decode(btns[1].value) == (item.id, "prod") # resolution IS the label +def test_mirrored_question_offers_a_skip_button(tmp_path): + """OPE-153 — the channel gets the same way out the card has. Skip trails the options and + carries the sentinel, which answer_result() turns into a null answer.""" + store = InboxStore(tmp_path / "inbox.json") + item = store.add_question("s1", "Env?", options=["staging", "prod"]) + skip = buttons_for(item)[-1] + assert skip.label == "Skip" + assert decode(skip.value) == (item.id, SKIP_SENTINEL) + # and that resolution is what the agent ends up seeing + assert answer_result(item.questions, SKIP_SENTINEL)["answer"] is None + + +def test_questions_without_options_get_no_buttons(tmp_path): + """A free-text question still mirrors as plain text — no lone Skip button, since the + "(Open the app to respond.)" fallback is the only way to answer it at all.""" + store = InboxStore(tmp_path / "inbox.json") + item = store.add_question("s1", "What should I name it?") + assert buttons_for(item) == [] + + def test_grouped_questions_get_no_buttons(tmp_path): store = InboxStore(tmp_path / "inbox.json") fields = question_item_fields( diff --git a/tests/test_interactions.py b/tests/test_interactions.py index 8b5617d80..45d3f3a7d 100644 --- a/tests/test_interactions.py +++ b/tests/test_interactions.py @@ -5,6 +5,7 @@ from coworker.inbox import InboxStore from coworker.interactions import Button, buttons_for, decode, encode +from coworker.tools.ask import SKIP_SENTINEL from coworker.connectors.base import InteractionEvent from coworker.connectors.senders import _slack_blocks from coworker.providers import ModelCapabilities, ProviderClient @@ -39,8 +40,10 @@ def test_buttons_for_kinds(tmp_path): q = st.add_question("s1", "Which region?", options=["us-east-1", "us-west-2"]) qb = buttons_for(q) - assert [b.label for b in qb] == ["us-east-1", "us-west-2"] + # Skip trails the options (OPE-153) — the channel gets the card's way out too. + assert [b.label for b in qb] == ["us-east-1", "us-west-2", "Skip"] assert decode(qb[0].value) == (q.id, "us-east-1") # resolution IS the option text + assert decode(qb[-1].value) == (q.id, SKIP_SENTINEL) # free-text question (no options) and notifications get no buttons → "open the app" assert buttons_for(st.add_question("s1", "Say something")) == []