feat(agent-core): feed AskUserQuestion answers back as question text and option labels - #1414
Conversation
…and option labels The flattened answers record the model receives was keyed by synthesized ids (q_0 / opt_0_1), forcing a cross-message positional lookup against the original tool call to understand what the user picked — both unreadable in transcripts and a real model-misreads-the-choice badcase. - toAgentCoreResponse now takes the original broker request and translates wire ids back to question text (keys) and option labels (values); unknown ids are kept verbatim, missing request falls back to raw ids - wire protocol unchanged: clients still answer with option ids; the resolve route reads the pending request before settling it - question texts must be unique per call and option labels unique per question, enforced in the tool execution path (AJV cannot express the zod refine) and mirrored on the exported schemas - web transcript card resolves both the new label form and legacy id transcripts; TUI and ACP paths already produced the text form
🦋 Changeset detectedLatest commit: 10fa5be The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12df36f9b1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| break; | ||
| case 'multi': | ||
| flattened[qid] = ans.option_ids.join(','); | ||
| flattened[key] = ans.option_ids.map(optionText).join(','); |
There was a problem hiding this comment.
Reject comma-bearing labels for multi-select answers
When an AskUserQuestion is multi_select and any selected option label contains a comma, this comma-join makes the model-facing answer ambiguous; e.g. selecting Fast, risky and Safe emits Fast, risky,Safe, and the web transcript resolver splits on commas so it displays Fast, risky as Other text rather than the selected option. The new uniqueness check still allows such labels, so either reject/escape comma-bearing labels for multi-select and multi_with_other, or use an unambiguous value shape.
Useful? React with 👍 / 👎.
|
|
||
| const flattened: InProcessQuestionAnswers = {}; | ||
| for (const [qid, ans] of Object.entries(resp.answers)) { | ||
| const key = itemsById.get(qid)?.question ?? qid; |
There was a problem hiding this comment.
Enforce unique question text at the broker boundary
When requestQuestion reaches the broker from anything other than AskUserQuestionTool, the QuestionRequest/IQuestionService boundary still accepts duplicate question strings, and both qids then map to the same flattened key here. Resolving q_0 and q_1 for two same-text questions silently overwrites one answer, so the uniqueness contract needs to be enforced in the broker/adapter path as well or this translation needs a disambiguating fallback for duplicates.
Useful? React with 👍 / 👎.
…rden question schema
- Join multi-select labels with ', ' in the server translator, matching
what the TUI reverse-RPC path already emits, so the model sees one
format regardless of which client answered
- Trim segments in the web transcript resolver before label matching:
TUI-answered multi-select transcripts (', '-joined) previously lost
their highlight to a spurious leading-space Other row
- Move the question-text/legacy-q_<i> answer lookup out of the SFC into
askUserToolParse as answerFor(), per that module's testability intent
- Require non-empty question text and option labels (.min(1)) so empty
strings are rejected by AJV at the tool boundary instead of failing
deeper in the protocol layer
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: afdf84fc7b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| itemsById.set(item.id, item); | ||
| for (const opt of item.options) labelsByOptionId.set(opt.id, opt.label); | ||
| } | ||
| const optionText = (id: string): string => labelsByOptionId.get(id) ?? id; |
There was a problem hiding this comment.
Reject cross-question option ids
When a client submits a stale or malformed response that pairs one question id with an option id from another question (for example q_1 with opt_0_0), the REST schema accepts the shape and this global lookup translates opt_0_0 to question 0's label before storing it under question 1's text. That gives the model a label that was never offered for that question instead of preserving the raw id for diagnosability, so the lookup should be scoped to the answered qid or reject cross-question ids.
Useful? React with 👍 / 👎.
The translator's option-id lookup was a single flat map across all questions, so a stale or malformed response pairing one question with another question's option id (q_1 + opt_0_0) was silently translated into a label that was never offered for that question. Scope the lookup to the answered question's own options; cross-question and unknown ids now both pass through verbatim, staying diagnosable.
Problem
The flattened
answersrecord fed back to the model was keyed by synthesized ids:{"answers":{"q_0":"opt_0_1"}}To understand what the user picked, the model had to map positional ids back to the original tool-call arguments across messages — unreadable in transcripts, and an observed badcase where the model misread the user's choice and went down the wrong path.
Change
After this PR the tool result is self-explanatory (all client paths now produce the same shape):
{"answers":{"Which database?":"Postgres"}}toAgentCoreResponsetranslates ids back to text: it now receives the original broker request and mapsq_<i>→ question text,opt_<q>_<o>→ option label (comma-joined for multi-select; Other stays free text). Unknown ids are kept verbatim for diagnosability; a missing request falls back to the old raw-id behavior.packages/protocolis untouched.opt_<q>_<o>decode so old session transcripts still render.Tests
Full suite: 9253 passed; the 6 failures in
test/cli/update/preflight.test.ts(rollout gating) reproduce on a clean checkout and are unrelated (time-dependent baseline).