Skip to content

chat: fix CLI hook command validation - #343651

Merged
Martin Aeschlimann (aeschli) merged 2 commits into
mainfrom
digitarald/agents/hook-validation-local-schema-fix
Nov 3, 2026
Merged

Martin Aeschlimann (aeschli) merged 2 commits into
mainfrom
digitarald/agents/hook-validation-local-schema-fix

Conversation

@digitarald

@digitarald Harald Kirschner (digitarald) commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #343595.

Changes

  • Accept string-valued command alongside bash/powershell in the Copilot CLI hook JSON schema, including completion metadata.
  • Update the missing-command diagnostic and preserve the existing command-field alternatives.
  • Add a regression test for the command property and accepted alternatives.

The reported version: 1 file already selects the CLI schema; its command-field requirement caused the squiggles. No harness-selection or runtime behavior changes are included.

The Local-format deprecation, default-snippet migration, and coordinated Configure Hooks changes are isolated in stacked draft #345634. This PR leaves the Local format and default snippet unchanged.

Validation

  • The focused branch retains the original command fix (ed908b856cb) and merges updated main in 145578ff518; its diff against that main revision remains limited to the command schema and regression test.
  • Standalone in-memory Ajv validation of the focused schema — passed 10 cases: command/bash/powershell, missing command, invalid command values, invalid type/timeout, Local compatibility, and the unchanged default snippet. A baseline regression check confirmed that the pre-fix schema rejects command-only input.
  • git diff --check ed908b856cb^ ed908b856cb — passed.
  • An independent full build/unit-test run of this focused branch was blocked during initial dependency bootstrap. After restoring public build dependencies, compilation, lint, hygiene, and 72 hook unit tests passed on stacked chat: deprecate the Local hook JSON format #345634, which includes this fix; those runs also include follow-up changes and are not an independent run of this branch.
  • Live UI/CLI execution and broad type-checking were not run.

Manual Check

  1. Open the JSON repro in Copilot CLI hook JSON validation rejects command-only hooks #343595 under .github/hooks/: command-only CLI hooks should have no missing-command diagnostic.
  2. Remove the command or use a non-string value: validation should still reject the configuration.
  3. Verify existing bash-only, powershell-only, and Local-format files remain accepted.

Draft for roblourens and Martin Aeschlimann (@aeschli).

Fixes #343595

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The focused schema correction matches the reported requirement, preserves existing alternatives, and has regression coverage with no actionable defects identified.

No open issues 🎉

What changed in this PR

Fixes #343595 by allowing command in the Copilot CLI hook JSON schema, without changing harness selection or runtime behavior.

Changes:

  • Accepts string-valued command alongside the existing bash and powershell alternatives.
  • Adds completion metadata and updates the missing-command diagnostic.
  • Adds a regression test for the command property and accepted alternatives.
File Description
src/​vs/​workbench/​contrib/​chat/​test/​common/​promptSyntax/​hookSchema.test.ts Tests the CLI schema’s command metadata and required-field alternatives.
src/​vs/​workbench/​contrib/​chat/​common/​promptSyntax/​hookSchema.ts Accepts command and updates the validation diagnostic.

Review effort: Balanced

@digitarald Harald Kirschner (digitarald) changed the title chat: accept command in Copilot CLI hook JSON schema chat: fix CLI hook validation and deprecate the Local format Oct 23, 2026
@aeschli

Copy link
Copy Markdown
Contributor

The command schema correction looks right. I recommend keeping this PR focused on #343595:

  • Keep command in the CLI schema properties and required-field alternatives, together with the updated missing-command diagnostic.
  • Move the Local-format deprecation and default-snippet migration into a separate follow-up PR.
  • Add behavioral JSON-schema validation tests for command-only, bash-only, powershell-only, missing-command, and non-string-command cases; the added tests currently inspect schema structure rather than diagnostics.

The deprecation follow-up should coordinate Configure Hooks (which still creates Local-format files), the VS Code CLI event map (which omits preCompact and subagentStart), and input/output compatibility during migration. The runtime tags PascalCase hooks with _vsCodeCompat, which affects payload mapping, so renaming events is not necessarily behavior-preserving.

I verified with the built native runtime that both PascalCase and camelCase PreCompact/SubagentStart configurations load successfully. My earlier review concern that the runtime would drop those events was incorrect; the omission is in VS Code hook discovery/parsing, not runtime support.

@aeschli
Martin Aeschlimann (aeschli) force-pushed the digitarald/agents/hook-validation-local-schema-fix branch from c01417d to ed908b8 Compare November 2, 2026 19:44
@aeschli Martin Aeschlimann (aeschli) changed the title chat: fix CLI hook validation and deprecate the Local format chat: fix CLI hook command validation Nov 2, 2026
@aeschli
Martin Aeschlimann (aeschli) marked this pull request as ready for review November 2, 2026 21:25
@aeschli
Martin Aeschlimann (aeschli) enabled stacked auto-merge (squash) November 2, 2026 21:55
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@aeschli
Martin Aeschlimann (aeschli) merged commit c5844f4 into main Nov 3, 2026
57 checks passed
@aeschli
Martin Aeschlimann (aeschli) deleted the digitarald/agents/hook-validation-local-schema-fix branch November 3, 2026 09:36
@vs-code-engineering vs-code-engineering Bot added this to the 1.146.0 milestone Nov 3, 2026
Martin Aeschlimann (aeschli) added a commit that referenced this pull request Nov 3, 2026
<!-- Thank you for submitting a Pull Request. Please:
* Read our Pull Request guidelines:

https://github.com/microsoft/vscode/wiki/How-to-Contribute#pull-requests
* Associate an issue with the Pull Request.
* Ensure that the code is up-to-date with the `main` branch.
* Include a description of the proposed changes and how to test them.
-->

Follow-up to merged #343651, now based on `main`. This draft deprecates
the Local hook format and updates Configure Hooks so newly generated
Copilot files use the recommended format. The command-validation fix is
already on `main` and is not part of this PR's diff.

## Changes

- Keep Local-format hook files valid, but add a deprecation warning
recommending `version: 1`, CLI event names, and `timeoutSec`.
- Change the default JSON schema snippet to the CLI format.
- Update both new-file creation paths in Configure Hooks to write
versioned camelCase Copilot files with a cross-platform `command`
placeholder. Claude files retain their nested format, and existing Local
files retain their naming convention.
- Honor `version` when adding events to an empty CLI file.
- Register the runtime-supported `preCompact` and `subagentStart` names
so generated hooks remain discoverable and receive schema metadata.
- Add thirteen tests covering generated files for all eleven hook types,
parser round trips, and unchanged Claude structure.

## Required Before Ready for Review

- Define and test input/output compatibility during migration of
existing files. The runtime normalizes PascalCase aliases and adds
`_vsCodeCompat`, which affects payload mapping; changing only
names/version/timeout is not necessarily behavior-preserving. This draft
does not automatically migrate existing files.
- Add behavioral JSON language-service validation tests for deprecation
diagnostics and the default snippet.
- Validate the Configure Hooks workflow in the running workbench,
including editor selection and existing-file updates.

The native runtime was checked separately: both PascalCase and camelCase
configurations for `PreCompact`/`SubagentStart` load successfully.
Runtime support is not the blocker.

## Validation

- After #343651 merged, merged `main` (`3bc96745d641`) in `4bcc64dc9ef`
and resolved the schema-test conflict, preserving both the upstream
command-validation test and this draft's deprecation/default-snippet
tests. The remaining diff contains only the follow-up changes.
- `node build/next/index.ts transpile` — passed against the updated
sources.
- `node test/unit/node/index.js --reporter dot --run
src/vs/platform/agentPlugins/test/common/pluginParsers.test.ts --run
src/vs/workbench/contrib/chat/test/common/promptSyntax/hookCompatibility.test.ts
--run
src/vs/workbench/contrib/chat/test/common/promptSyntax/hookSchema.test.ts`
— passed, 136 tests after the latest main merge.
- `git diff --cached --check` and normal pre-commit hygiene checks —
passed for both update merges.
- The fast-build prerequisite check was blocked by an installed
SDK/runtime version mismatch; dependency refresh was blocked by
package-feed connectivity. The direct client transpile and targeted
tests above succeeded; no new broad type-check was run.

- Feedback follow-up: plugin parsing now also canonicalizes `preCompact`
and `subagentStart`, preserving their original event names and commands.
- `node test/unit/node/index.js --run
src/vs/platform/agentPlugins/test/common/pluginParsers.test.ts --run
src/vs/workbench/contrib/chat/test/common/promptSyntax/hookCompatibility.test.ts
--run
src/vs/workbench/contrib/chat/test/common/promptSyntax/hookSchema.test.ts`
— passed, 131 tests after the feedback fix; client compilation also
passed.
- `node build/eslint.ts
src/vs/platform/agentPlugins/common/pluginParsers.ts
src/vs/platform/agentPlugins/test/common/pluginParsers.test.ts` and
`node build/hygiene.ts
src/vs/platform/agentPlugins/common/pluginParsers.ts
src/vs/platform/agentPlugins/test/common/pluginParsers.test.ts` — both
passed; the normal pre-commit hook also passed.

- `node build/next/build-fast.ts --client-only` — passed.
- `node test/unit/node/index.js --run
src/vs/workbench/contrib/chat/test/common/promptSyntax/hookCompatibility.test.ts
--run
src/vs/workbench/contrib/chat/test/common/promptSyntax/hookSchema.test.ts`
— passed, 72 tests.
- `node build/eslint.ts
src/vs/workbench/contrib/chat/browser/promptSyntax/hookActions.ts
src/vs/workbench/contrib/chat/common/promptSyntax/hookCompatibility.ts
src/vs/workbench/contrib/chat/common/promptSyntax/hookTypes.ts
src/vs/workbench/contrib/chat/test/common/promptSyntax/hookCompatibility.test.ts`
— passed, four files.
- `node build/hygiene.ts
src/vs/workbench/contrib/chat/browser/promptSyntax/hookActions.ts
src/vs/workbench/contrib/chat/common/promptSyntax/hookCompatibility.ts
src/vs/workbench/contrib/chat/common/promptSyntax/hookTypes.ts
src/vs/workbench/contrib/chat/test/common/promptSyntax/hookCompatibility.test.ts`
— passed, four files; the normal pre-commit hook also passed.
- `git diff --check` — passed before commit.
- Standalone in-memory checks — all ten generated Copilot event
configurations validate against the schema and remain discoverable.
Earlier standalone Ajv validation also passed ten input cases on each
split branch state and the baseline command-only regression check.
- `./scripts/test.sh --run
src/vs/workbench/contrib/chat/test/common/promptSyntax/hookCompatibility.test.ts
--run
src/vs/workbench/contrib/chat/test/common/promptSyntax/hookSchema.test.ts
--run
src/vs/workbench/contrib/chat/test/browser/promptSyntax/hookActions.test.ts
--run
src/vs/workbench/contrib/chat/test/browser/promptSyntax/hookUtils.test.ts
--reporter dot` — Electron crashed before tests ran (exit 139).
- `node test/unit/browser/index.js --run
src/vs/workbench/contrib/chat/test/common/promptSyntax/hookCompatibility.test.ts
--run
src/vs/workbench/contrib/chat/test/common/promptSyntax/hookSchema.test.ts
--run
src/vs/workbench/contrib/chat/test/browser/promptSyntax/hookActions.test.ts
--run
src/vs/workbench/contrib/chat/test/browser/promptSyntax/hookUtils.test.ts
--browser chromium --reporter dot` — blocked by sandbox policy on
loopback listening (`EPERM ::1`); browser tests were not run.
- Live UI, end-to-end migration, and broad type-checking were not run.
No dependency manifests were changed.

## Manual Checks

1. Configure a new hook and verify the generated JSON has `version: 1`,
the correct CLI event name, and no Local-format deprecation warning; the
command placeholder should be selected.
2. Repeat for PreCompact, SubagentStart, UserPromptSubmit, and Stop;
verify generated events remain visible in Configure Hooks.
3. Add a hook to an empty versioned CLI file and verify its event uses
CLI naming.
4. Verify existing Local files and Claude files retain their structure
when adding hooks.
5. Verify the default CLI snippet has no schema diagnostics.

This draft targets `main`; the former stacked parent #343651 has merged.
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.

Copilot CLI hook JSON validation rejects command-only hooks

5 participants