refactor(hooks): follow-up improvements from PR #1069

[saitota]

Accidentally closed PR #1204 by deleting the fork. Recreating with the same diff.

Summary

• Extract shared Pascal hooks converter from duplicate Claude/Factory Droid conversion functions into pascal-hooks-converter.ts
• Add regression test verifying Factory Droid config isolation (Claude-specific hooks don't leak)
• Add event map completeness tests for all 4 tools (Claude, Factory Droid, Cursor, OpenCode)
• Add Factory Droid event naming verification against official documentation
• Add documentation comments explaining Cursor identity mapping loops

Closes #1077

References

• Factory Droid hooks reference: https://docs.factory.ai/reference/hooks-reference

Test plan

• pnpm check
• pnpm test

[dyoshikawa-claw]

/opencode review

[dyoshikawa]

@saitota Thank you. Please fix the points of the AI review.

[saitota]

Fixed. PTAL

• No4(MEDIUM): Applied safeString to prompt field in HookDefinitionSchema for consistency with command/matcher
• No3(Regex): Escaped regex special characters in projectDirVar
• Reviewer comment: Renamed Pascal → Tool (pascal-hooks-converter.ts → tool-hooks-converter.ts, canonicalToPascalHooks → canonicalToToolHooks, etc.) to align with existing canonicalToCopilotHooks naming pattern
• No7: Removed unnecessary const entry = rawEntry after type guard
• No5/No10(MEDIUM): Skipped — all 4 tool converters consistently delegate output validation to RulesyncHooks via toRulesyncHooksDefault(). This is a cross-cutting architectural change affecting all tools, beyond this refactoring PR's scope.

[r1bilski]

I'd say that the fact that for existing events simply converting the casing works is accidental. For some agents, the event names are completely different, and this might be the case for claude code or factorydroid if they add new events as well.

[saitota]

Thank you. Renamed pascal-hooks-converter.ts → tool-hooks-converter.ts and related symbols (canonicalToPascalHooks → canonicalToToolHooks, etc.) to align with the existing canonicalToCopilotHooks naming pattern.

[github-actions]

PR Review Summary: PR #1215 - Hooks Conversion Refactoring

Code Review Findings

1. Excellent DRY Application ✅ - Extraction of shared hooks conversion logic from duplicate Claude/Factory Droid implementations into tool-hooks-converter.ts eliminates ~200 lines of duplicate code. The configuration-based approach with ToolHooksConverterConfig is clean and extensible.

2. Security Improvement Applied ✅ - safeString applied to prompt field in src/types/hooks.ts:31 prevents potential injection vulnerabilities via control characters, aligning with existing command and matcher fields.

3. Regex Escaping Implemented ✅ - projectDirVar is properly escaped before use in regex (/[.*+?^${}()|[\]\\]/g), preventing regex injection vulnerabilities.

4. Good Test Coverage Additions ✅ - New isolation test in factorydroid-hooks.test.ts verifies Claude-specific hooks don't leak into Factory Droid output. Event map completeness tests ensure mappings stay synchronized.

5. Missing: Unit Tests for Converter Module ⚠️ - src/features/hooks/tool-hooks-converter.ts lacks dedicated unit test file (tool-hooks-converter.test.ts). Direct unit tests would document expected behavior of edge cases and make future refactoring safer.

6. Minor: Inconsistency with Other Tools ⚠️ - Cursor, Copilot, OpenCode, and Gemini CLI still have their own conversion functions. Consider adding a JSDoc comment explaining why tool-hooks-converter.ts only applies to tools with matcher/hooks structure (Claude, Factory Droid).

7. Minor: Documentation Comment Location ⚠️ - Added comments in cursor-hooks.ts explaining identity mapping could be JSDoc comments on mapping constants in types/hooks.ts instead, keeping hook files cleaner.

8. Adherence to Coding Guidelines ✅ - Full compliance: uses object parameters, static imports, kebab-case filenames, z.looseObject(), test file co-location.

9. Suggestion: Type Safety for ToolMatcherEntry ⚠️ - Consider defining more specific types for hook properties instead of Record<string, unknown>.

10. Previous Review Feedback Addressed ✅ - All previous findings correctly addressed: safeString for prompt, regex escaping, Pascal → Tool naming, removed unnecessary variable.

Security Review Findings

11. [MEDIUM] Validation Gap on Import Path - toRulesyncHooksDefault() in src/features/hooks/tool-hooks.ts:50-61 creates RulesyncHooks instances without validate: true, so canonical hooks JSON from toolHooksToCanonical() is never re-validated against HooksConfigSchema. Malicious external tool configs could inject control characters that bypass validation. Recommendation: Add validate: true to toRulesyncHooksDefault().

12. [LOW] Inconsistent Validation Across Tools - ClaudecodeHooks.validate(), FactorydroidHooks.validate(), and CursorHooks.validate() are no-ops while RulesyncHooks.validate() properly validates. Creates inconsistent security posture. Add comment explaining design rationale or implement proper validation.

13. [LOW] Type Guard Allows Entries Without Hooks - isToolMatcherEntry() in tool-hooks-converter.ts:8-19 accepts entries without hooks property (e.g., {}), potentially leading to silent data loss. Consider requiring hooks property.

14. [LOW] Command Prefix Edge Case - Logic in tool-hooks-converter.ts:67-70 only checks !def.command.startsWith("$"). Commands like ${MY_VAR}/script.sh pass through, but behavior should be documented.

15. [INFO] Positive Security Controls ✅ - Control character filtering via safeString, proper regex escaping, no direct command execution, JSON parsing errors handled, explicit event name mapping tables prevent injection.

16. [INFO] Missing Unit Tests for Converter - Same as finding Add support for opencode-ai/opencode #5. Add direct unit tests for edge cases: malformed input, commands with special characters, empty hooks arrays, unknown event names.

---

Overall Assessment: The refactoring is well-executed with good security improvements applied. The most actionable items are:

• High Priority: Add validate: true to toRulesyncHooksDefault() (finding feat: improve Cursor .mdc file processing with comprehensive frontmatter handling #11)
• Medium Priority: Add unit tests for tool-hooks-converter.ts (findings Add support for opencode-ai/opencode #5, refactor: remove 'claude' from union type and unify with 'claudecode' #16)
• Low Priority: Add documentation for design decisions (findings Add support for gemini-cli #6, implement issue12 #14)

github run