Skip to content

fix(ask_user_question): add minLength/maxLength to header JSON Schema #4681

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
phpclub wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(ask_user_question): add minLength/maxLength to header JSON Schema #4681

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions packages/core/src/tools/askUserQuestion.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,24 @@ describe('AskUserQuestionTool', () => {
});
});

describe('JSON Schema', () => {
it('should enforce header maxLength 12 and minLength 1 in the schema', () => {
Copy link
Copy Markdown
Collaborator

@wenshao wenshao Jun 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] Test name "should enforce header maxLength 12 and minLength 1 in the schema" implies enforcement testing, but the test only reads static schema values — it doesn't exercise any validation path. Consider renaming to something like "should declare minLength and maxLength on header schema" to accurately reflect what's being asserted.

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Collaborator

@wenshao wenshao Jun 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] Test name "should enforce header maxLength 12 and minLength 1 in the schema" implies enforcement testing, but the test only reads static schema values — it doesn't exercise any validation path. Consider renaming to something like "should declare header maxLength 12 and minLength 1 in the schema" to accurately describe what is being verified.

— qwen3.7-max via Qwen Code /review

const schema = tool.schema.parametersJsonSchema as Record<
string,
unknown
>;
const questions = (schema['properties'] ?? {}) as Record<string, unknown>;
const items = (questions['questions'] ?? {}) as Record<string, unknown>;
const questionProps = (items['items'] ?? {}) as Record<string, unknown>;
const header = (questionProps['properties'] ?? {}) as Record<
string,
unknown
>;
expect(header['maxLength']).toBe(12);
Copy link
Copy Markdown
Collaborator

@wenshao wenshao Jun 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] Schema traversal stops one level too shallow. The variable header resolves to questionProps['properties'] — the full properties map {question, header, options, multiSelect} — not the header field's own schema. So header['maxLength'] evaluates to undefined, and the test fails:

FAIL  AskUserQuestionTool > JSON Schema > should enforce header maxLength 12 and minLength 1
AssertionError: expected undefined to be 12

Need one more traversal step to reach the actual header schema:

Suggested change
expect(header['maxLength']).toBe(12);
const innerProps = (questionProps['properties'] ?? {}) as Record<
string,
unknown
>;
const headerSchema = (innerProps['header'] ?? {}) as Record<
string,
unknown
>;
expect(headerSchema['maxLength']).toBe(12);
expect(headerSchema['minLength']).toBe(1);

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Collaborator

@wenshao wenshao Jun 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] Schema traversal stops one level too shallow. The variable header resolves to questionProps['properties'] — the full properties map {question, header, options, multiSelect} — not the header field's own schema. So header['maxLength'] returns undefined and the test fails.

Add one more dereference step:

Suggested change
expect(header['maxLength']).toBe(12);
const properties = (questionProps['properties'] ?? {}) as Record<
string,
unknown
>;
const header = (properties['header'] ?? {}) as Record<
string,
unknown
>;
expect(header['maxLength']).toBe(12);
expect(header['minLength']).toBe(1);

— qwen3.7-max via Qwen Code /review

expect(header['minLength']).toBe(1);
});
});

describe('validateToolParams', () => {
it('should accept valid params with single question', () => {
const params = {
Expand Down
4 changes: 3 additions & 1 deletion packages/core/src/tools/askUserQuestion.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,10 @@ const askUserQuestionToolSchemaData: FunctionDeclaration = {
},
header: {
description:
'Very short label displayed as a chip/tag (max 12 chars). Examples: "Auth method", "Library", "Approach".',
'Required: 1-12 characters. Very short label displayed as a chip/tag. Examples: "Auth method", "Library", "Approach".',
type: 'string',
minLength: 1,
maxLength: 12,
},
options: {
description:
Expand Down
Loading