refactor(hooks): use shared converter for Codex CLI, add auxiliary file support and TOML error handling#1535
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors Codex CLI hooks conversion to reuse the shared hooks converter, adds a generic auxiliary-file mechanism for hook processors, and improves .codex/config.toml generation by preserving existing [features] values and surfacing TOML parse errors.
Changes:
- Extended the shared hooks converter with
supportedHookTypes+passthroughFields, and made export drop empty matcher/event entries. - Refactored
CodexcliHooksto use the shared converter and added robust TOML parsing/preservation for.codex/config.toml. - Added
ToolHooks.getAuxiliaryFiles()and updatedHooksProcessorto merge auxiliary generated files (e.g., Codexconfig.toml) without tool-specific branching; updated tests accordingly.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/features/hooks/tool-hooks.ts | Adds a base getAuxiliaryFiles() hook for tools to emit extra generated files. |
| src/features/hooks/tool-hooks-converter.ts | Adds converter config options for hook-type filtering and passthrough fields; drops empty outputs. |
| src/features/hooks/hooks-processor.ts | Removes codex-specific branching and merges auxiliary files from hook tool classes. |
| src/features/hooks/hooks-processor.test.ts | Adds coverage ensuring Codex conversion returns hooks + auxiliary config TOML. |
| src/features/hooks/codexcli-hooks.ts | Switches Codex hooks conversion to the shared converter and improves .codex/config.toml parsing/error reporting. |
| src/features/hooks/codexcli-hooks.test.ts | Adds tests for [features] preservation and readable TOML parse errors. |
| : cmd; | ||
| const hookType = h.type === "command" || h.type === "prompt" ? h.type : "command"; | ||
| const timeout = typeof h.timeout === "number" ? h.timeout : undefined; | ||
| const prompt = typeof h.prompt === "string" ? h.prompt : undefined; | ||
| defs.push({ | ||
| type: hookType, | ||
| ...(command !== undefined && command !== null && { command }), |
There was a problem hiding this comment.
supportedHookTypes is applied in canonicalToToolHooks, but toolHooksToCanonical currently ignores it and will import prompt hooks (etc.) into canonical config even when a converter config declares command-only support (e.g. Codex CLI). Apply the same supportedHookTypes filtering on import so round-trips don’t reintroduce unsupported hook types.
| const hookDefs = rawEntry.hooks ?? []; | ||
| for (const h of hookDefs) { | ||
| const cmd = typeof h.command === "string" ? h.command : undefined; | ||
| const command = | ||
| typeof cmd === "string" && cmd.includes(`${converterConfig.projectDirVar}/`) | ||
| converterConfig.projectDirVar !== "" && | ||
| typeof cmd === "string" && | ||
| cmd.includes(`${converterConfig.projectDirVar}/`) |
There was a problem hiding this comment.
toolHooksToCanonical assumes every element in rawEntry.hooks is a non-null object. If a tool config contains hooks: [null] (or other non-object values), h.command / h.type access will throw at runtime during import. Add a guard inside the for (const h of hookDefs) loop to skip non-object entries before reading properties.
Motivation
.codex/config.tomlgeneration preserves existing[features]and surfaces parse errors clearly.Description
supportedHookTypesandpassthroughFieldsoptions toToolHooksConverterConfigand updatedcanonicalToToolHooks/toolHooksToCanonicalto honor them and to drop empty entries.CodexcliHooksto use the sharedcanonicalToToolHooks/toolHooksToCanonicalwith aCODEXCLI_CONVERTER_CONFIG(command-only, passthroughname/description, no project-dir prefix).buildCodexConfigTomlContentto throw a readable error on invalid.codex/config.tomland preserve existing[features]values while enablingcodex_hooks.ToolHooks.getAuxiliaryFiles()(default no-op) andCodexcliHooks.getAuxiliaryFiles()to return the generatedCodexcliConfigToml, and updatedHooksProcessorto merge auxiliary files into results instead of using a codex-specific branch.[features], and TOML parse error behavior, and updated code formatting/types accordingly.Testing
pnpm vitest run src/features/hooks/codexcli-hooks.test.ts src/features/hooks/hooks-processor.test.tsand they passed (both files passed during iterative runs).pnpm cicheck(includes formatting, linting, typecheck, and the entire test suite) and it completed successfully with all tests passing.pnpm fmtandpnpm cicheckuntil clean; final run reports all tests green.Codex Task