Refactor: Address code quality issues in Codex CLI hooks implementation (PR #1439)

[dyoshikawa]

Background

PR #1439 added Codex CLI hooks support (feat(hooks): add Codex CLI hooks support). A code review identified several code quality and maintainability issues that should be addressed as follow-up work. None of these are blockers or security concerns, but they impact long-term maintainability and consistency with the codebase's established patterns.

Details

1. [high] DRY Violation: Inline converter duplicates tool-hooks-converter.ts

src/features/hooks/codexcli-hooks.ts reimplements canonicalToCodexcliHooks() and codexcliHooksToCanonical() with logic nearly identical to the shared canonicalToToolHooks() / toolHooksToCanonical() in tool-hooks-converter.ts.

The claudecode-hooks.ts pattern (defining a ToolHooksConverterConfig and delegating to the shared converter) should be followed. Codex CLI-specific differences (no projectDirVar, command-only hooks) can be handled via converter config options.

Note: geminicli-hooks.ts has the same duplication issue (predates the shared converter), so both should ideally be refactored together.

2. [mid] OCP Violation: Tool-specific branching in hooks-processor.ts

In src/features/hooks/hooks-processor.ts, generateToolFiles() has a codexcli-specific conditional:

if (this.toolTarget === "codexcli") {
  result.push(await CodexcliConfigToml.fromBaseDir({ baseDir: this.baseDir }));
}

This breaks the Open/Closed Principle. Consider adding an optional getAuxiliaryFiles() method to ToolHooksFactory or ToolHooks so the processor can handle auxiliary files generically.

3. [mid] Missing TOML parse error handling in buildCodexConfigTomlContent

smolToml.parse(existingContent) in src/features/hooks/codexcli-hooks.ts is not wrapped in try/catch. If the user has a malformed config.toml, the error will be unclear. Other implementations (e.g., codexcli-mcp.ts) wrap parsing with formatError.

4. [low] Missing name/description fields in output

canonicalToCodexcliHooks only emits type, command, and timeout. The Gemini CLI converter preserves name/description as well. If Codex CLI supports these fields, they should be passed through.

5. [low] Asymmetric prompt type handling between import/export

codexcliHooksToCanonical accepts prompt-type hooks, but canonicalToCodexcliHooks filters them out. This causes silent data loss on round-trip. At minimum, add a comment explaining this asymmetry.

6. [low] Duplicate import statements

import type { AiFileParams } from "../../types/ai-file.js";
import type { ValidationResult } from "../../types/ai-file.js";

Should be consolidated into: import type { AiFileParams, ValidationResult } from "../../types/ai-file.js"

7. [low] Missing test for [features] section merge in CodexcliConfigToml

The test for preserving existing config.toml content only covers [mcp_servers]. It should also verify that an existing [features] section with other flags is correctly merged (not overwritten).

Solution / Next Steps

1. Refactor to shared converter (items 1, 4, 5): Extend ToolHooksConverterConfig to support Codex CLI's requirements (e.g., projectDirVar: "", supportedHookTypes: ["command"]). Refactor codexcli-hooks.ts (and optionally geminicli-hooks.ts) to delegate to the shared converter. This is the highest-value change.
2. Generalize auxiliary file generation (item 2): Add an auxiliaryFiles? or getAuxiliaryFiles() mechanism to the factory/hooks interface to remove the codexcli-specific branch.
3. Add error handling (item 3): Wrap smolToml.parse() in try/catch with formatError.
4. Fix minor issues (items 6, 7): Consolidate imports, add test case for features section merge.