refactor(hooks): replace flat backend flags with hooks.scm category-scoped infrastructure#606
Conversation
…coped infrastructure
|
✨ On it — checking the hooks infrastructure changes Progress: [░░░░░░░░░░] 4% (iteration 3/70) 🔍 Code Review Update (1 min) I've completed reviewing the provided files, Last updated: iteration 3 · review |
|
🔍 Reviewing PR #606... |
|
✅ Review complete — Approved Clean refactoring that replaces agent-type string matching with a data-driven |
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Clean, well-structured refactoring that replaces brittle agent-type string matching with a data-driven hook system. The implementation is correct, backward-compatible, and well-tested. Approved.
Architecture & Design
The PR makes a solid architectural improvement:
- Hook-driven validation eliminates the coupling between
validateFinish()and specific agent type strings. New agent types get finish validation "for free" by declaring hooks in their YAML definition — no code changes needed. - Category-scoped design (
hooks.scm) is extensible for future hook categories (e.g.,hooks.pm,hooks.email) without polluting the flat namespace. resolveScmHooks()merge strategy is clean: legacy flat flags spread first, thenhooks.scmoverrides — simple, predictable, and well-documented.- Threading path is straightforward: YAML →
resolveScmHooks()→ profile → llmist backend →createConfiguredBuilder()→initSessionState()→Finishgadget →validateFinish().
Observations (non-blocking)
-
respond-to-pr-commentgainsrequiresPushedChanges: true— this is a new behavioral constraint that wasn't previously enforced (the old code only checkedrespond-to-reviewandrespond-to-ci). This seems intentional given the agent's purpose ("make the changes, commit, and push"), but worth noting as a behavioral change beyond pure refactoring. -
debug.yamlchanged fromenableStopHooks: trueto no hooks — defaults tofalse. Correct for a read-only agent (onlyfs:readcapability), so the originaltruewas likely a misconfiguration. -
CLI
finish.tspasseshooks: {}— meaning CLI-driven finish bypasses all hook validation. This is the right call since CLI users manage their own workflow, but it's worth documenting this design decision for future maintainers.
Summary
Card: https://trello.com/c/69a59cdcad0376266f527504
Test plan
Generated with Claude Code