mirror of
https://github.com/wu736139669/hapi.git
synced 2026-08-08 07:17:39 +00:00
fix(codex): bridge MCP elicitation through user input (#1008)
* fix(codex): bridge MCP elicitation through user input * fix(codex): allow MCP elicitation in yolo mode * fix(codex): preserve MCP form semantics * fix(codex): accept implicit MCP form mode * fix(codex): harden MCP elicitation prompts * fix(codex): require valid MCP choice answers * fix(codex): round-trip MCP array elicitation * fix(web): require explicit MCP URL confirmation * fix(codex): preserve MCP array item types * fix(codex): support multi-select MCP elicitation * fix(codex): allow MCP elicitation in read-only mode * fix(codex): route MCP tool approvals through permissions
This commit is contained in:
@@ -8,6 +8,9 @@ import {
|
||||
isRequestUserInputToolName,
|
||||
parseRequestUserInputInput,
|
||||
formatRequestUserInputAnswers,
|
||||
isRequestUserInputQuestionAnswered,
|
||||
isRequestUserInputUrlConfirmed,
|
||||
openRequestUserInputUrl,
|
||||
type RequestUserInputQuestion
|
||||
} from '@/components/ToolCard/requestUserInput'
|
||||
import { cn } from '@/lib/utils'
|
||||
@@ -57,7 +60,7 @@ function OptionRow(props: {
|
||||
}
|
||||
|
||||
type QuestionState = {
|
||||
selected: string | null
|
||||
selected: string[]
|
||||
userNote: string
|
||||
}
|
||||
|
||||
@@ -84,7 +87,7 @@ export function RequestUserInputFooter(props: {
|
||||
setStep(0)
|
||||
const initial: Record<string, QuestionState> = {}
|
||||
for (const q of questions) {
|
||||
initial[q.id] = { selected: null, userNote: '' }
|
||||
initial[q.id] = { selected: [], userNote: '' }
|
||||
}
|
||||
setStateByQuestion(initial)
|
||||
setLoading(false)
|
||||
@@ -112,16 +115,7 @@ export function RequestUserInputFooter(props: {
|
||||
const currentQuestion = questions[clampedStep] as RequestUserInputQuestion | undefined
|
||||
|
||||
const validateQuestion = (question: RequestUserInputQuestion): boolean => {
|
||||
const state = stateByQuestion[question.id]
|
||||
if (!state) return false
|
||||
|
||||
// For questions with options, require a selection OR user note
|
||||
if (question.options.length > 0) {
|
||||
return state.selected !== null || state.userNote.trim().length > 0
|
||||
}
|
||||
|
||||
// For pure text questions (no options), require user note
|
||||
return state.userNote.trim().length > 0
|
||||
return isRequestUserInputQuestionAnswered(question, stateByQuestion[question.id])
|
||||
}
|
||||
|
||||
const submit = async () => {
|
||||
@@ -140,6 +134,18 @@ export function RequestUserInputFooter(props: {
|
||||
// Format answers for submission
|
||||
const formattedAnswers = formatRequestUserInputAnswers(stateByQuestion)
|
||||
|
||||
if (parsed.url) {
|
||||
if (!isRequestUserInputUrlConfirmed(parsed, stateByQuestion)) {
|
||||
setError(t('tool.selectOption'))
|
||||
return
|
||||
}
|
||||
if (!openRequestUserInputUrl(parsed.url)) {
|
||||
setError(t('tool.requestUserInput.popupBlocked'))
|
||||
haptic.notification('error')
|
||||
return
|
||||
}
|
||||
}
|
||||
|
||||
setLoading(true)
|
||||
await run(() => props.api.approvePermission(props.sessionId, permission.id, formattedAnswers), 'success')
|
||||
setLoading(false)
|
||||
@@ -160,13 +166,17 @@ export function RequestUserInputFooter(props: {
|
||||
setStep((s) => Math.max(s - 1, 0))
|
||||
}
|
||||
|
||||
const selectOption = (questionId: string, optionLabel: string) => {
|
||||
const selectOption = (question: RequestUserInputQuestion, optionLabel: string) => {
|
||||
haptic.selection()
|
||||
setStateByQuestion((prev) => ({
|
||||
...prev,
|
||||
[questionId]: {
|
||||
...prev[questionId],
|
||||
selected: optionLabel
|
||||
[question.id]: {
|
||||
...prev[question.id],
|
||||
selected: question.multiple
|
||||
? prev[question.id]?.selected.includes(optionLabel)
|
||||
? prev[question.id].selected.filter((value) => value !== optionLabel)
|
||||
: [...(prev[question.id]?.selected ?? []), optionLabel]
|
||||
: [optionLabel]
|
||||
}
|
||||
}))
|
||||
}
|
||||
@@ -227,7 +237,7 @@ export function RequestUserInputFooter(props: {
|
||||
<>
|
||||
<div className="mt-3 flex flex-col gap-1">
|
||||
{currentQuestion.options.map((opt, optIdx) => {
|
||||
const isSelected = currentState?.selected === opt.label
|
||||
const isSelected = currentState?.selected.includes(opt.label) ?? false
|
||||
return (
|
||||
<OptionRow
|
||||
key={optIdx}
|
||||
@@ -235,7 +245,7 @@ export function RequestUserInputFooter(props: {
|
||||
disabled={props.disabled || loading}
|
||||
title={opt.label}
|
||||
description={opt.description}
|
||||
onClick={() => selectOption(currentQuestion.id, opt.label)}
|
||||
onClick={() => selectOption(currentQuestion, opt.label)}
|
||||
/>
|
||||
)
|
||||
})}
|
||||
|
||||
@@ -215,3 +215,45 @@ describe('getToolPresentation — Codex agent tools', () => {
|
||||
expect(presentation.minimal).toBe(true)
|
||||
})
|
||||
})
|
||||
|
||||
describe('getToolPresentation — request_user_input', () => {
|
||||
it('uses the question header instead of exposing its protocol id', () => {
|
||||
const presentation = getToolPresentation({
|
||||
toolName: 'request_user_input',
|
||||
input: {
|
||||
questions: [{
|
||||
id: '__mcp_url_confirmation',
|
||||
header: 'Sign in',
|
||||
question: 'Sign in to continue'
|
||||
}]
|
||||
},
|
||||
result: null,
|
||||
childrenCount: 0,
|
||||
description: null,
|
||||
metadata: null,
|
||||
})
|
||||
|
||||
expect(presentation.title).toBe('Sign in')
|
||||
expect(presentation.title).not.toContain('__mcp_url_confirmation')
|
||||
expect(presentation.subtitle).toBe('Sign in to continue')
|
||||
})
|
||||
|
||||
it('falls back to Question rather than exposing an id when no header is present', () => {
|
||||
const presentation = getToolPresentation({
|
||||
toolName: 'request_user_input',
|
||||
input: {
|
||||
questions: [{
|
||||
id: '__mcp_form_confirmation',
|
||||
question: 'Continue?'
|
||||
}]
|
||||
},
|
||||
result: null,
|
||||
childrenCount: 0,
|
||||
description: null,
|
||||
metadata: null,
|
||||
})
|
||||
|
||||
expect(presentation.title).toBe('Question')
|
||||
expect(presentation.subtitle).toBe('Continue?')
|
||||
})
|
||||
})
|
||||
|
||||
@@ -508,13 +508,13 @@ export const knownTools: Record<string, {
|
||||
? opts.input.questions : []
|
||||
const count = questions.length
|
||||
const first = questions[0] ?? null
|
||||
const id = isObject(first) && typeof first.id === 'string'
|
||||
? first.id.trim() : ''
|
||||
const header = isObject(first) && typeof first.header === 'string'
|
||||
? first.header.trim() : ''
|
||||
|
||||
if (count > 1) {
|
||||
return `${count} Questions`
|
||||
}
|
||||
return id.length > 0 ? id : 'Question'
|
||||
return header.length > 0 ? header : 'Question'
|
||||
},
|
||||
subtitle: (opts) => {
|
||||
const questions = isObject(opts.input) && Array.isArray(opts.input.questions)
|
||||
|
||||
@@ -0,0 +1,122 @@
|
||||
import { afterEach, describe, expect, it, vi } from 'vitest'
|
||||
import {
|
||||
isRequestUserInputQuestionAnswered,
|
||||
isRequestUserInputUrlConfirmed,
|
||||
formatRequestUserInputAnswers,
|
||||
openRequestUserInputUrl,
|
||||
parseRequestUserInputInput
|
||||
} from './requestUserInput'
|
||||
|
||||
describe('MCP URL request user input', () => {
|
||||
afterEach(() => vi.restoreAllMocks())
|
||||
|
||||
it('only exposes http(s) URLs to the approval UI', () => {
|
||||
expect(parseRequestUserInputInput({ url: 'https://example.com/login', questions: [] }).url)
|
||||
.toBe('https://example.com/login')
|
||||
expect(parseRequestUserInputInput({ url: 'javascript:alert(1)', questions: [] }).url)
|
||||
.toBeNull()
|
||||
})
|
||||
|
||||
it('reports popup failures instead of treating the URL as opened', () => {
|
||||
const open = vi.spyOn(window, 'open').mockReturnValue(null)
|
||||
expect(openRequestUserInputUrl('https://example.com/login')).toBe(false)
|
||||
expect(open).toHaveBeenCalledWith('about:blank', '_blank')
|
||||
})
|
||||
|
||||
it('severs opener access before navigating to an external MCP URL', () => {
|
||||
const replace = vi.fn()
|
||||
const popup = {
|
||||
opener: window,
|
||||
location: { replace }
|
||||
} as unknown as Window
|
||||
const open = vi.spyOn(window, 'open').mockReturnValue(popup)
|
||||
|
||||
expect(openRequestUserInputUrl('https://example.com/login')).toBe(true)
|
||||
expect(open).toHaveBeenCalledWith('about:blank', '_blank')
|
||||
expect(popup.opener).toBeNull()
|
||||
expect(replace).toHaveBeenCalledWith('https://example.com/login')
|
||||
})
|
||||
|
||||
it('preserves optional form questions and allows them to stay empty', () => {
|
||||
const parsed = parseRequestUserInputInput({
|
||||
questions: [{ id: 'comment', question: 'Comment', required: false, options: [] }]
|
||||
})
|
||||
|
||||
expect(parsed.questions[0]).toEqual({
|
||||
id: 'comment',
|
||||
question: 'Comment',
|
||||
required: false,
|
||||
multiple: false,
|
||||
options: []
|
||||
})
|
||||
expect(isRequestUserInputQuestionAnswered(parsed.questions[0]!, {
|
||||
selected: [],
|
||||
userNote: ''
|
||||
})).toBe(true)
|
||||
})
|
||||
|
||||
it('requires an actual selection for required choice questions', () => {
|
||||
const question = parseRequestUserInputInput({
|
||||
questions: [{
|
||||
id: 'approved',
|
||||
question: 'Approved?',
|
||||
required: true,
|
||||
options: [{ label: 'true', description: '' }, { label: 'false', description: '' }]
|
||||
}]
|
||||
}).questions[0]!
|
||||
|
||||
expect(isRequestUserInputQuestionAnswered(question, {
|
||||
selected: [],
|
||||
userNote: 'please approve'
|
||||
})).toBe(false)
|
||||
expect(isRequestUserInputQuestionAnswered(question, {
|
||||
selected: ['true'],
|
||||
userNote: 'please approve'
|
||||
})).toBe(true)
|
||||
})
|
||||
|
||||
it('opens an MCP URL only after selecting its explicit confirmation option', () => {
|
||||
const url = 'https://example.com/login'
|
||||
const hidden = parseRequestUserInputInput({
|
||||
url,
|
||||
questions: [{ id: 'unrelated', question: 'Continue?', options: [] }]
|
||||
})
|
||||
expect(isRequestUserInputUrlConfirmed(hidden, {
|
||||
unrelated: { selected: [], userNote: 'yes' }
|
||||
})).toBe(false)
|
||||
|
||||
const explicit = parseRequestUserInputInput({
|
||||
url,
|
||||
questions: [{
|
||||
id: '__mcp_url_confirmation',
|
||||
question: 'Sign in',
|
||||
options: [{ label: 'Open', description: url }]
|
||||
}]
|
||||
})
|
||||
expect(isRequestUserInputUrlConfirmed(explicit, {
|
||||
__mcp_url_confirmation: { selected: [], userNote: '' }
|
||||
})).toBe(false)
|
||||
expect(isRequestUserInputUrlConfirmed(explicit, {
|
||||
__mcp_url_confirmation: { selected: ['Open'], userNote: '' }
|
||||
})).toBe(true)
|
||||
})
|
||||
|
||||
it('serializes every selected value for multiple-choice questions', () => {
|
||||
const parsed = parseRequestUserInputInput({
|
||||
questions: [{
|
||||
id: 'tags',
|
||||
question: 'Tags',
|
||||
required: true,
|
||||
multiple: true,
|
||||
options: [{ label: 'bug' }, { label: 'feature' }]
|
||||
}]
|
||||
})
|
||||
|
||||
expect(parsed.questions[0]?.multiple).toBe(true)
|
||||
expect(formatRequestUserInputAnswers({
|
||||
tags: { selected: ['bug', 'feature'], userNote: '' }
|
||||
})).toEqual({
|
||||
answers: { tags: { answers: ['bug', 'feature'] } }
|
||||
})
|
||||
})
|
||||
})
|
||||
@@ -8,9 +8,21 @@ export type RequestUserInputOption = {
|
||||
export type RequestUserInputQuestion = {
|
||||
id: string
|
||||
question: string
|
||||
required: boolean
|
||||
multiple: boolean
|
||||
options: RequestUserInputOption[]
|
||||
}
|
||||
|
||||
export type RequestUserInputQuestionAnswer = {
|
||||
selected: string[]
|
||||
userNote: string
|
||||
}
|
||||
|
||||
export type ParsedRequestUserInput = {
|
||||
questions: RequestUserInputQuestion[]
|
||||
url: string | null
|
||||
}
|
||||
|
||||
export type RequestUserInputQuestionInfo = {
|
||||
id: string
|
||||
question: string | null
|
||||
@@ -23,11 +35,34 @@ export function isRequestUserInputToolName(toolName: string): boolean {
|
||||
return toolName === 'request_user_input'
|
||||
}
|
||||
|
||||
export function parseRequestUserInputInput(input: unknown): { questions: RequestUserInputQuestion[] } {
|
||||
if (!isObject(input)) return { questions: [] }
|
||||
export function openRequestUserInputUrl(url: string): boolean {
|
||||
const opened = window.open('about:blank', '_blank')
|
||||
if (!opened) return false
|
||||
try {
|
||||
opened.opener = null
|
||||
opened.location.replace(url)
|
||||
return true
|
||||
} catch {
|
||||
opened.close()
|
||||
return false
|
||||
}
|
||||
}
|
||||
|
||||
export function parseRequestUserInputInput(input: unknown): ParsedRequestUserInput {
|
||||
if (!isObject(input)) return { questions: [], url: null }
|
||||
|
||||
let url: string | null = null
|
||||
if (typeof input.url === 'string') {
|
||||
try {
|
||||
const parsed = new URL(input.url)
|
||||
if (parsed.protocol === 'https:' || parsed.protocol === 'http:') url = parsed.toString()
|
||||
} catch {
|
||||
// Invalid and non-web URLs must never be opened by the approval UI.
|
||||
}
|
||||
}
|
||||
|
||||
const rawQuestions = input.questions
|
||||
if (!Array.isArray(rawQuestions)) return { questions: [] }
|
||||
if (!Array.isArray(rawQuestions)) return { questions: [], url }
|
||||
|
||||
const questions: RequestUserInputQuestion[] = []
|
||||
for (const raw of rawQuestions) {
|
||||
@@ -52,11 +87,39 @@ export function parseRequestUserInputInput(input: unknown): { questions: Request
|
||||
questions.push({
|
||||
id,
|
||||
question,
|
||||
required: raw.required !== false,
|
||||
multiple: raw.multiple === true,
|
||||
options
|
||||
})
|
||||
}
|
||||
|
||||
return { questions }
|
||||
return { questions, url }
|
||||
}
|
||||
|
||||
export function isRequestUserInputUrlConfirmed(
|
||||
parsed: ParsedRequestUserInput,
|
||||
answersByQuestion: Record<string, RequestUserInputQuestionAnswer>
|
||||
): boolean {
|
||||
if (!parsed.url) return false
|
||||
return parsed.questions.some((question) => {
|
||||
if (question.id !== '__mcp_url_confirmation') return false
|
||||
const selected = answersByQuestion[question.id]?.selected ?? []
|
||||
return question.options.some((option) => (
|
||||
selected.includes(option.label) && option.description === parsed.url
|
||||
))
|
||||
})
|
||||
}
|
||||
|
||||
export function isRequestUserInputQuestionAnswered(
|
||||
question: RequestUserInputQuestion,
|
||||
answer: RequestUserInputQuestionAnswer | undefined
|
||||
): boolean {
|
||||
if (!question.required) return true
|
||||
if (!answer) return false
|
||||
if (question.options.length > 0) {
|
||||
return answer.selected.length > 0
|
||||
}
|
||||
return answer.userNote.trim().length > 0
|
||||
}
|
||||
|
||||
export function extractRequestUserInputQuestionsInfo(input: unknown): RequestUserInputQuestionInfo[] | null {
|
||||
@@ -83,16 +146,14 @@ export function extractRequestUserInputQuestionsInfo(input: unknown): RequestUse
|
||||
* Format: { answers: { [id]: { answers: ["option", "user_note: note text"] } } }
|
||||
*/
|
||||
export function formatRequestUserInputAnswers(
|
||||
answersByQuestion: Record<string, { selected: string | null; userNote: string }>
|
||||
answersByQuestion: Record<string, RequestUserInputQuestionAnswer>
|
||||
): { answers: RequestUserInputAnswers } {
|
||||
const answers: RequestUserInputAnswers = {}
|
||||
|
||||
for (const [id, answer] of Object.entries(answersByQuestion)) {
|
||||
const answerArray: string[] = []
|
||||
|
||||
if (answer.selected) {
|
||||
answerArray.push(answer.selected)
|
||||
}
|
||||
answerArray.push(...answer.selected)
|
||||
|
||||
const note = answer.userNote.trim()
|
||||
if (note.length > 0) {
|
||||
@@ -110,13 +171,13 @@ export function formatRequestUserInputAnswers(
|
||||
*/
|
||||
export function parseRequestUserInputAnswers(
|
||||
answers: unknown
|
||||
): Record<string, { selected: string | null; userNote: string | null }> | null {
|
||||
): Record<string, { selected: string[]; userNote: string | null }> | null {
|
||||
if (!isObject(answers)) return null
|
||||
|
||||
// Handle nested format: { answers: { [id]: { answers: string[] } } }
|
||||
const answersObj = isObject(answers.answers) ? answers.answers : answers
|
||||
|
||||
const parsed: Record<string, { selected: string | null; userNote: string | null }> = {}
|
||||
const parsed: Record<string, { selected: string[]; userNote: string | null }> = {}
|
||||
|
||||
for (const [id, value] of Object.entries(answersObj)) {
|
||||
let answerArray: string[] = []
|
||||
@@ -127,15 +188,15 @@ export function parseRequestUserInputAnswers(
|
||||
answerArray = value.filter((a): a is string => typeof a === 'string')
|
||||
}
|
||||
|
||||
let selected: string | null = null
|
||||
const selected: string[] = []
|
||||
let userNote: string | null = null
|
||||
|
||||
for (const item of answerArray) {
|
||||
if (item.startsWith('user_note: ')) {
|
||||
userNote = item.slice('user_note: '.length).trim()
|
||||
} else if (!selected) {
|
||||
} else {
|
||||
// Trim to match option labels which are also trimmed
|
||||
selected = item.trim()
|
||||
selected.push(item.trim())
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -68,7 +68,7 @@ export function RequestUserInputView(props: ToolViewProps) {
|
||||
// Question with options
|
||||
<div className="mt-3 flex flex-col gap-1">
|
||||
{q.options.map((opt, optIdx) => {
|
||||
const isSelected = answer?.selected === opt.label
|
||||
const isSelected = answer?.selected.includes(opt.label) ?? false
|
||||
|
||||
return (
|
||||
<div
|
||||
|
||||
@@ -408,6 +408,7 @@ export default {
|
||||
'tool.requestUserInput.textPlaceholder': 'Type your answer…',
|
||||
'tool.requestUserInput.noteLabel': 'Additional note (optional)',
|
||||
'tool.requestUserInput.notePlaceholder': 'Add a note…',
|
||||
'tool.requestUserInput.popupBlocked': 'Could not open the sign-in page. Allow popups and try again.',
|
||||
'toolGroup.title': 'Tool activity',
|
||||
'toolGroup.primary.fileTargets': '{target} +{n}',
|
||||
'toolGroup.primary.commandTargets': '{target} +{n}',
|
||||
|
||||
@@ -412,6 +412,7 @@ export default {
|
||||
'tool.requestUserInput.textPlaceholder': '输入您的答案…',
|
||||
'tool.requestUserInput.noteLabel': '补充说明(可选)',
|
||||
'tool.requestUserInput.notePlaceholder': '添加备注…',
|
||||
'tool.requestUserInput.popupBlocked': '无法打开登录页面。请允许弹出窗口后重试。',
|
||||
'toolGroup.title': '工具活动',
|
||||
'toolGroup.primary.fileTargets': '{target} 等 +{n}',
|
||||
'toolGroup.primary.commandTargets': '{target} 等 +{n}',
|
||||
|
||||
Reference in New Issue
Block a user