fix(hooks): list each tool's own built-in hook set - #718
Conversation
The PR description otherwise contains a sufficient unit-test plan and real-CLI end-to-end record. |
|
Addressed the P1 review finding on What was wrong: the per-tool rendering assumed every row receives the full How the real surface was determined: by reading each adapter that owns its own format, instead of guessing from the status table.
Change: new Tests (real runs on this branch):
🤖 Generated with Claude Code |
The PR description includes unit/type-check results and a detailed real-CLI end-to-end record, so its testing documentation is sufficient. |
|
Both P1s from the latest review are fixed, plus the P2.
Tests on Rebuilt CLI E2E (
🤖 Generated with Claude Code |
|
|
Round-3 review addressed on
Listing rule after this round: a tool appears only if Tests on 🤖 Generated with Claude Code |
|
Findings
|
|
Round-4 review addressed on
Tests on 🤖 Generated with Claude Code |
The PR description otherwise contains a sufficient test plan and real-CLI end-to-end verification record. |
|
Round-5 P1 resolved by taking the second option you offered: OpenClaw reconciliation was added to the authoritative path (
New tests in Verification on |
|
|
Round-6 findings fixed on
Verification on |
`hooks list` rendered builtinHookDefs('claude') for every tool, so the built-in
block described Claude's hook set no matter which tool the row was for:
Copilot's SessionEnd hook never appeared, while tools that receive no hooks at
all were credited with six.
The displayed set is now derived from what each tool actually receives through
reconciliation, and a tool with no hook surface is omitted rather than shown an
invented list.
Fixes Tencent#717
c0f247f to
b9d15e9
Compare
|
Findings
The PR description includes a detailed test plan and real-CLI end-to-end records, so it satisfies the testing-documentation requirement. |
|
Findings
The PR description includes a detailed test plan and real-CLI end-to-end verification, so its testing documentation is sufficient. |
`hooks list` probed a settings file per tool, so Hermes, OpenCode and OpenClaw — which reconciliation installs as one generated script/plugin/ handler each — fell through to the generic branch and were reported as "not configured" even right after `hooks inject` wrote their hook. OMP already had a bespoke branch for this. Resolve that artifact per adapter and use its presence as the status, the same rule OMP used, so the status column matches the built-in block. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Round-7 review, on Fixed — OpenClaw reported as ( Not fixed, deliberately — Verification on |
|
Findings
Testing
|
…ugin Two status-column defects the per-tool listing exposed: - `getHookStatus` checked the unmodified `builtinHookDefs(tool)`, so a built-in the team disabled through hooks.yaml was still expected on disk and every tool read `missing` right after a correct reconciliation. It now takes the same §4.8 override reconciliation applies. - The OpenCode artifact was probed under the config scope's base dir, but `reconcileOpencodePlugin` always installs the single plugin under the user path, so a project-scope config reported `missing` after a successful injection. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Round-8 findings fixed on
Existing Verification on |
|
Rebased onto 1. 2. The claw family overwriting one handler. The reconcile branch in 3. Missing real-CLI verification. Run against the built binary (
Tests at head |
|
Findings
Testing
|
The adapter status probe accepted a single file, so an OpenClaw installation missing its handler.ts — the file HOOK.md points at, without which no hook runs — still read as `installed`. Check every generated file the adapter writes and report `installed` only when all are present. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Round-9 finding, on Fixed — partial installations reported as installed: the adapter probe now checks every file the adapter generates, not one. For OpenClaw that is Not taken further, deliberately: validating the adapters' registration state as well — Hermes' Verification on |
|
Findings
The PR description includes a detailed test plan and real-CLI end-to-end records, so it satisfies the testing-documentation requirement. |
|
On the latest two findings, both concern behavior this PR did not introduce, so I am leaving them for a maintainer call rather than widening the patch again:
Happy to open follow-up issues for both, or to fold either into this PR if you prefer. The review finding this PR was opened to fix — the built-in block crediting tools with hooks they never receive — is resolved, along with four adjacent defects found on the way ( |
|
This PR had drifted well past the display defect it was opened for (8 files, +414), so I have reduced it back to its original scope. The reduced work is on What the reduced change does (#717)
The block is now rendered per tool, from the defs the settings reconcile path writes into the very file each row probes. Tools whose set is identical once the tool id is folded out share one block, so the listing stays about as short as before. What was removed, and why
The separate defects found during the earlier rounds ( Diff Tests
🤖 Generated with Claude Code |
Summary
teamai hooks listrenderedbuiltinHookDefs('claude')for every tool, so the listing did not match what the hook pipeline writes: Copilot'sSessionEndwas never shown, the command string was wrong for ZCode / WorkBuddy / CodeBuddy, and tools whose real surface is narrower (or empty) were advertised a six-hook set they never receive. The listing is now built from what each tool actually receives.Type of Change
Root cause
src/hooks-cmd.ts(before this change):builtinHookDefs(tool)is tool-dependent by design: it appendsCOPILOT_SESSION_END_SPECforcopilotand selects the command builder per tool (raw forzcode, PATH wrapper for the shell-dependent GUI tools). The listing called it once with a literal, so both variations were invisible.It is also not the whole truth for tools that are not reconciled through a settings/hooks file: the standalone adapters install a narrower slice, and some tools get nothing at all.
Fix
installedBuiltinHookDefs(tool, settingsDriven)insrc/builtin-hooks.tsresolves the displayed set from what actually installs it;hooks listuses it per row and omits tools whose set is empty. Two gaps surfaced by that audit are fixed too:reconcileHooksToAllTools(the path behindhooks inject/init/pull) had no OpenClaw branch and skipped it for lack of asettingspath, so the two hooks listed for OpenClaw are now also the two that path installs; and the listing applies the team'sbuiltin:override (§4.8), which the reconcile engine already applies, so a disabled built-in no longer stays on screen after it was removed from the settings files.reconcileHooks(src/hooks.ts)SessionEndopencodeTOOL_MATCHER(src/opencode-hooks.ts)Skill/TodoWritematchers), raw commandompsrc/omp-hooks.ts)tool_resultcarries no matcherhermesbuildReportScript()(src/hermes-hooks.ts)>/dev/null 2>&1 || trueopenclawEVENT_MAP+ generated handler (src/openclaw-hooks.ts)SessionStart,UserPromptSubmit), raw commandjoycode,qclaw/easyclaw/autoclawkirorenderForKiroembedshooks.agentSpawnper synced agent, not via the hook pipelineTools whose rendered set is identical once
--tool <tool>is folded out still share one block, so the section stays short.Test Plan
npx tsc --noEmitpassesnpm testpassesdist/index.jsUnit tests
src/__tests__/hooks-cmd.test.ts:main(expect(text).toContain(' copilot:')failed), GREEN after the per-tool rendering.expected [ …(6) ] to deeply equal [ StringContaining "SessionStart" ]for hermes), GREEN after.src/__tests__/openclaw-hooks.test.ts: "reconcileHooksToAllTools routes the OpenClaw family to its adapter" — inject writes the workspace hook dir,removeAlldeletes it, and nothing is created when the workspace cannot be resolved.Results on the current head:
End-to-end with the real CLI
npm run build, then the builtdist/index.jsagainst sandboxedHOMEs (Ubuntu 24.04, Node v22.22.0).Matrix —
teamai hooks injectthenteamai hooks list, once per provider (github,gitlab,git), tool roots pre-created for Claude, Codex, CodeBuddy, OpenCode (plus Hermes, OpenClaw): identical result in all three runs —~/.claude/settings.jsonand~/.codebuddy/settings.jsoneach carrySessionStart x1, Stop x1, PostToolUse x3, UserPromptSubmit x1(the 6 entries listed for them),~/.codex/hooks.jsonis written,~/.config/opencode/plugin/teamai-hooks.tsspawns['hook-dispatch', event, '--tool', 'opencode'], and~/.hermes/hooks/teamai-status-report.shcontains exactlyteamai hook-dispatch session-start --tool hermes >/dev/null 2>&1 || true— i.e. the artifacts match the lines printed for those tools. OpenClaw is now reconciled from that same path:hooks injectwrites<workspace>/hooks/teamai-status-report/{HOOK.md,handler.ts}(handlerEVENT_MAP=session:start→ session-start,command:new→ prompt-submit, matching the two lines listed for it) andhooks removedeletes them.teamai hooks listoutput of the current head:joycodeandkirodo not appear in the built-in block;ompshows 4 entries without theSkill/TodoWritematchers;openclawshows its 2;hermesshows its single entry exactly as its generated script runs it.OpenClaw reconciliation (new in this PR), real CLI: with
~/.openclaw/workspacepresent,teamai hooks injectwrites<workspace>/hooks/teamai-status-report/{HOOK.md,handler.ts}(handlerEVENT_MAP={"session:start":"session-start","command:new":"prompt-submit"}) andteamai hooks removedeletes that directory.builtin:override parity, real CLI: withhooks/hooks.yamlcarryingbuiltin.disabled: ["Hook dispatch stop"],teamai hooks injectleaves~/.claude/settings.jsonwithSessionStart x1, PostToolUse x3, UserPromptSubmit x1— andteamai hooks listprints exactly those 5 lines, noStop.Documentation
docs/usage-guide.mdanddocs/usage-guide.zh-CN.mdnow state that the built-in block is per tool and why some tools are omitted.Related Issues
Fixes #717
Notes for Reviewers
--tool <tool>in the rendered command is a template placeholder so a shared block does not repeat one identical stanza per tool.🤖 Generated with Claude Code