Skip to content

fix: correct hooks JSON schema type definition#2280

Open
xuewenjie123 wants to merge 2 commits intoQwenLM:mainfrom
xuewenjie123:fix/hooks-json-schema-type
Open

fix: correct hooks JSON schema type definition#2280
xuewenjie123 wants to merge 2 commits intoQwenLM:mainfrom
xuewenjie123:fix/hooks-json-schema-type

Conversation

@xuewenjie123
Copy link
Collaborator

Pull Request Description

English Version

Fix: Correct hooks JSON schema type definition

Issue: #2246

Problem

When configuring Hooks in settings.json, VS Code shows type errors for HookDefinition objects under hooks.UserPromptSubmit and hooks.Stop:

Type is incorrect. Expected "string"

This is because settings.schema.json incorrectly defines the array element type as string for these hook events, while the actual runtime expects HookDefinition object structures.

Impact

  • VS Code IntelliSense: Users cannot get proper auto-completion and type hints when configuring hooks
  • Schema Validation: Correct HookDefinition object configurations are marked as type errors, causing confusion
  • Runtime: No impact - runtime parsing doesn't depend on JSON Schema, functionality works correctly

Solution

This PR adds proper schema support for complex array item types:

  1. New Interface: Added SettingItemDefinition interface to define schema for array item types, supporting simple types (string, number, boolean) and complex object types

  2. Schema Definition: Added complete items schema for UserPromptSubmit and Stop hooks with full HookDefinition structure:

    • matcher: Optional matcher pattern
    • sequential: Whether to execute hooks sequentially
    • hooks: Array of hook configurations (required)
      • type: Hook type (required, currently only 'command')
      • command: Command to execute (required)
      • name, description, timeout, env: Optional fields
  3. Generator Update: Updated generate-settings-schema.ts with convertItemDefinitionToJsonSchema function to convert complex item definitions to standard JSON Schema format

Files Changed

File Change
packages/cli/src/config/settingsSchema.ts Added SettingItemDefinition interface and items schema for hooks
packages/vscode-ide-companion/schemas/settings.schema.json Updated hooks items type from string to proper object schema
scripts/generate-settings-schema.ts Added conversion function for complex item types

Testing

  • VS Code no longer shows type errors for valid hook configurations
  • Auto-completion works correctly for hook properties
  • Required fields validation works properly
  • npm run generate-settings-schema generates correct schema

Screenshots

Before Fix

Image ### After Fix image

The hooks array items were incorrectly typed as 'string' in the JSON
schema, causing VS Code to show type errors when users configure
HookDefinition objects. This fix adds proper schema support for complex
array item types.

- Add SettingItemDefinition interface for array item schema
- Add items schema for UserPromptSubmit and Stop hooks
- Update generate-settings-schema.ts to convert complex item types

Fixes QwenLM#2246

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Extract common hook definition items schema into a reusable constant
to avoid code duplication between UserPromptSubmit and Stop hooks.

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
@github-actions
Copy link
Contributor

📋 Review Summary

This PR fixes an important JSON schema issue where VS Code was incorrectly showing type errors for hook configurations in settings.json. The fix adds proper schema support for complex array item types, enabling correct IntelliSense and validation. Overall, this is a well-structured fix that addresses a real user-facing problem, but there is one critical bug that must be fixed before merging.

🔍 General Feedback

  • The PR correctly identifies and addresses the root cause of the VS Code type errors
  • Good separation of concerns with a new SettingItemDefinition interface for reusable array item schemas
  • The addition of the convertItemDefinitionToJsonSchema function is a clean solution for handling complex nested types
  • Using a shared HOOK_DEFINITION_ITEMS constant avoids duplication between UserPromptSubmit and Stop hooks
  • All existing tests pass, and the type check passes successfully

🎯 Specific Feedback

🔴 Critical

  • File: packages/cli/src/config/settingsSchema.ts:1293 - The hooks property inside HOOK_DEFINITION_ITEMS has an incorrect type definition. It's defined as type: 'object' but should be type: 'array' since it represents an array of hook configurations (HookConfig[] in the runtime type). This inconsistency will cause the generated JSON schema to be semantically incorrect.

    Current code:

    hooks: {                                                                   
      type: 'object',  // ❌ Should be 'array'
      description: 'The list of hook configurations to execute.',              
      required: true,                                                          
      items: { ... }
    },

    Should be:

    hooks: {                                                                   
      type: 'array',  // ✅ Correct type
      description: 'The list of hook configurations to execute.',              
      required: true,                                                          
      items: { ... }
    },

    This bug propagates to the generated settings.schema.json file, where the hooks property will have the wrong type declaration. While JSON Schema validators may still work due to the presence of the items property, this is semantically incorrect and could cause issues with strict validators or tooling.

🟡 High

  • File: packages/cli/src/config/settingsSchema.ts - The SettingItemDefinition interface has a recursive structure where properties values are SettingItemDefinition & { required?: boolean; ... }. This intersection type adds required as a property on the child definition, but the base SettingItemDefinition already has a required?: boolean field. This creates some confusion about whether required should be at the property level or the definition level. Consider clarifying this pattern with a comment or restructuring.

🟢 Medium

  • File: scripts/generate-settings-schema.ts:66-72 - The convertItemDefinitionToJsonSchema function builds the required array by checking childDef.required on each property. However, in standard JSON Schema, the required array is at the parent object level, not embedded in each property definition. The current approach works but differs from the typical JSON Schema pattern. Consider documenting this design choice or aligning with standard JSON Schema conventions where required is an array of property names at the object level.

  • File: packages/cli/src/config/settingsSchema.ts:97 - The SettingItemDefinition interface could benefit from JSDoc comments on individual properties to clarify their purpose, especially for developers unfamiliar with JSON Schema structure.

🔵 Low

  • File: packages/cli/src/config/settingsSchema.ts - Consider extracting the nested hook configuration schema (the inner items object with type, command, name, etc.) into its own constant like HOOK_CONFIG_ITEMS for better reusability and testability. This would also make the HOOK_DEFINITION_ITEMS constant more readable.

  • File: scripts/generate-settings-schema.ts - The JsonSchemaProperty interface now has a required?: string[] field added for the PR. Consider adding a comment explaining this field's purpose, as it's not immediately clear why it differs from the required field in SettingItemDefinition.

✅ Highlights

  • Excellent PR description with clear before/after screenshots showing the VS Code error and the fix
  • The fix correctly addresses issue Hooks JSON Schema 将 HookDefinition 数组元素类型错误定义为 string #2246 and improves the developer experience for users configuring hooks
  • Good use of TypeScript interfaces to define the schema structure in a type-safe manner
  • The generator script update is well-implemented with proper handling of nested object types and arrays
  • Comprehensive schema definition includes all hook configuration fields (matcher, sequential, hooks, type, command, name, description, timeout, env)
  • Proper enum validation for the type field (currently only 'command' is supported)
  • The changes maintain backward compatibility - no breaking changes to existing configurations

@wenshao
Copy link
Collaborator

wenshao commented Mar 11, 2026

  1. [Bug] env.additionalProperties lost in generated schema

Source definition in settingsSchema.ts:
env: {
type: 'object',
description: '...',
additionalProperties: { type: 'string' },
}

Generated settings.schema.json:
"env": {
"description": "Environment variables...",
"type": "object"
}

The additionalProperties is missing. Root cause: convertItemDefinitionToJsonSchema only processes additionalProperties when itemDef.type === 'object' && itemDef.properties. Since env has no properties, the entire block is skipped. Fix: move additionalProperties handling outside the if (itemDef.properties) block.

  1. [Misleading] hooks field uses type: 'object' but represents an array

In HOOK_DEFINITION_ITEMS:
hooks: {
type: 'object', // misleading — this is HookConfig[] (an array)
items: { ... }
}

The generator silently overrides the type to 'array' when items is present, so the output is correct. But the source definition is semantically misleading. Should be type: 'object' with a comment, or better yet, support an 'array' type in SettingItemDefinition.

  1. [Missing] source field not in schema

CommandHookConfig has source?: HooksConfigSource (values: 'project' | 'user' | 'system' | 'extensions'), which is absent from the schema. Low priority since source is likely an internal field not user-configurable.

  1. [Scope] Only 2 of 12 hook events covered

Only UserPromptSubmit and Stop get the items schema. Other events (PreToolUse, PostToolUse, etc.) aren't in settingsSchema.ts at all, so this is a pre-existing gap — not this PR's responsibility.

  1. [Suggestion] Non-standard required placement in SettingItemDefinition

required: true is placed at the property level instead of standard JSON Schema's parent-level required array. The converter handles the mapping correctly, but this non-standard design may confuse developers familiar with JSON Schema conventions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants