Skip to content

Commit 4f847d1

Browse files
committed
fix: stop reporting expired questions as user dismissals
A question asked through the daemon path expired after 60 seconds. The tool then told the model "User dismissed the question without answering", so the agent claimed the user dismissed a question they were still reading. Transport failures and server shutdown produced the same message. - The question timer becomes a 30-minute lease. It guards against a leak, it is not a network timeout, and a person needs time to read and answer. - Expiry carries the new `question.expired` error code, so the tool tells the model the question was not answered instead of inventing a dismissal. Other failures become real tool errors. - Answers reach the model as the question text and the option labels the user saw, not synthesized ids such as `q_0` and `opt_0_1`. This matches the terminal and ACP surfaces, and repairs MCP elicitation, which reads answers by question text. - Escape no longer dismisses a question. The visible Dismiss button stays. - The web card warns when less than five minutes of the lease remain.
1 parent 4096505 commit 4f847d1

17 files changed

Lines changed: 358 additions & 109 deletions

File tree

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@pymodel/pythinker-code': patch
3+
---
4+
5+
Questions no longer expire after 60 seconds, expired questions are not reported as user dismissals, answers retain question text and option labels, and Escape no longer dismisses a question.

apps/pythinker-web/src/components/QuestionCard.vue

Lines changed: 34 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,22 @@ const total = computed(() => props.question.questions.length);
3030
const hasPreview = computed(() =>
3131
current.value.options.some((option) => option.preview?.trim()),
3232
);
33+
const now = ref(Date.now());
34+
const remainingMinutes = computed(() => {
35+
const expiresAt = Date.parse(props.question.expiresAt);
36+
if (Number.isNaN(expiresAt)) return undefined;
37+
return Math.ceil((expiresAt - now.value) / 60_000);
38+
});
39+
const leaseWarning = computed(() => {
40+
const expiresAt = Date.parse(props.question.expiresAt);
41+
if (Number.isNaN(expiresAt)) return undefined;
42+
const remainingMs = expiresAt - now.value;
43+
if (remainingMs <= 0 || remainingMs > 5 * 60_000) return undefined;
44+
if (remainingMs < 60_000) return t('question.expiresSoonSeconds');
45+
const minutes = remainingMinutes.value;
46+
if (minutes === undefined) return undefined;
47+
return t('question.expiresSoon', { minutes });
48+
});
3349
3450
function goBack(): void {
3551
if (step.value > 0) step.value--;
@@ -207,17 +223,15 @@ function dismiss(): void {
207223
}
208224
209225
// ---------------------------------------------------------------------------
210-
// Keyboard: number keys pick options for current question, Enter submit, Esc dismiss
226+
// Keyboard: number keys pick options for the current question and Enter submits.
211227
// ---------------------------------------------------------------------------
212228
213229
function handleKeydown(e: KeyboardEvent): void {
214230
const tag = (document.activeElement?.tagName ?? '').toLowerCase();
215231
if (tag === 'input' || tag === 'textarea') return;
216-
// While minimized the options aren't visible, so don't let number keys pick
217-
// an unseen answer; only Escape (dismiss) stays live.
218-
if (minimized.value && e.key !== 'Escape') return;
232+
// While minimized the options are not visible, so keyboard selection is disabled.
233+
if (minimized.value) return;
219234
220-
if (e.key === 'Escape') { e.preventDefault(); dismiss(); return; }
221235
if (e.key === 'Enter') { e.preventDefault(); submit(); return; }
222236
223237
const num = parseInt(e.key, 10);
@@ -236,15 +250,27 @@ function handleKeydown(e: KeyboardEvent): void {
236250
}
237251
}
238252
239-
onMounted(() => document.addEventListener('keydown', handleKeydown));
240-
onUnmounted(() => document.removeEventListener('keydown', handleKeydown));
253+
let leaseTimer: ReturnType<typeof setInterval> | undefined;
254+
255+
onMounted(() => {
256+
document.addEventListener('keydown', handleKeydown);
257+
leaseTimer = setInterval(() => {
258+
now.value = Date.now();
259+
}, 30_000);
260+
});
261+
262+
onUnmounted(() => {
263+
document.removeEventListener('keydown', handleKeydown);
264+
if (leaseTimer !== undefined) clearInterval(leaseTimer);
265+
});
241266
</script>
242267

243268
<template>
244269
<div class="qcard" :class="{ minimized }">
245270
<!-- Step indicator (multi-question) -->
246271
<div class="qh">
247272
<span class="qtitle">{{ t('question.title') }}</span>
273+
<span v-if="leaseWarning" class="qexpires">{{ leaseWarning }}</span>
248274
<template v-if="total > 1 && !minimized">
249275
<span class="qstep">{{ t('question.step', { current: step + 1, total }) }}</span>
250276
<button class="qnav" :disabled="step === 0" @click="goBack">{{ t('question.prev') }}</button>
@@ -371,6 +397,7 @@ onUnmounted(() => document.removeEventListener('keydown', handleKeydown));
371397
}
372398
.qtitle { color: var(--blue2); font-weight: 700; }
373399
.qstep { color: var(--muted); font-size: calc(var(--ui-font-size) - 3px); margin-left: 4px; }
400+
.qexpires { color: var(--muted); font-size: calc(var(--ui-font-size) - 3px); margin-left: 4px; }
374401
.qnav {
375402
font-family: var(--mono);
376403
font-size: calc(var(--ui-font-size) - 3px);

apps/pythinker-web/src/composables/usePythinkerWebClient.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1660,6 +1660,7 @@ function toUiQuestion(q: AppQuestionRequest): UIQuestion {
16601660
return {
16611661
questionId: q.questionId,
16621662
sessionId: q.sessionId,
1663+
expiresAt: q.expiresAt,
16631664
questions: q.questions.map((qi) => ({
16641665
id: qi.id,
16651666
question: qi.question,

apps/pythinker-web/src/i18n/locales/en/question.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,4 +10,6 @@ export default {
1010
dismiss: 'Dismiss',
1111
minimize: 'Minimize',
1212
expand: 'Expand',
13+
expiresSoon: 'Expires in {minutes} min',
14+
expiresSoonSeconds: 'Expires in less than a minute',
1315
} as const;

apps/pythinker-web/src/types.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -242,6 +242,7 @@ export interface QueuedPromptView {
242242
export interface UIQuestion {
243243
questionId: string;
244244
sessionId: string;
245+
expiresAt: string;
245246
questions: {
246247
id: string;
247248
question: string;
Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,91 @@
1+
import { mount } from '@vue/test-utils';
2+
import { createI18n } from 'vue-i18n';
3+
import { afterEach, describe, expect, it } from 'vitest';
4+
5+
import QuestionCard from '../src/components/QuestionCard.vue';
6+
import type { UIQuestion } from '../src/types';
7+
8+
const i18n = createI18n({
9+
legacy: false,
10+
locale: 'en',
11+
messages: {
12+
en: {
13+
question: {
14+
title: 'Question',
15+
step: '{current}/{total}',
16+
prev: 'Prev',
17+
next: 'Next',
18+
expand: 'Expand',
19+
minimize: 'Minimize',
20+
otherDefault: 'Other',
21+
submit: 'Submit',
22+
dismiss: 'Dismiss',
23+
notes: 'Notes',
24+
notesPlaceholder: 'Add notes on this option',
25+
expiresSoon: 'Expires in {minutes} min',
26+
expiresSoonSeconds: 'Expires in less than a minute',
27+
},
28+
},
29+
},
30+
missingWarn: false,
31+
fallbackWarn: false,
32+
});
33+
34+
const mounted: ReturnType<typeof mount>[] = [];
35+
36+
function question(expiresAt: string): UIQuestion {
37+
return {
38+
questionId: 'qreq_1',
39+
sessionId: 'sess_1',
40+
expiresAt,
41+
questions: [
42+
{
43+
id: 'q1',
44+
question: 'Pick one',
45+
options: [
46+
{ id: 'a', label: 'A' },
47+
{ id: 'b', label: 'B' },
48+
],
49+
},
50+
],
51+
};
52+
}
53+
54+
function mountCard(input: UIQuestion) {
55+
const wrapper = mount(QuestionCard, {
56+
props: { question: input },
57+
global: {
58+
plugins: [i18n],
59+
stubs: {
60+
Markdown: {
61+
props: ['text'],
62+
template: '<pre class="markdown-stub">{{ text }}</pre>',
63+
},
64+
},
65+
},
66+
});
67+
mounted.push(wrapper);
68+
return wrapper;
69+
}
70+
71+
afterEach(() => {
72+
for (const wrapper of mounted.splice(0)) wrapper.unmount();
73+
});
74+
75+
describe('QuestionCard lifecycle', () => {
76+
it('does not dismiss when Escape is pressed', () => {
77+
const wrapper = mountCard(question(new Date(Date.now() + 20 * 60_000).toISOString()));
78+
79+
document.dispatchEvent(new KeyboardEvent('keydown', { key: 'Escape', bubbles: true }));
80+
81+
expect(wrapper.emitted('dismiss')).toBeUndefined();
82+
});
83+
84+
it('shows an expiry warning within five minutes and hides it at twenty minutes', () => {
85+
const soon = mountCard(question(new Date(Date.now() + 2 * 60_000).toISOString()));
86+
const later = mountCard(question(new Date(Date.now() + 20 * 60_000).toISOString()));
87+
88+
expect(soon.find('.qexpires').text()).toBe('Expires in 2 min');
89+
expect(later.find('.qexpires').exists()).toBe(false);
90+
});
91+
});

apps/pythinker-web/test/question-card-recommended.test.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ function question(overrides: Partial<UIQuestion['questions'][number]> = {}): UIQ
3535
return {
3636
questionId: 'qreq_1',
3737
sessionId: 'sess_1',
38+
expiresAt: new Date(Date.now() + 20 * 60_000).toISOString(),
3839
questions: [
3940
{
4041
id: 'q1',

packages/agent-core/src/errors/codes.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ export const ErrorCodes = {
3333

3434
AGENT_NOT_FOUND: 'agent.not_found',
3535
TURN_AGENT_BUSY: 'turn.agent_busy',
36+
QUESTION_EXPIRED: 'question.expired',
3637

3738
GOAL_ALREADY_EXISTS: 'goal.already_exists',
3839
GOAL_NOT_FOUND: 'goal.not_found',
@@ -229,6 +230,12 @@ export const PYTHINKER_ERROR_INFO = {
229230
public: true,
230231
action: 'Wait for the current turn to finish or steer it.',
231232
},
233+
'question.expired': {
234+
title: 'Question expired',
235+
retryable: false,
236+
public: true,
237+
action: 'Ask the user again if you still need the answer.',
238+
},
232239

233240
'goal.already_exists': {
234241
title: 'A goal is already active',

packages/agent-core/src/services/question/question.ts

Lines changed: 30 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -3,46 +3,37 @@
33
*
44
* **Service interface** (`IQuestionService`): Reverse-RPC one-shot broker
55
* role — routes `QuestionRequest`s coming out of `PythinkerCore` to a waiter
6-
* (web client over WS, mock handler in tests) and resolves the
7-
* promise when the response arrives — or `dismiss()`-es it if the user
8-
* closes the panel (SCHEMAS.md §6.3).
6+
* and resolves the promise when the response arrives or the user dismisses it.
97
*
108
* Role: one-shot broker — see `packages/services/AGENTS.md`. Kept under the
119
* `Service` suffix per the package-wide convention; the broker semantics
1210
* lives in the interface shape (`request` + `resolve` + `dismiss`) and the
1311
* docstring, not in the type name.
1412
*
1513
* **Shape note:** the service returns the in-process
16-
* `QuestionResult = null | QuestionAnswers | QuestionResponse` (see
17-
* `packages/agent-core/src/rpc/sdk-api.ts:48`). SCHEMAS.md §6.2/§6.4 defines
18-
* a protocol-level `QuestionResponse` with a 5-kind discriminated union
19-
* (`single` / `multi` / `other` / `multi_with_other` / `skipped`); the
20-
* protocol↔in-process adapter lives at the daemon boundary, NOT inside the
21-
* service interface. This keeps the SDK side of the adapter untouched and
22-
* confines protocol shape decisions to one place.
14+
* `QuestionResult = null | QuestionAnswers | QuestionResponse`. The
15+
* protocol↔in-process adapter lives at the daemon boundary, not inside the
16+
* service interface.
2317
*
2418
* **Adapter** (`toBrokerRequest` / `toAgentCoreResponse` / `dismissedResult`):
25-
* Bridges two representations of the same question interaction:
19+
* Bridges two representations of the same question interaction. Protocol ids
20+
* go in; question text and option labels that the user saw come out. This is
21+
* the only protocol↔SDK translation site for questions:
2622
*
2723
* 1. **In-process SDK shape** (agent-core, camelCase) — what
28-
* `BridgeClientAPI` sees from `PythinkerCore.requestQuestion(...)`. See
29-
* `packages/agent-core/src/rpc/sdk-api.ts:50-54`:
24+
* `BridgeClientAPI` sees from `PythinkerCore.requestQuestion(...)`:
3025
* `QuestionRequest { turnId?, toolCallId?, questions: QuestionItem[] }`
3126
* where `QuestionItem` has `question, header?, body?, options[],
3227
* multiSelect?, allowOther?, otherLabel?, otherDescription?`.
3328
* `QuestionResult = null | QuestionAnswers | QuestionResponse`,
3429
* `QuestionAnswers = Record<string, string | true>`.
3530
*
36-
* 2. **Protocol wire shape** (snake_case, with daemon-allocated metadata) —
37-
* defined in `packages/protocol/src/question.ts`. 5-kind discriminated
38-
* union for answers: `single | multi | other | multi_with_other | skipped`.
31+
* 2. **Protocol wire shape** (snake_case, with daemon-allocated metadata).
3932
*
4033
* **Synthesizing stable ids** (SDK has no per-item / per-option `id`):
4134
* - `QuestionItem.id` ← `q_<index>` (e.g. `q_0`, `q_1`, ...)
4235
* - `QuestionOption.id` ← `opt_<parent_idx>_<option_idx>` (e.g. `opt_0_0`)
4336
*
44-
* **Anti-corruption**: this is the ONLY place protocol↔SDK shape translation
45-
* happens for question.
4637
*/
4738

4839
import { createDecorator } from '../../di';
@@ -104,14 +95,14 @@ export interface QuestionToBrokerRequestParams {
10495
readonly sessionId: string;
10596
/** `createdAt` ISO string; broker passes `new Date().toISOString()`. */
10697
readonly createdAt: string;
107-
/** `expiresAt` ISO string; broker computes `createdAt + 60s`. */
98+
/** `expiresAt` ISO string; broker computes the lease deadline. */
10899
readonly expiresAt: string;
109100
}
110101

111102
/**
112103
* Build a protocol option from an SDK option. SDK has only `label?:string` +
113104
* `description?:string`; we synthesize `id` from parent and child indices so
114-
* `toAgentCoreAnswers` can map back through `Record<qid, string>`.
105+
* the response adapter can map answers back to the question text and labels.
115106
*/
116107
function buildOption(
117108
opt: {
@@ -134,7 +125,7 @@ function buildOption(
134125

135126
/**
136127
* Build a protocol question item from an SDK item + its position. The
137-
* synthesized `id` (`q_<parentIdx>`) is the key the SDK answers Record uses.
128+
* synthesized `id` (`q_<parentIdx>`) identifies the matching response item.
138129
*/
139130
function buildItem(
140131
item: InProcessQuestionItem,
@@ -178,36 +169,42 @@ export function toBrokerRequest(
178169
}
179170

180171
/**
181-
* Protocol REST response body → in-process SDK `QuestionResponse` (with
182-
* `answers` flattened to `Record<string, string | true>`).
172+
* Protocol response ids + the original request → in-process SDK
173+
* `QuestionResponse` with answers flattened to `Record<string, string | true>`.
183174
*
184-
* Normalization rules from SCHEMAS §6.4:
185-
* - single → option_id
186-
* - multi → option_ids.join(',')
175+
* The original request is the lookup for the text and labels displayed to the
176+
* user:
177+
* - single → option label
178+
* - multi → option labels joined with `, `
187179
* - other → text
188-
* - multi_with_other → [...option_ids, other_text].join(',')
180+
* - multi_with_other → option labels and text joined with `, `
189181
* - skipped → OMIT entry
190182
*/
191183
export function toAgentCoreResponse(
192184
resp: ProtocolQuestionResponse,
185+
request: ProtocolQuestionRequest,
193186
): InProcessQuestionResponse {
194187
const flattened: InProcessQuestionAnswers = {};
195188
for (const [qid, ans] of Object.entries(resp.answers)) {
189+
const item = request.questions.find((question) => question.id === qid);
190+
const question = item?.question ?? qid;
191+
const optionLabel = (id: string): string =>
192+
item?.options.find((option) => option.id === id)?.label ?? id;
196193
switch (ans.kind) {
197194
case 'single':
198-
flattened[qid] = ans.option_id;
195+
flattened[question] = optionLabel(ans.option_id);
199196
break;
200197
case 'multi':
201-
flattened[qid] = ans.option_ids.join(',');
198+
flattened[question] = ans.option_ids.map(optionLabel).join(', ');
202199
break;
203200
case 'other':
204-
flattened[qid] = ans.text;
201+
flattened[question] = ans.text;
205202
break;
206203
case 'multi_with_other':
207-
flattened[qid] = [...ans.option_ids, ans.other_text].join(',');
204+
flattened[question] = [...ans.option_ids.map(optionLabel), ans.other_text].join(', ');
208205
break;
209206
case 'skipped':
210-
// Omitted from the record — matches SCHEMAS §6.4 ("if skipped continue").
207+
// Omitted from the record.
211208
break;
212209
default: {
213210
// Defensive: never-reached if Zod schema is the SOT, but TS narrowing
@@ -219,7 +216,7 @@ export function toAgentCoreResponse(
219216
}
220217
const out: InProcessQuestionResponse = { answers: flattened };
221218
if (resp.method !== undefined) {
222-
// SCHEMAS §6.2 protocol allows 'click' as a method; agent-core's in-process
219+
// Protocol allows 'click' as a method; agent-core's in-process
223220
// `QuestionAnswerMethod` is `'enter' | 'space' | 'number_key'` (NO 'click').
224221
// Drop 'click' on the in-process side to preserve type safety; the wire
225222
// form keeps it for clients that want to surface the affordance used.

0 commit comments

Comments
 (0)