refactor(hooks): clean up Codex CLI hooks implementation (#1445)#1451
Closed
dyoshikawa wants to merge 2 commits intomainfrom
Closed
refactor(hooks): clean up Codex CLI hooks implementation (#1445)#1451dyoshikawa wants to merge 2 commits intomainfrom
dyoshikawa wants to merge 2 commits intomainfrom
Conversation
…verter - Extend ToolHooksConverterConfig with supportedHookTypes, omitEmptyEvents, empty-projectDirVar support, and pass-through of name/description fields, then refactor codexcli-hooks.ts and geminicli-hooks.ts to delegate to the shared canonicalToToolHooks/toolHooksToCanonical helpers instead of reimplementing the conversion logic. - Generalize auxiliary file generation in hooks-processor.ts via an optional getAuxiliaryFiles() factory hook so the codexcli-specific config.toml branch is no longer hardcoded. - Wrap smolToml.parse() in buildCodexConfigTomlContent() with try/catch and formatError so malformed config.toml files surface a clear error. - Consolidate duplicate AiFileParams/ValidationResult imports. - Add tests covering the [features] section merge and the malformed config.toml error path for CodexcliConfigToml. Refs #1445 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors Codex CLI (and Gemini CLI) hooks handling to reuse the shared tool-hooks converter, removes codexcli-specific branching in the hooks processor via an auxiliary-files hook, and improves Codex CLI config.toml error handling + test coverage.
Changes:
- Extend
tool-hooks-converter.tswith Codex CLI–specific config options (supported hook types, omit empty events, empty project dir var) and preservename/descriptionin conversions. - Generalize auxiliary file emission in
hooks-processor.tsvia optionalgetAuxiliaryFiles(), implemented byCodexcliHooksto emit.codex/config.toml. - Add Codex CLI tests for
[features]merge behavior and malformed TOML error messaging.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/features/hooks/tool-hooks-converter.ts |
Adds converter config knobs for Codex CLI quirks; passes through name/description; supports omitting empty events. |
src/features/hooks/hooks-processor.ts |
Replaces codexcli-specific branching with a generic getAuxiliaryFiles() extension point. |
src/features/hooks/geminicli-hooks.ts |
Removes duplicated conversion logic and delegates to the shared converter. |
src/features/hooks/codexcli-hooks.ts |
Delegates to shared converter, adds auxiliary file generation, and improves TOML parse error handling. |
src/features/hooks/codexcli-hooks.test.ts |
Adds coverage for [features] merge and malformed TOML error path. |
Comment on lines
170
to
+179
| for (const rawEntry of matcherEntries) { | ||
| if (!isToolMatcherEntry(rawEntry)) continue; | ||
| const hookDefs = rawEntry.hooks ?? []; | ||
| for (const h of hookDefs) { | ||
| const cmd = typeof h.command === "string" ? h.command : undefined; | ||
| const prefixingEnabled = converterConfig.projectDirVar !== ""; | ||
| const command = | ||
| typeof cmd === "string" && cmd.includes(`${converterConfig.projectDirVar}/`) | ||
| prefixingEnabled && | ||
| typeof cmd === "string" && | ||
| cmd.includes(`${converterConfig.projectDirVar}/`) |
There was a problem hiding this comment.
toolHooksToCanonical assumes every element of rawEntry.hooks is a non-null object, but isToolMatcherEntry() only checks that hooks is an array. If a tool config is manually edited (or otherwise malformed) and includes null/primitives inside hooks, typeof h.command will throw at runtime. Please harden parsing by skipping non-object hook entries (e.g., guard h with h && typeof h === "object") or strengthen isToolMatcherEntry() to validate hook array elements before iterating.
- Add logger to ToolHooksFromRulesyncHooksParams and pass this.logger from HooksProcessor so converter warnings actually fire (#1). - Add passthroughNameDescription flag to ToolHooksConverterConfig and enable it only for codexcli/geminicli, preventing the unconditional pass-through from leaking unknown 'name'/'description' keys into Claude Code / Factory Droid hook outputs (#2). - Document the Codex CLI command-passthrough behavior for hand-edited configs containing other tools' project-dir variables (#3). Refs #1445 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses the code-quality follow-ups from #1445 (review of #1439).
codexcli-hooks.tsandgeminicli-hooks.tsnow delegate to the sharedcanonicalToToolHooks/toolHooksToCanonicalhelpers intool-hooks-converter.tsinstead of reimplementing the conversion logic.ToolHooksConverterConfiggainedsupportedHookTypes,omitEmptyEvents, and empty-projectDirVarsupport to express the Codex CLI quirks (command-only hooks, no project dir variable, drop empty events). The shared converter also now passesname/descriptionthrough in both directions, fixing the silent drop on Codex CLI export.if (toolTarget === 'codexcli')branch inhooks-processor.ts. Tools that need extra files now expose an optional staticgetAuxiliaryFiles({ baseDir }), andCodexcliHooksimplements it to emit.codex/config.toml.smolToml.parse()inbuildCodexConfigTomlContentwithtry/catchandformatError, so a malformedconfig.tomlproduces a clear error mentioning the path.toolHooksToCanonicalintentionally still preservesprompt-type hooks on import even for Codex CLI; the asymmetry is now documented in the converter config so the next export silently filters them (with the existinghooks-processorwarning making the loss visible).AiFileParams/ValidationResultimport lines.[features]section merge inCodexcliConfigTomland for the malformed-TOML error path.Test plan
pnpm cicheck(lint, typecheck, 4774 tests, content checks) — all green