feat(cli): native hooks adapter for Droid (Factory.ai) - #1130
Merged
Conversation
Droid ships a first-party hooks system (~/.factory/hooks.json) using the same manifest shape as Codex/Claude Code. `agentmemory connect droid --with-hooks` now merges the bundled hooks.droid.json into it, covering SessionStart, UserPromptSubmit, PreToolUse, PostToolUse, and SessionEnd via the existing buildMergedHooks merge/re-install logic. Also fixes a Windows test-isolation bug in connect-new-agents.test.ts where os.homedir() reads USERPROFILE (not HOME), which was silently letting adapter tests write into the real user home directory. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… for Droid hooks CHANGELOG [Unreleased] entry for the new --with-hooks path, and npm run skills:gen to sync the auto-generated agents table (pulled from src/cli/connect/index.ts protocolNote) so it reflects Droid's updated hooks capability. tools/rest/config/hooks reference docs showed as stale too but diffed empty (pre-existing line-ending-only drift on this checkout) so left untouched. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@berthojoris is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughDroid now supports optional native hook installation through ChangesDroid native hook installation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant DroidAdapter
participant JsonMcpAdapter
participant FactoryHooks
User->>DroidAdapter: connect with --with-hooks
DroidAdapter->>JsonMcpAdapter: provide installHooks callback
JsonMcpAdapter->>DroidAdapter: invoke callback after MCP wiring
DroidAdapter->>FactoryHooks: merge and atomically write hooks.json
FactoryHooks-->>DroidAdapter: return installation result
DroidAdapter-->>User: report installation or skip status
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/droid-connect-hooks.test.ts (2)
68-73: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCompare complete manifests for idempotency.
The test compares only entry counts. A merge regression can keep the same counts while changing commands or retaining duplicate entries. Compare
secondwithfirstto prove that reinstallation produces the same manifest.Proposed assertion
- for (const event of Object.keys(first.hooks)) { - expect(second.hooks[event]!.length).toBe(first.hooks[event]!.length); - } + expect(second).toEqual(first);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/droid-connect-hooks.test.ts` around lines 68 - 73, Update the idempotency test around buildMergedHooks to compare the complete second manifest with first, rather than checking only per-event entry counts. Assert deep equality so command contents, ordering, and duplicate entries are all validated.
24-39: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the complete Droid event set.
The test checks that five events exist and rejects only
PreCompactandStop. Another unsupported event can still entermerged.hookswithout failing the test. Compare the sorted keys with the exact five-event list.Proposed assertion
- for (const event of [ + const expectedEvents = [ "SessionStart", "UserPromptSubmit", "PreToolUse", "PostToolUse", "SessionEnd", - ]) { + ]; + for (const event of expectedEvents) { expect(Object.keys(merged.hooks)).toContain(event); } + expect(Object.keys(merged.hooks).sort()).toEqual( + [...expectedEvents].sort(), + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/droid-connect-hooks.test.ts` around lines 24 - 39, Update the test around buildMergedHooks to assert that the sorted merged.hooks keys exactly equal the sorted five-event list: SessionStart, UserPromptSubmit, PreToolUse, PostToolUse, and SessionEnd. Replace the partial presence/absence checks so any unsupported Droid event causes the test to fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli/connect/json-mcp-adapter.ts`:
- Around line 89-94: In the dry-run branch where opts.dryRun is true and
opts.withHooks && config.installHooks is called, capture the return value from
config.installHooks(opts) instead of discarding it. Check if the returned
hookResult has kind === "skipped" and if so, warn the user with the same warning
pattern used in the already-wired branch (Lines 78-87) and post-install branch
(Lines 124-133) to ensure hook-skipped conditions are surfaced consistently
across all code paths.
---
Nitpick comments:
In `@test/droid-connect-hooks.test.ts`:
- Around line 68-73: Update the idempotency test around buildMergedHooks to
compare the complete second manifest with first, rather than checking only
per-event entry counts. Assert deep equality so command contents, ordering, and
duplicate entries are all validated.
- Around line 24-39: Update the test around buildMergedHooks to assert that the
sorted merged.hooks keys exactly equal the sorted five-event list: SessionStart,
UserPromptSubmit, PreToolUse, PostToolUse, and SessionEnd. Replace the partial
presence/absence checks so any unsupported Droid event causes the test to fail.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 45923377-d13a-4030-b951-af399b5a34b3
📒 Files selected for processing (10)
CHANGELOG.mdREADME.mdplugin/hooks/hooks.droid.jsonplugin/skills/agentmemory-agents/REFERENCE.mdsrc/cli/connect/codex-hooks.tssrc/cli/connect/droid.tssrc/cli/connect/json-mcp-adapter.tssrc/cli/connect/types.tstest/connect-new-agents.test.tstest/droid-connect-hooks.test.ts
…id hook tests - json-mcp-adapter: capture and warn on skipped hook results in the dry-run branch (consistent with already-wired and post-install branches) - droid-connect-hooks test: assert exact five-event set instead of presence-only plus selective absence checks - droid-connect-hooks test: use deep equality (toEqual) for idempotency check instead of per-event length comparison Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
rohitg00
requested changes
Aug 2, 2026
Owner
There was a problem hiding this comment.
Checked this against Factory's hooks reference and it holds up: paths, events, and stdin payload shape all match, and reusing the bundled scripts plus buildMergedHooks instead of hand-rolling new hook logic is exactly what I want these adapters to do. Tests pass in a clean worktree and the win32 USERPROFILE isolation fix is a nice catch.
One real bug before merge: the PreToolUse matcher in hooks.droid.json was carried over from the Claude manifest as Edit|Write|Read|Glob|Grep, but Droid has no Write tool. File creation there is Create, so memory enrichment never fires on file creation. Change the matcher to Edit|Create|Read|Glob|Grep (pre-tool-use.mjs already handles create, only the matcher blocks it).
Two non-blocking notes: the test comment saying PreCompact/Stop are Claude-only is wrong, Droid supports Stop/SubagentStop/PreCompact/Notification too, so wiring stop.mjs is a natural follow-up; and the droid comment in guidelines.ts about no auto-capture hooks goes stale once this lands, fine to update here or after.
Droid has no Write tool — file creation is Create — so the Claude-era matcher blocked enrichment on new files. Align the manifest and test. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Author
|
@rohitg00 Thanks for the review — fixed the blocking matcher bug. Done
On the non-blocking notes
Ready for another look when you have a moment. |
rohitg00
reviewed
Aug 2, 2026
|
|
||
| ## [Unreleased] | ||
|
|
||
| ### Added |
Contributor
Author
There was a problem hiding this comment.
Removed — CHANGELOG Unreleased entry is gone.
rohitg00
reviewed
Aug 2, 2026
| | **Continue.dev** | `~/.continue/config.yaml` (preferred) or `config.json` (legacy) | `agentmemory connect continue` creates `config.yaml` from scratch when neither exists, or modifies existing `config.json`. **If you already have `config.yaml`** the adapter prints the exact block to paste under `mcpServers:` — it won't silently rewrite your yaml because preserving comments and anchors safely needs a YAML parser the package doesn't ship. Continue uses array form (not object) for `mcpServers`. | | ||
| | **Zed** | `~/.config/zed/settings.json` | `agentmemory connect zed` writes under `context_servers` (Zed's key, NOT `mcpServers`). Remote MCP servers can be wired via `{"url": "..."}` instead. | | ||
| | **Droid (Factory.ai)** | `~/.factory/mcp.json` | `agentmemory connect droid` writes the standard `mcpServers` block. Project-scoped overrides go in `<repo>/.factory/mcp.json`. The `/mcp` slash command inside droid lists configured servers. | | ||
| | **Droid (Factory.ai)** | `~/.factory/mcp.json` + `~/.factory/hooks.json` | `agentmemory connect droid` writes the standard `mcpServers` block. Project-scoped overrides go in `<repo>/.factory/mcp.json`. The `/mcp` slash command inside droid lists configured servers. Run `agentmemory connect droid --with-hooks` to also wire Droid's native hooks (SessionStart, UserPromptSubmit, PreToolUse, PostToolUse, SessionEnd) into `~/.factory/hooks.json` for zero-effort auto-capture, same merge/re-install behavior as the Codex and Claude Code hook installers. | |
Contributor
Author
There was a problem hiding this comment.
Trimmed — restored the short Droid row and left only a brief --with-hooks mention.
Per review: remove the Unreleased CHANGELOG addition, and keep the Droid agents-table row short with a brief --with-hooks mention. Co-authored-by: Cursor <cursoragent@cursor.com>
rohitg00
approved these changes
Aug 2, 2026
rohitg00
left a comment
rohitg00
left a comment
Owner
There was a problem hiding this comment.
Thanks @berthojoris for your contribution, If you'd like to test - please test antigravity, vscode, copilot, and codex plugins, CLIs thoroughly with pulling from main and fix the changes if anything is broken and submit PR. Happy to review them.
Contributor
Author
|
@rohitg00 Thanks — I'll pull latest main, thoroughly test the antigravity, vscode, copilot, and codex plugins/CLIs, and open a follow-up PR if anything is broken. |
berthojoris
added a commit
to berthojoris/agentmemory
that referenced
this pull request
Aug 3, 2026
Antigravity ships two products with unrelated configuration: the IDE, already wired by `connect antigravity`, and the `agy` CLI, which reads its customizations out of ~/.gemini/ and until now was not wired at all. This adds `connect antigravity-cli` for the latter — MCP via ~/.gemini/config/mcp_config.json, plus optional native auto-capture hooks behind --with-hooks. Unlike Droid (rohitg00#1130), the Codex merge engine could not be reused. The Antigravity hooks contract differs in three ways: * hooks.json is a map of *named* hook bundles at the root, not the `{ hooks: { <Event>: [...] } }` envelope, so antigravity-hooks.ts implements a merge that owns top-level keys instead of per-event entries. User-authored bundles are preserved; a re-install replaces only the bundle whose commands point under the bundled plugin dir. * only five events exist (PreToolUse, PostToolUse, PreInvocation, PostInvocation, Stop) — no SessionStart/SessionEnd/UserPromptSubmit, so the session lifecycle is synthesized from the first PreInvocation and from Stop. PostInvocation is left unwired to avoid double-capture. * the stdin payload is camelCase and nested (`toolCall.args` with PascalCase keys, `conversationId`, `workspacePaths`), and stdout must be a JSON object — `pre-tool-use.mjs` writes raw prose when context injection is on. plugin/scripts/antigravity-bridge.mjs bridges all three: it normalizes the payload onto the shape the bundled hooks already accept, maps Cascade tool names (view_file, replace_file_content, …) onto the read/edit/write/grep vocabulary the capture heuristics use, pipes to the right script, discards child stdout and always answers `{}` so Antigravity's own permission decisions are never overridden. Event names, tool names and arg keys were verified against the shipped agy binary rather than docs alone (docs disagree on the global hooks path); the customization dir is ~/.gemini/config/, matching where agy already keeps mcp_config.json and plugins/. Signed-off-by: Bertho Joris <bertho_joris@yahoo.co.id>
3 tasks
rohitg00
pushed a commit
that referenced
this pull request
Aug 3, 2026
* feat(cli): native hooks adapter for Antigravity CLI (agy) Antigravity ships two products with unrelated configuration: the IDE, already wired by `connect antigravity`, and the `agy` CLI, which reads its customizations out of ~/.gemini/ and until now was not wired at all. This adds `connect antigravity-cli` for the latter — MCP via ~/.gemini/config/mcp_config.json, plus optional native auto-capture hooks behind --with-hooks. Unlike Droid (#1130), the Codex merge engine could not be reused. The Antigravity hooks contract differs in three ways: * hooks.json is a map of *named* hook bundles at the root, not the `{ hooks: { <Event>: [...] } }` envelope, so antigravity-hooks.ts implements a merge that owns top-level keys instead of per-event entries. User-authored bundles are preserved; a re-install replaces only the bundle whose commands point under the bundled plugin dir. * only five events exist (PreToolUse, PostToolUse, PreInvocation, PostInvocation, Stop) — no SessionStart/SessionEnd/UserPromptSubmit, so the session lifecycle is synthesized from the first PreInvocation and from Stop. PostInvocation is left unwired to avoid double-capture. * the stdin payload is camelCase and nested (`toolCall.args` with PascalCase keys, `conversationId`, `workspacePaths`), and stdout must be a JSON object — `pre-tool-use.mjs` writes raw prose when context injection is on. plugin/scripts/antigravity-bridge.mjs bridges all three: it normalizes the payload onto the shape the bundled hooks already accept, maps Cascade tool names (view_file, replace_file_content, …) onto the read/edit/write/grep vocabulary the capture heuristics use, pipes to the right script, discards child stdout and always answers `{}` so Antigravity's own permission decisions are never overridden. Event names, tool names and arg keys were verified against the shipped agy binary rather than docs alone (docs disagree on the global hooks path); the customization dir is ~/.gemini/config/, matching where agy already keeps mcp_config.json and plugins/. Signed-off-by: Bertho Joris <bertho_joris@yahoo.co.id> * fix(cli): keep $-bearing plugin paths literal when resolving hook commands resolveBundle() expanded ${CLAUDE_PLUGIN_ROOT} via String.prototype.replace with a string argument, so a plugin root containing `$$`, `$&`, "$`" or `$'` was read as a replacement pattern and rewritten: C:/plug$&in -> C:/plug${CLAUDE_PLUGIN_ROOT}in/scripts/... C:/plug$$in -> C:/plug$in/scripts/... `$1` and `$<name>` are unaffected — the regex has no capture groups. Switching to a replacer function keeps the path verbatim. The failure mode this closes is silent: the hook installs with a broken command and auto-capture simply never fires. Regression test builds the manifest against a temp plugin root named `plug$&$$in` and asserts the resolved command contains it literally. Reported by CodeRabbit on #1146. Signed-off-by: Bertho Joris <bertho_joris@yahoo.co.id> * fix(antigravity): emit an explicit allow decision from the PreToolUse hook Antigravity documents `decision` as a required field of PreToolUse hook output, and agy treats a response that omits it as a denial: the bare `{}` the bridge used to write made the agent refuse every matched tool call (reported against agy 1.0.5 in cmux#5358) instead of passively capturing it. `responseFor` now answers PreToolUse with `{"decision":"allow"}` and leaves every other event on `{}`, so no event that carries no permission decision starts overriding the user's own settings. The response is written from the `finally` block, so a failed capture or an unparseable payload still produces the contract rather than empty stdout, which PreToolUse would read the same way as `{}`. Tests cover both the pure contract and the built bundled script running end to end with no server listening. Also extends the ARG_KEY_MAP test to every mapped key and pins that an explicit canonical key wins over a PascalCase alias. * fix(antigravity): match agy's real hooks.json schema, verified against 1.0.15 Three defects found by probing a live agy 1.0.15 with an instrumented hook, each of which stopped the adapter from capturing anything at all. Lifecycle events take a flat handler list, not the tool-event wrapper. agy parses `PreToolUse`/`PostToolUse` as `[{matcher, hooks: [...]}]` but `PreInvocation`/`PostInvocation`/`Stop` as a bare `[{type, command}]`, since there is no tool name to match on. Wrapping a lifecycle event makes agy read the wrapper itself as a handler and reject the *whole file* with `invalid hook "agentmemory": command hook must specify 'command'` — so the mis-shaped Stop entry disabled every hook in the bundle, and would have disabled hooks other tools had written to the same file. `command` is not run through a shell and quotes are not stripped, so the quoted path resolved to a module name that literally began with a double quote: `Cannot find module 'C:\Users\…\.gemini\config\"C:\…\bridge.mjs"'`. Commands are now bare. That also means a path containing spaces cannot be expressed at all — quoted and unquoted both fail — so the installer refuses with an explanation instead of writing hooks that can only fail at tool time. The merge engine reads both shapes when deciding which bundles agentmemory owns, so a re-install over the old wrapped layout still replaces it rather than leaving a second copy behind. Tests pin both event shapes, the absence of quotes, the space check, and normalization of a payload captured verbatim from the live run — which also confirms `conversationId`, PascalCase `toolCall.args`, and that agy sends no `cwd` key at all. * refactor(antigravity): cut comment volume to match the sibling adapters The bundled script carried 24 comment lines where every other script in plugin/scripts has three. The bundler strips `//` comments but preserves JSDoc blocks, so the fix is to document the bridge's exported helpers with line comments: the explanations stay in source and the generated artifact comes out as clean as its siblings. The connect adapter and merge engine restated the same facts in a file header and again in a per-function block. Kept one statement of each, dropped the repetition, and left the verified agy behaviour in place since that is the part not derivable from the code. --------- Signed-off-by: Bertho Joris <bertho_joris@yahoo.co.id>
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
2 participants
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
Droid CLI (Factory.ai) currently only gets wired via MCP (
agentmemory connect droid), even though Droid ships its own native, first-party hooks system — a real gap compared to Claude Code and Codex, which both get auto-capture hooks alongside MCP.This PR adds that missing coverage:
agentmemory connect droid --with-hooksnow also installs Droid's native~/.factory/hooks.json, giving it the same zero-effort auto-capture that Claude Code and Codex already have.What changed
plugin/hooks/hooks.droid.json(new) — bundled hook manifest for the 5 lifecycle events Droid documents (SessionStart,UserPromptSubmit,PreToolUse,PostToolUse,SessionEnd; per docs.factory.ai/cli/configuration/hooks-guide). Reuses the same bundledplugin/scripts/*.mjshook scripts Claude Code and Codex already call — no new hook logic, just new wiring.src/cli/connect/droid.ts— addsinstallDroidHooks(), which merges the bundled manifest into~/.factory/hooks.jsonusing the existingbuildMergedHooks()engine fromcodex-hooks.ts(same shape:{ hooks: { <event>: [{ matcher?, hooks: [{ type, command }] }] } }). Idempotent re-install: agentmemory's own entries are identified and replaced by their<pluginRoot>/scripts/path prefix, so user-authored hook entries in the same file are never touched.src/cli/connect/json-mcp-adapter.ts—createJsonMcpAdapter()gained an optionalinstallHookscallback, invoked whenever--with-hooksis passed (on both the fresh-install and already-wired paths, mirroring the Claude Code / Codex adapters). This lets any future JSON-MCP-shaped adapter opt into the same native-hooks pattern without a bespokeinstall()rewrite.src/cli/connect/codex-hooks.ts,types.ts— doc comments updated;buildMergedHooks/--with-hooksnow back three hosts (Codex, Claude Code, Droid), not just Codex, and the code was already host-agnostic (manifest filename is a parameter) so no logic changes were needed there.README.md,plugin/skills/agentmemory-agents/REFERENCE.md,CHANGELOG.md— docs updated to describe the new--with-hookspath for Droid. The agents skill reference is auto-generated (npm run skills:gen) fromsrc/cli/connect/index.ts'sprotocolNote, so it now reflects the new capability automatically.Why this approach
Droid's native hooks config uses the exact same JSON shape (
matcher+hooks: [{ type, command }]) as Codex's~/.codex/hooks.jsonworkaround, so this reusesbuildMergedHooks()as-is rather than writing a third merge implementation — only the bundled manifest file passed in differs per host.Test plan
npx tsc --noEmit— clean on every file touched by this PR (pre-existing unrelated errors elsewhere in the repo are untouched by this change).test/droid-connect-hooks.test.ts: manifest merge produces the correct 5 events (and explicitly asserts Claude-only events likePreCompact/Stopdo not leak in), preserves thePreToolUsematcher, appends to/doesn't clobber user-authored hooks, and re-install is idempotent.test/connect-new-agents.test.ts(Droid describe block):--with-hookswrites~/.factory/hooks.jsonwith all 5 events; omitting the flag does not touchhooks.json; re-running--with-hookson an already-wired MCP config still refreshes the hooks file.npm testrun — all Droid/connect-specific tests pass; the handful of pre-existing failures elsewhere in the suite were confirmed to reproduce identically on unmodifiedmain(unrelated Windows-path/env flakiness, not caused by this change).test/connect-new-agents.test.tshad a pre-existing Windows test-isolation bug —os.homedir()readsUSERPROFILEon win32, notHOME, so the "isolated temp home" trick silently no-op'd and adapter tests were writing into the real developer's home directory (discovered the hard way while validating this PR — confirmed via git-stash bisection that it's not introduced by this change, just uncovered by it). Fixed by also setting/restoringUSERPROFILEalongsideHOMEin that file'sbeforeEach/afterEach. The same pattern exists in 6 other test files (slots-flag-gate.test.ts,preferences.test.ts,env-loader.test.ts,consolidation-default.test.ts,cli-onboarding.test.ts,cli-connect.test.ts) but those are left untouched here to keep this PR scoped — flagging for a follow-up.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
agentmemory connect droid --with-hooks.Documentation