diff --git a/CHANGELOG.md b/CHANGELOG.md index 2d811fda..e4c241bb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,24 @@ source layout, CLI surface, and state schema are stabilizing but may still chang ### Added +- **Factory Droid hooks.** `apply` writes command hooks to `~/.factory/hooks.json` + and `.factory/hooks.json`. `timeout` is seconds, the unit Droid documents. + Events Droid does not have, such as `PostCompact`, are skipped, so the shared + `hooks/.toml` other agents use is left alone. Matchers are written + as-is and Droid uses its own tool names (`Execute`, `Edit`, …), so a matcher + written for Claude Code's names matches nothing there. +- **`import factory` reads Factory hooks.** It reads `hooks.json`, or the `hooks` + key of `settings.json` when `hooks.json` does not exist yet, which is where + Droid reads them from too. An event with a field agentsync cannot represent + (`commandRegex`, an explicit `timeout` of `0`) is left uncaptured with a + warning instead of being captured without it. +- **The first apply keeps your `settings.json` hooks running.** Once + `hooks.json` exists, Droid stops reading the `hooks` key in `settings.json`. + So the apply that creates `hooks.json` copies those hooks in first, and only + the events agentsync renders replace what was copied. If one of those + replaces handlers you had in `settings.json`, apply now warns and names the + event, so a hook does not quietly stop firing. `settings.json` itself is + never edited. - **Hook handlers can now carry a `timeout`.** Add `timeout = 30` to an entry in `hooks/.toml` and the handler is allowed 30 seconds across Claude Code, Codex CLI, Cursor, Gemini CLI and Grok Build. Before this, `timeout` was a @@ -453,6 +471,17 @@ source layout, CLI surface, and state schema are stabilizing but may still chang ### Changed +- **Factory skills move to `~/.agents/skills/`.** Droid scans that shared + directory as well as its own `.factory/skills/`, so writing both registered + every skill twice whenever Codex or another `.agents/skills` agent was also + enabled. The next apply after upgrading removes the skill copies agentsync + wrote under `.factory/skills/`, backing up any you edited first, and reports + them as removed files. Skills you created there yourself are left alone. +- **Factory user memory now renders to `~/.factory/AGENTS.md`.** Factory had + only a project memory file before. If you already keep a hand-written + `~/.factory/AGENTS.md`, the next apply backs it up to + `~/.agentsync/.state/backups/` and replaces it with the managed file; move + anything you want to keep into `~/.agentsync/memory/AGENTS.md`. - **`secret edit` no longer exits the process from a signal handler goroutine** ([#235](https://github.com/spxrogers/agentsync/issues/235)). SIGINT/SIGTERM while the decrypted vault was on disk removed the temp file and diff --git a/README.md b/README.md index 08e4c90e..6166a937 100644 --- a/README.md +++ b/README.md @@ -93,8 +93,9 @@ The ten **deep adapters** (rich, agent-specific, often bidirectional): | **Grok Build** | ✓ adapter | MCP (◐, TOML), instructions, complete skills, commands (◐), and command hooks (◐) at user and project scopes. [Supported paths and limits](docs/grok.md). Subagent/LSP projection and plugin import deferred. | Plus a **breadth tier** of 22 more via one data-driven generic adapter — **memory** -for all, **MCP** where the agent reads a JSON server-map (15 of 22), and **Agent -Skills** where the agent natively scans a `SKILL.md` directory (18 of 22): `amp`, +for all, **MCP** where the agent reads a JSON server-map (15 of 22), **Agent +Skills** where the agent natively scans a `SKILL.md` directory (18 of 22), and +command hooks for Factory only: `amp`, `goose`, `qwen`, `warp`, `jules`, `junie`, `openhands`, `amazonq`, `zed`, `kilocode`, `kiro`, `trae`, `jetbrains`, `firebase`, `antigravity`, `augmentcode`, `copilot`, `copilot-cli`, `crush`, `factory`, `pi`, `mistral`. Each is a *verified* diff --git a/docs/architecture.md b/docs/architecture.md index b468d947..7c6db750 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -248,7 +248,7 @@ published `Adapter` interface (`KeyMergeStrategy() string`) and is deferred to them, a single data-driven *generic* adapter (`internal/adapter/generic`) serves a long tail of agents from a verified `Spec` table — memory, (where expressible) MCP, and (where the agent scans a `SKILL.md` directory) Agent Skills, every other -component reported as a skip. Both kinds implement the same +component reported as a skip except Factory command hooks (see the capability matrix). Both kinds implement the same `Adapter` interface and register identically, so the rest of the pipeline (plan, classify, write, capture, state) treats them uniformly. The set of valid agent names is derived from the deep package list **plus** `generic.Specs()` (see @@ -673,8 +673,9 @@ type HookEventNamer interface { } ``` -- **`HookIngestGuard`** (claude, gemini, cursor, codex, grok — every hook-rendering - adapter) re-reads the destination and returns the hook events ingest +- **`HookIngestGuard`** (claude, gemini, cursor, codex, grok, and Factory — every + hook-rendering adapter; the other breadth-tier agents do not implement it) + re-reads the destination and returns the hook events ingest *semantically* refused: unmodeled fields on well-formed entries, plus non-command handlers where the adapter's render cannot round-trip them (claude, gemini, cursor, grok); codex re-renders non-command types verbatim, so @@ -696,7 +697,8 @@ type HookEventNamer interface { adapter uses in its owned `/hooks/` pointers (`PreToolUse` → `BeforeTool` / `preToolUse`). Canonical hooks are **shared** across agents, so a retirement disowns every agent's state key for the event at that scope - — and a renaming agent's key is only findable under its native spelling. + — a renaming agent's key under its native spelling, and Factory's key at + `/` because `hooks.json` is the event map rather than a `hooks` object. An adapter that renames without declaring would silently escape the disown and the next apply's orphan cleanup would delete the event from its native config; the registry-wide guard diff --git a/docs/capability-matrix.md b/docs/capability-matrix.md index 693f8903..cc6b215f 100644 --- a/docs/capability-matrix.md +++ b/docs/capability-matrix.md @@ -30,7 +30,7 @@ nothing is dropped silently. | **Roo Code** | ✅ Adapter (some components projected) | MCP, memory, and slash commands — clean filesystem `.roo/` paths (rulesync and ruler converged on these). **MCP → `.roo/mcp.json`** (project-level, `mcpServers` with explicit `type: streamable-http`/`sse` for remote + `url`/`headers`; merge-by-server-name preserves foreign servers). Roo's *global* MCP lives in VS Code globalStorage (OS/editor-specific), which agentsync intentionally does **not** target — so user-scope MCP is reported as a skip. **Memory → `.roo/rules/agentsync.md`** (plain-markdown always-applied rule) and **commands → `.roo/commands/.md`** (markdown + frontmatter — Roo keeps **both** `description` *and* `argument-hint`; only `allowed-tools` drops), both at **user and project scope** (`~/.roo/` + `/.roo/`). **Skills, hooks, and LSP** have no Roo concept, and Roo's "custom modes" are not per-file **subagents**, so all four are skipped. No `PluginIngester`; it still *receives* plugin-projected components on `apply`. | | **Cline** | ✅ Adapter (scope-asymmetric) | MCP, memory, and slash commands. **MCP → `~/.cline/mcp.json`** at **user scope** — the Cline CLI's clean config (`mcpServers`, transport inferred: stdio command/args/env, remote `url` + `headers`). Cline has no project MCP file, and its VS Code-extension MCP lives in OS/editor-specific globalStorage no config-sync tool writes, so project-scope MCP is reported as a skip. **Memory → `.clinerules/agentsync.md`** (plain markdown — Cline concatenates `.clinerules/`) and **commands → `.clinerules/workflows/.md`** (plain markdown workflows invoked as `/.md`; command frontmatter drops), both at **project scope** (Cline's global rules and workflows live in `~/Documents/Cline/`, a non-XDG app path agentsync deliberately does not target). **Skills, subagents, hooks, and LSP** have no Cline concept and are skipped. No `PluginIngester`; it still *receives* plugin-projected components on `apply`. | | **Grok Build** | ✅ Dedicated adapter | User/project instructions, skills with bundled files, legacy Markdown commands, TOML MCP, and JSON command hooks. Import captures the managed hook file only; unsupported handler fields are refused. See [Grok support](grok.md) for paths, trust requirements, verified upstream contracts, and limitations. | -| **Breadth tier (22 agents)** | ✅ Generic adapter (memory + MCP + skills) | A long tail of agents supported by one data-driven [generic adapter](#breadth-tier) — **memory** (rules file) for all, **MCP** where the agent reads a JSON server-map agentsync can express (15 of 22), and **Agent Skills** where the agent natively scans a `SKILL.md` directory (18 of 22). Each is a *verified* spec, not a hand-written package; see the [Breadth tier](#breadth-tier) table for per-agent coverage. They flow through the normal apply/import pipeline (drift, secrets, capture), unlike a one-way rules dump. | +| **Breadth tier (22 agents)** | ✅ Generic adapter (memory + MCP + skills; Factory hooks) | A long tail of agents supported by one data-driven [generic adapter](#breadth-tier) — **memory** (rules file) for all, **MCP** where the agent reads a JSON server-map agentsync can express (15 of 22), and **Agent Skills** where the agent natively scans a `SKILL.md` directory (18 of 22). **Factory** is the one row with command hooks. Each is a *verified* spec, not a hand-written package; see the [Breadth tier](#breadth-tier) table for per-agent coverage. They flow through the normal apply/import pipeline (drift, secrets, capture), unlike a one-way rules dump. | ## Plugin import/apply: the shared invariant @@ -159,7 +159,10 @@ the deep adapters use (`claude.SkillFileOps`) — bundled files and executable b survive byte-for-byte. Most agents read the cross-vendor **`.agents/skills/`** convention (the same directory Codex targets, so the render pipeline dedupes the byte-identical ops rather than fighting over the path); a few scan only their own -`./skills/` (Qwen, Junie, Kiro, Factory, Copilot's `.github/skills/`). The +`./skills/` (Qwen, Junie, Kiro, Copilot's `.github/skills/`). Factory +scans the shared `.agents/skills/` directory as a compatibility source and does +not get a second `.factory/skills/` copy. Droid overrides a cross-source name +clash rather than rejecting it; one copy is still the right render. The **four without skills** are the ones that don't natively scan a `SKILL.md` directory: **Jules** and **Firebase Studio** *publish* skills for *other* agents, **Amazon Q** consumes skills only through an MCP server (a different component), and @@ -193,10 +196,48 @@ guessed. | **copilot** | `.github/copilot-instructions.md` | ✓ `.vscode/mcp.json` (`servers` key, `type`) | ✓ `.github/skills/` · `~/.copilot/skills/` | | **copilot-cli** | `AGENTS.md` | ✓ `~/.copilot/mcp-config.json` (`type`, stdio="local") | ✓ `.agents/skills/` (+ user) | | **crush** | `AGENTS.md` | ✓ crush.json `mcp` key (+ user) | ✓ `.agents/skills/` · `~/.config/crush/skills/` | -| **factory** | `AGENTS.md` | ✓ `.factory/mcp.json` · `~/.factory/mcp.json` (`type`) | ✓ `.factory/skills/` (+ user) | +| **factory** | `AGENTS.md` · `~/.factory/AGENTS.md` | ✓ `.factory/mcp.json` · `~/.factory/mcp.json` (`type`) | ✓ `.agents/skills/` (+ user) | | **pi** | `AGENTS.md` · `~/.pi/agent/AGENTS.md` | ✓ `~/.pi/agent/mcp.json` | ✓ `.agents/skills/` (+ user) | | **mistral** | `AGENTS.md` | ✗ TOML `.vibe/config.toml` | ✓ `.agents/skills/` · `~/.vibe/skills/` | +**Factory hooks** are the one breadth-tier exception to "no hooks". Droid's +`~/.factory/hooks.json` and `.factory/hooks.json` are an event map at the file +root (command hooks only; `timeout` is seconds, the same unit as the canonical +field). `PostCompact` is not a Droid event and is skipped, not refused, so the +shared `hooks/PostCompact.toml` other agents use is left alone. A group field +Droid supports and agentsync does not (`commandRegex`) refuses that whole +event rather than being stripped on the next apply. If `hooks.json` is absent, +import reads the `hooks` object in the matching `settings.json`. The first apply +that creates `hooks.json` copies that object in as the merge base, then overlays +only the events agentsync is rendering, so a `commandRegex` group or any other +unmodeled sibling keeps running after Droid stops reading `settings.json`. Those +copied events are not recorded as owned, so a later apply does not orphan-delete +them. An event agentsync **does** render replaces the copied array for that +event, so a `settings.json` handler under the same event stops running; apply +warns, naming the event, unless the copied handlers are the same ones canonical +renders (the usual `import` then `apply` case). `settings.json` is never edited. +The legacy `.factory/hooks/hooks.json` path still loads beside `hooks.json` +until Droid's next save archives it to `hooks/hooks.migrated.json`; apply warns +and does not move it. Import retirement disowns Factory's `/` state +pointer, not only the `/hooks/` spelling the nested adapters use. + +**Factory matchers are written verbatim, and Droid's tool names are its own.** +A canonical `matcher` reaches `hooks.json` unchanged. Droid's matchers are +case-sensitive, accept regexes, and name Droid's tools: `Execute`, `Read`, +`Edit`, `Create`, `ApplyPatch`, `LS`, `Glob`, `Grep`, `Task`, `FetchUrl`, +`WebSearch`, and `mcp____` +([hooks reference](https://docs.factory.ai/reference/hooks-reference)). A +matcher written for Claude Code's names (`Bash`, `Write`, …) is valid JSON but +matches nothing in Droid. agentsync does not translate tool names for any +agent, so a hook meant for Droid needs a Droid matcher, `*`, or no matcher. + +**Upgrading moves two Factory paths.** Skills now render to `.agents/skills/` +instead of `.factory/skills/`; the next apply removes the copies agentsync wrote +under `.factory/skills/` (backing up any you edited first) and leaves skills +you created there yourself alone. User memory now renders to +`~/.factory/AGENTS.md`; an existing hand-written file there is backed up, then +replaced by the managed file, like any other first apply over a foreign file. + **Deliberate exclusions:** **Aider** (no native MCP; memory only via an `.aider.conf.yml` `read:` pointer — needs a content+config-pointer adapter, out of the generic tier) and **Firebender** (single-sourced JSON-manifest model) are not diff --git a/docs/components.md b/docs/components.md index ea1dc8b9..8197f9c2 100644 --- a/docs/components.md +++ b/docs/components.md @@ -454,7 +454,8 @@ many agents from a table of verified `Spec`s (`specs.go`) rather than a package each. Covers **memory** (a rules/instructions file, plain markdown), **MCP** where the agent reads a JSON server-map agentsync can express, and **Agent Skills** (`SKILL.md` directories) where the agent natively scans a skills directory — every -other component is reported as a skip. A `Spec` declares per-scope memory/MCP/skills +other component is reported as a skip, except Factory command hooks (`hooks.json`; +only that registered wrapper implements `HookIngestGuard`). A `Spec` declares per-scope memory/MCP/skills paths plus MCP "dialect" knobs that capture the tail's variance (top-level key `mcpServers`/`servers`/`mcp`/`context_servers`/the flat namespaced `amp.mcpServers`; transport field `type`/`transport`/inferred; stdio value `stdio`/`local`; remote diff --git a/docs/user-guide.md b/docs/user-guide.md index 271b6ac7..aa1a4b7b 100644 --- a/docs/user-guide.md +++ b/docs/user-guide.md @@ -982,8 +982,8 @@ Claude, OpenCode, Codex, Cursor, Gemini CLI, Continue, Windsurf, Roo Code, Cline Beyond these ten deep adapters, a **breadth tier** of 22 more agents (amp, goose, qwen, warp, zed, kiro, junie, factory, copilot, crush, …) is supported via one data-driven generic adapter — memory for all, MCP where the agent reads a JSON -server-map, and Agent Skills (`SKILL.md` directories) where the agent natively scans -a skills directory. Run `agentsync agent list --all` to see them all; see the +server-map, Agent Skills (`SKILL.md` directories) where the agent natively scans +a skills directory, and command hooks for Factory only. Run `agentsync agent list --all` to see them all; see the [capability matrix → Breadth tier](capability-matrix.md#breadth-tier) for per-agent coverage. diff --git a/internal/adapter/generic/apply.go b/internal/adapter/generic/apply.go index 887f2182..f735fad9 100644 --- a/internal/adapter/generic/apply.go +++ b/internal/adapter/generic/apply.go @@ -1,7 +1,13 @@ package generic import ( + "encoding/json" "fmt" + "io" + "os" + "path/filepath" + "reflect" + "sort" "github.com/spxrogers/agentsync/internal/adapter" "github.com/spxrogers/agentsync/internal/jsonkeys" @@ -28,17 +34,143 @@ func (a *Adapter) applyWrite(op adapter.FileOp, w adapter.DestWriter) error { if op.MergeStrategy != "merge-jsonc-keys" { return w.Write(op, op.Content) } - existing, err := adapter.ReadExisting(op.Path) + // ReadFileOptional rather than ReadExisting: the seed below needs to know + // the destination is ABSENT, and ReadExisting folds absent into a nil + // error. Testing errors.Is(err, fs.ErrNotExist) after it would never fire. + existing, present, err := adapter.ReadFileOptional(op.Path) if err != nil { return fmt.Errorf("read %s: %w", op.Path, err) } + var seeded map[string]any + if !present { + // Droid reads settings.json "hooks" only while hooks.json is absent, + // then ignores that key once hooks.json exists. Seed the new file from + // that object so a first apply does not drop events this render does + // not own (a commandRegex group, a sibling Stop, timeout 0). The seed + // is the merge base, not op.Content, so RecordOpsState does not record + // those pointers as owned and a later apply cannot orphan-delete them. + existing, seeded = a.factoryHooksSeed(op.Path) + } + if a.isFactoryHooksFile(op.Path) { + a.warnFactoryLegacyHooks(op.Path) + } ours, err := jsonkeys.DecodeObject(op.Content) if err != nil { return fmt.Errorf("parse our payload for %s: %w", op.Path, err) } + a.warnSeededEventsReplaced(op.Path, seeded, ours) merged, err := jsonkeys.MergeJSONC(existing, ours, op.OwnedKeys) if err != nil { return fmt.Errorf("merge %s: %w", op.Path, err) } return w.Write(op, merged) } + +func (a *Adapter) isFactoryHooksFile(path string) bool { + if a.spec.Hooks.User == "" && a.spec.Hooks.Project == "" { + return false + } + return filepath.Base(path) == "hooks.json" +} + +// factoryHooksSeed returns the settings.json "hooks" object twice: encoded as +// the merge base for a new hooks.json, and decoded so applyWrite can tell the +// user which of those events the render replaces. Both are nil when there is +// nothing to preserve. A missing or unreadable settings file is not an apply +// failure: the hooks.json write still proceeds, and a read or shape problem is +// warned rather than blocking every other component. +func (a *Adapter) factoryHooksSeed(hooksPath string) ([]byte, map[string]any) { + if !a.isFactoryHooksFile(hooksPath) { + return nil, nil + } + settings := filepath.Join(filepath.Dir(hooksPath), "settings.json") + // Through ReadFileOptional like every other destination read (#241/#242): + // a FIFO at settings.json must not hang apply in open(2). + data, present, err := adapter.ReadFileOptional(settings) + if err != nil { + fmt.Fprintf(a.warn(), "warning: could not read %s to preserve its hooks in hooks.json: %v\n", settings, err) + return nil, nil + } + if !present { + return nil, nil + } + doc, err := jsonkeys.DecodeJSONC(data) + if err != nil { + fmt.Fprintf(a.warn(), "warning: could not parse %s to preserve its hooks in hooks.json: %v\n", settings, err) + return nil, nil + } + rawHooks, hasHooks := doc["hooks"] + if !hasHooks { + return nil, nil + } + hooks, ok := rawHooks.(map[string]any) + if !ok { + // Import warns about this shape too; staying silent here would let + // apply create hooks.json, after which Droid never reads that key again. + fmt.Fprintf(a.warn(), "warning: %s hooks value is not an object; nothing from it is carried into hooks.json\n", settings) + return nil, nil + } + if len(hooks) == 0 { + return nil, nil + } + body, err := json.Marshal(hooks) + if err != nil { + return nil, nil + } + return body, hooks +} + +// warnSeededEventsReplaced reports every settings.json event whose handlers +// stop running because this render owns that event in the new hooks.json. +// +// The seed keeps events agentsync does not render, but an event it DOES render +// is replaced by the canonical array. Nothing is deleted (settings.json is +// left as it was), yet Droid stops reading that key once hooks.json exists, so +// without this the user's handler would simply stop firing. +// +// The comparison is semantic, through the same parser import uses, not a byte +// compare. The common path is `agentsync import factory` (which reads the +// settings.json fallback) followed by `apply`; byte-comparing would warn there +// because a settings.json handler written without "matcher" or "type" +// re-renders with both. A seeded event agentsync cannot represent (a +// commandRegex group, say) never equals a rendered one, so it always warns. +func (a *Adapter) warnSeededEventsReplaced(hooksPath string, seeded, ours map[string]any) { + if len(seeded) == 0 { + return + } + events := make([]string, 0, len(ours)) + for event := range ours { + if _, ok := seeded[event]; ok { + events = append(events, event) + } + } + sort.Strings(events) + settings := filepath.Join(filepath.Dir(hooksPath), "settings.json") + for _, event := range events { + if sameFactoryEvent(event, seeded[event], ours[event]) { + continue + } + fmt.Fprintf(a.warn(), "warning: the %q hooks in %s will stop running: agentsync now writes %q to %s, "+ + "which Droid reads instead of settings.json. Add them to hooks/%s.toml to keep them\n", + event, settings, event, hooksPath, event) + } +} + +// sameFactoryEvent reports whether two native values for one event describe +// the same handlers once parsed the way import parses them. +func sameFactoryEvent(event string, native, rendered any) bool { + nativeHooks, nativeRefused := parseFactoryHooks(map[string]any{event: native}, io.Discard) + if len(nativeRefused) > 0 { + return false + } + renderedHooks, _ := parseFactoryHooks(map[string]any{event: rendered}, io.Discard) + return len(nativeHooks) > 0 && reflect.DeepEqual(nativeHooks, renderedHooks) +} + +func (a *Adapter) warnFactoryLegacyHooks(hooksPath string) { + legacy := filepath.Join(filepath.Dir(hooksPath), "hooks", "hooks.json") + if _, err := os.Stat(legacy); err != nil { + return + } + fmt.Fprintf(a.warn(), "warning: %s still loads after hooks.json exists; Droid archives it to hooks/hooks.migrated.json on the next save in Droid\n", legacy) +} diff --git a/internal/adapter/generic/factory_hooks_edges_test.go b/internal/adapter/generic/factory_hooks_edges_test.go new file mode 100644 index 00000000..bed8cbbd --- /dev/null +++ b/internal/adapter/generic/factory_hooks_edges_test.go @@ -0,0 +1,441 @@ +package generic_test + +import ( + "bytes" + "fmt" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/spxrogers/agentsync/internal/adapter" + "github.com/spxrogers/agentsync/internal/adapter/generic" + "github.com/spxrogers/agentsync/internal/jsonkeys" + "github.com/spxrogers/agentsync/internal/render" + "github.com/spxrogers/agentsync/internal/secrets" + "github.com/spxrogers/agentsync/internal/source" + "github.com/spxrogers/agentsync/internal/state" + "github.com/spxrogers/agentsync/internal/testenv" + "github.com/spxrogers/agentsync/internal/untrusted" +) + +// readHooksJSON parses a Factory hooks.json. Assertions go through the parsed +// document rather than substring matches, which pass on a value that landed +// under the wrong event or in a key Droid never reads. +func readHooksJSON(t *testing.T, path string) map[string]any { + t.Helper() + raw, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read %s: %v", path, err) + } + doc, err := jsonkeys.DecodeObject(raw) + if err != nil { + t.Fatalf("parse %s: %v\n%s", path, err, raw) + } + return doc +} + +// firstDef returns the first matcher group of an event, failing (not +// panicking) on any shape mismatch. +func firstDef(t *testing.T, doc map[string]any, event string) map[string]any { + t.Helper() + defs, ok := doc[event].([]any) + if !ok || len(defs) == 0 { + t.Fatalf("hooks.json %q is %T (len 0?), want a non-empty array: %#v", event, doc[event], doc) + } + def, ok := defs[0].(map[string]any) + if !ok { + t.Fatalf("hooks.json %q[0] is %T, want an object", event, defs[0]) + } + return def +} + +// eventCommands lists every handler command under one event, in order. An +// absent event is an empty list. +func eventCommands(t *testing.T, doc map[string]any, event string) []string { + t.Helper() + raw, present := doc[event] + if !present { + return nil + } + defs, ok := raw.([]any) + if !ok { + t.Fatalf("hooks.json %q is %T, want an array", event, raw) + } + var out []string + for i, d := range defs { + def, ok := d.(map[string]any) + if !ok { + t.Fatalf("hooks.json %q[%d] is %T, want an object", event, i, d) + } + handlers, ok := def["hooks"].([]any) + if !ok { + t.Fatalf("hooks.json %q[%d].hooks is %T, want an array", event, i, def["hooks"]) + } + for j, h := range handlers { + handler, ok := h.(map[string]any) + if !ok { + t.Fatalf("hooks.json %q[%d].hooks[%d] is %T, want an object", event, i, j, h) + } + cmd, _ := handler["command"].(string) + out = append(out, cmd) + } + } + return out +} + +func writeFactoryFile(t *testing.T, dir, name, body string) string { + t.Helper() + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + p := filepath.Join(dir, name) + if err := os.WriteFile(p, []byte(body), 0o644); err != nil { + t.Fatal(err) + } + return p +} + +func renderApplyFactory(t *testing.T, a adapter.Adapter, c source.Canonical) error { + t.Helper() + ops, _, err := a.Render(secrets.ForRender(c), adapter.ScopeUser, "") + if err != nil { + t.Fatalf("Render: %v", err) + } + return a.Apply(ops, adapter.PassThroughWriter{}) +} + +// TestApply_FactoryHooks_SeedWarnsWhenAnOwnedEventReplacesUserHooks is the +// review's repro. settings.json has a Stop handler; canonical also renders +// Stop. Creating hooks.json makes Droid stop reading settings.json, and the +// canonical array replaces the seeded one, so the user's handler silently +// stops running. Nothing is deleted, but the user must hear about it. +// +// The SessionStart row is the control that keeps the warning honest: the same +// handler written without "matcher" (as settings.json commonly has it) is what +// `agentsync import factory` captures and `apply` re-renders WITH one. Warning +// there would fire on the ordinary import-then-apply path. +func TestApply_FactoryHooks_SeedWarnsWhenAnOwnedEventReplacesUserHooks(t *testing.T) { + testenv.RequireContainer(t) + tmp := t.TempDir() + dir := filepath.Join(tmp, ".factory") + settingsPath := writeFactoryFile(t, dir, "settings.json", `{ + "hooks": { + "Stop": [ { "matcher": "", "hooks": [ { "type": "command", "command": "echo user-stop" } ] } ], + "SessionStart": [ { "hooks": [ { "command": "bd prime", "timeout": 3 } ] } ] + } + }`) + settingsBefore, err := os.ReadFile(settingsPath) + if err != nil { + t.Fatal(err) + } + + var warn bytes.Buffer + a := generic.New(factorySpec(t), generic.Options{TargetRoot: tmp, Stderr: &warn}) + err = renderApplyFactory(t, a, source.Canonical{Hooks: []source.Hook{ + {Event: untrusted.Wrap("Stop"), Type: "command", Command: "echo canonical-stop"}, + {Event: untrusted.Wrap("SessionStart"), Type: "command", Command: "bd prime", Timeout: 3}, + }}) + if err != nil { + t.Fatal(err) + } + + doc := readHooksJSON(t, filepath.Join(dir, "hooks.json")) + if got := eventCommands(t, doc, "Stop"); len(got) != 1 || got[0] != "echo canonical-stop" { + t.Fatalf("Stop = %v; agentsync owns Stop, so hooks.json carries the canonical handler only", got) + } + + out := warn.String() + for _, want := range []string{`"Stop"`, "settings.json", "stop running", "hooks/Stop.toml"} { + if !strings.Contains(out, want) { + t.Fatalf("replacement warning must contain %q so the user can act on it; got:\n%s", want, out) + } + } + if strings.Contains(out, `"SessionStart"`) { + t.Fatalf("SessionStart is the same handler on both sides; warning about it is a false alarm on the import-then-apply path:\n%s", out) + } + + settingsAfter, err := os.ReadFile(settingsPath) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(settingsBefore, settingsAfter) { + t.Fatal("settings.json must be left exactly as it was; the user's handler is reported, never edited") + } + + // A second apply has nothing left to seed (hooks.json exists), so it must + // not repeat the warning. + warn.Reset() + if err := renderApplyFactory(t, a, source.Canonical{Hooks: []source.Hook{ + {Event: untrusted.Wrap("Stop"), Type: "command", Command: "echo canonical-stop"}, + }}); err != nil { + t.Fatal(err) + } + if strings.Contains(warn.String(), "stop running") { + t.Fatalf("the warning belongs to the apply that creates hooks.json, not every apply after:\n%s", warn.String()) + } +} + +// TestFactoryHooks_MalformedFiles pins what each malformed native shape does +// on all three paths. The invariants, per row: import never captures a guess +// and never REFUSES (a refusal retires the shared canonical file for every +// agent, and a typo must not do that); apply never replaces a hooks.json it +// cannot parse; a bad settings.json never blocks writing hooks.json and is +// never edited. +func TestFactoryHooks_MalformedFiles(t *testing.T) { + testenv.RequireContainer(t) + tests := []struct { + name string + hooksJSON string // "" = absent + settingsJSON string // "" = absent + wantIngestErr bool // import refuses loudly rather than reading nothing + wantApplyErr bool // apply refuses rather than overwrite what it cannot parse + wantWarn string // substring of a warning from ingest or apply + }{ + {name: "hooks.json is not valid JSON", hooksJSON: `{not json`, wantIngestErr: true, wantApplyErr: true}, + {name: "hooks.json root is an array", hooksJSON: `[ { "Stop": [] } ]`, wantIngestErr: true, wantApplyErr: true}, + { + name: "hooks.json event value is not an array", + hooksJSON: `{ "Stop": { "hooks": [] } }`, + wantWarn: "is not an array", + }, + { + name: "settings.json is not valid JSON", + settingsJSON: `{not json`, + wantIngestErr: true, + wantWarn: "could not parse", + }, + { + name: "settings.json root is an array", + settingsJSON: `[ { "hooks": {} } ]`, + wantIngestErr: true, + wantWarn: "could not parse", + }, + { + // The phrase is apply's, not import's: import warns about this shape + // too, so matching its wording would not catch a silent apply. + name: `settings.json "hooks" is not an object`, + settingsJSON: `{ "hooks": [ "echo hi" ] }`, + wantWarn: "nothing from it is carried into hooks.json", + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + tmp := t.TempDir() + dir := filepath.Join(tmp, ".factory") + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + hooksPath := filepath.Join(dir, "hooks.json") + settingsPath := filepath.Join(dir, "settings.json") + if tc.hooksJSON != "" { + writeFactoryFile(t, dir, "hooks.json", tc.hooksJSON) + } + if tc.settingsJSON != "" { + writeFactoryFile(t, dir, "settings.json", tc.settingsJSON) + } + + var warn bytes.Buffer + a, g := factoryRegistered(t, generic.Options{TargetRoot: tmp, Stderr: &warn}) + + got, err := a.Ingest(adapter.ScopeUser, "") + if tc.wantIngestErr { + if err == nil { + t.Fatalf("Ingest read a malformed file as empty; it must refuse loudly. hooks=%+v", got.Hooks) + } + } else { + if err != nil { + t.Fatalf("Ingest: %v", err) + } + if len(got.Hooks) != 0 { + t.Fatalf("a malformed shape must leave hooks uncaptured, got %+v", got.Hooks) + } + } + refused, rerr := g.RefusedHookEvents(adapter.ScopeUser, "") + if rerr != nil && !tc.wantIngestErr { + t.Fatalf("RefusedHookEvents: %v", rerr) + } + if len(refused) != 0 { + t.Fatalf("a malformed shape is structural and must never be refused (refusal retires shared canonical config): %v", refused) + } + + var hooksBefore, settingsBefore []byte + if tc.hooksJSON != "" { + hooksBefore = []byte(tc.hooksJSON) + } + if tc.settingsJSON != "" { + settingsBefore = []byte(tc.settingsJSON) + } + aerr := renderApplyFactory(t, a, source.Canonical{Hooks: []source.Hook{ + {Event: untrusted.Wrap("Stop"), Type: "command", Command: "echo canonical-stop"}, + }}) + if tc.wantApplyErr { + if aerr == nil { + t.Fatal("apply must refuse a hooks.json it cannot parse rather than replace it") + } + after, err := os.ReadFile(hooksPath) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(after, hooksBefore) { + t.Fatalf("a refused apply must leave hooks.json byte-identical; got:\n%s", after) + } + } else { + if aerr != nil { + t.Fatalf("apply: %v", aerr) + } + if cmds := eventCommands(t, readHooksJSON(t, hooksPath), "Stop"); len(cmds) != 1 || cmds[0] != "echo canonical-stop" { + t.Fatalf("Stop = %v, want the canonical handler", cmds) + } + } + if settingsBefore != nil { + after, err := os.ReadFile(settingsPath) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(after, settingsBefore) { + t.Fatal("settings.json must never be edited") + } + } + if tc.wantWarn != "" && !strings.Contains(warn.String(), tc.wantWarn) { + t.Fatalf("want a warning containing %q, got:\n%s", tc.wantWarn, warn.String()) + } + }) + } +} + +// TestFactoryHooks_OrphanCleanupKeepsForeignKeys drives the real render +// pipeline (Plan, Apply, PruneStaleState, RecordOpsState) against the real +// Factory adapter. A hook removed from the canonical source must disappear +// from hooks.json — partially, while another event is still rendered, and +// entirely, through the synthesized cleanup op — while every key agentsync +// never wrote (hooksDisabled, showHookOutput, a user's SessionEnd) survives. +func TestFactoryHooks_OrphanCleanupKeepsForeignKeys(t *testing.T) { + testenv.RequireContainer(t) + tmp := t.TempDir() + home := filepath.Join(tmp, ".agentsync") + if err := os.MkdirAll(home, 0o755); err != nil { + t.Fatal(err) + } + dir := filepath.Join(tmp, ".factory") + hooksPath := writeFactoryFile(t, dir, "hooks.json", `{ + "hooksDisabled": false, + "showHookOutput": true, + "SessionEnd": [ { "hooks": [ { "type": "command", "command": "echo user-end" } ] } ] + }`) + + reg := adapter.NewRegistry() + if err := reg.Register(generic.Register(factorySpec(t), generic.Options{TargetRoot: tmp})); err != nil { + t.Fatal(err) + } + st := state.New() + apply := func(hooks ...source.Hook) map[string]any { + t.Helper() + plan, err := render.Plan(secrets.ForRender(source.Canonical{Hooks: hooks}), reg, []string{"factory"}, adapter.ScopeUser, "", st, tmp) + if err != nil { + t.Fatalf("Plan: %v", err) + } + if _, _, _, err := render.Apply(plan, reg, st, home, tmp, adapter.ScopeUser, ""); err != nil { + t.Fatalf("Apply: %v", err) + } + ops := plan.PerAgent["factory"].Ops + render.PruneStaleState(st, tmp, "factory", adapter.ScopeUser, "", ops, render.NewSharedDests(plan, tmp)) + if err := render.RecordOpsState(st, tmp, "factory", adapter.ScopeUser, "", ops); err != nil { + t.Fatalf("RecordOpsState: %v", err) + } + return readHooksJSON(t, hooksPath) + } + assertForeignKept := func(stage string, doc map[string]any) { + t.Helper() + if doc["hooksDisabled"] != false || doc["showHookOutput"] != true { + t.Fatalf("%s: foreign top-level keys not preserved: %#v", stage, doc) + } + if got := eventCommands(t, doc, "SessionEnd"); len(got) != 1 || got[0] != "echo user-end" { + t.Fatalf("%s: the user's SessionEnd = %v; agentsync never owned it", stage, got) + } + } + pre := source.Hook{Event: untrusted.Wrap("PreToolUse"), Matcher: "Execute", Type: "command", Command: "echo pre"} + stop := source.Hook{Event: untrusted.Wrap("Stop"), Type: "command", Command: "echo stop"} + + doc := apply(pre, stop) + if got := eventCommands(t, doc, "Stop"); len(got) != 1 { + t.Fatalf("setup: Stop = %v", got) + } + assertForeignKept("initial apply", doc) + + doc = apply(pre) + if _, ok := doc["Stop"]; ok { + t.Fatalf("Stop was removed from canonical but is still in hooks.json: %#v", doc["Stop"]) + } + if got := eventCommands(t, doc, "PreToolUse"); len(got) != 1 || got[0] != "echo pre" { + t.Fatalf("PreToolUse = %v; it is still rendered", got) + } + assertForeignKept("after removing Stop", doc) + + doc = apply() + if _, ok := doc["PreToolUse"]; ok { + t.Fatalf("every canonical hook is gone, so the cleanup op must remove PreToolUse: %#v", doc["PreToolUse"]) + } + assertForeignKept("after removing every hook", doc) +} + +// factoryResolver is a fixed secret backend for the round trip below. +type factoryResolver map[string]string + +func (r factoryResolver) Resolve(key string) (string, error) { + if v, ok := r[key]; ok { + return v, nil + } + return "", fmt.Errorf("unknown secret %q", key) +} + +// TestFactoryHooks_SecretCommandRoundTrip pins the secret boundary for Factory +// hooks end to end. A hook command is secret-bearing (walkSecretFields), so +// apply writes the RESOLVED value — Droid has to run it — and the trip back +// through Ingest must re-reference it to ${secret:…} with nothing left for the +// leak backstop to refuse. A regression here persists a live credential into +// ~/.agentsync, often a committed dotfiles repo. +func TestFactoryHooks_SecretCommandRoundTrip(t *testing.T) { + testenv.RequireContainer(t) + tmp := t.TempDir() + const live = "fct_LIVE_7f3a9c21d4" + const templated = "deploy --token ${secret:factory.token}" + sec := factoryResolver{"factory.token": live} + env := factoryResolver{} + + canonical := source.Canonical{Hooks: []source.Hook{ + {Event: untrusted.Wrap("SessionStart"), Type: "command", Command: templated, Timeout: 5}, + }} + resolved, err := secrets.SubstituteCanonical(canonical, sec, env) + if err != nil { + t.Fatalf("SubstituteCanonical: %v", err) + } + a := generic.New(factorySpec(t), generic.Options{TargetRoot: tmp}) + ops, _, err := a.Render(resolved, adapter.ScopeUser, "") + if err != nil { + t.Fatal(err) + } + if err := a.Apply(ops, adapter.PassThroughWriter{}); err != nil { + t.Fatal(err) + } + + doc := readHooksJSON(t, filepath.Join(tmp, ".factory", "hooks.json")) + if got := eventCommands(t, doc, "SessionStart"); len(got) != 1 || got[0] != "deploy --token "+live { + t.Fatalf("hooks.json SessionStart = %v; Droid must receive the resolved command", got) + } + + ingested, err := a.Ingest(adapter.ScopeUser, "") + if err != nil { + t.Fatal(err) + } + if len(ingested.Hooks) != 1 || ingested.Hooks[0].Command != "deploy --token "+live { + t.Fatalf("ingested hooks = %+v", ingested.Hooks) + } + secrets.ReReferenceCanonical(&ingested, &canonical, sec, env) + if got := ingested.Hooks[0].Command; got != templated { + t.Fatalf("re-referenced command = %q, want %q (a live secret would be written back)", got, templated) + } + if leaks := secrets.ResidualSecretCleartext(&ingested, &canonical, sec, env); len(leaks) != 0 { + t.Fatalf("leak backstop would refuse this write-back: %v", leaks) + } +} diff --git a/internal/adapter/generic/factory_hooks_test.go b/internal/adapter/generic/factory_hooks_test.go new file mode 100644 index 00000000..d7ab050b --- /dev/null +++ b/internal/adapter/generic/factory_hooks_test.go @@ -0,0 +1,418 @@ +package generic_test + +import ( + "bytes" + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/spxrogers/agentsync/internal/adapter" + "github.com/spxrogers/agentsync/internal/adapter/generic" + "github.com/spxrogers/agentsync/internal/render" + "github.com/spxrogers/agentsync/internal/secrets" + "github.com/spxrogers/agentsync/internal/source" + "github.com/spxrogers/agentsync/internal/state" + "github.com/spxrogers/agentsync/internal/testenv" + "github.com/spxrogers/agentsync/internal/untrusted" +) + +func factoryRegistered(t *testing.T, opts generic.Options) (adapter.Adapter, adapter.HookIngestGuard) { + t.Helper() + a := generic.Register(factorySpec(t), opts) + g, ok := a.(adapter.HookIngestGuard) + if !ok { + t.Fatal("factory registry value must implement HookIngestGuard") + } + return a, g +} + +func TestRegister_OnlyFactoryImplementsHookGuard(t *testing.T) { + for _, spec := range generic.Specs() { + a := generic.Register(spec, generic.Options{}) + _, ok := a.(adapter.HookIngestGuard) + if spec.Name == "factory" && !ok { + t.Fatal("factory must implement HookIngestGuard") + } + if spec.Name != "factory" && ok { + t.Fatalf("%s implements HookIngestGuard; only a spec that declares hooks may", spec.Name) + } + } +} + +func factorySpec(t *testing.T) generic.Spec { + t.Helper() + for _, s := range generic.Specs() { + if s.Name == "factory" { + return s + } + } + t.Fatal("factory spec missing") + return generic.Spec{} +} + +func TestRefusedHookEvents_RequiresProjectRoot(t *testing.T) { + _, g := factoryRegistered(t, generic.Options{}) + if _, err := g.RefusedHookEvents(adapter.ScopeProject, ""); err == nil { + t.Fatal("project scope with an empty root must be rejected") + } +} + +func TestApply_FactoryHooks_SeedsSettingsWhenHooksJSONAbsent(t *testing.T) { + testenv.RequireContainer(t) + tmp := t.TempDir() + dir := filepath.Join(tmp, ".factory") + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + settings := `{ + "hooksDisabled": false, + "hooks": { + "PreToolUse": [ { "matcher": "Execute", "commandRegex": "^git ", "hooks": [ { "type": "command", "command": "echo audit" } ] } ], + "Stop": [ { "matcher": "", "hooks": [ { "type": "command", "command": "echo stop" } ] } ] + } + }` + if err := os.WriteFile(filepath.Join(dir, "settings.json"), []byte(settings), 0o644); err != nil { + t.Fatal(err) + } + a := generic.New(factorySpec(t), generic.Options{TargetRoot: tmp}) + c := source.Canonical{Hooks: []source.Hook{ + {Event: untrusted.Wrap("SessionStart"), Type: "command", Command: "echo session"}, + }} + ops, _, err := a.Render(secrets.ForRender(c), adapter.ScopeUser, "") + if err != nil { + t.Fatal(err) + } + var hooksOp *adapter.FileOp + for i := range ops { + if strings.HasSuffix(ops[i].Path, "hooks.json") { + hooksOp = &ops[i] + } + } + if hooksOp == nil { + t.Fatal("no hooks.json op") + } + hooksPath := filepath.Join(dir, "hooks.json") + for i := 0; i < 2; i++ { + if err := a.Apply(ops, adapter.PassThroughWriter{}); err != nil { + t.Fatal(err) + } + doc := readHooksJSON(t, hooksPath) + if got := eventCommands(t, doc, "PreToolUse"); len(got) != 1 || got[0] != "echo audit" { + t.Fatalf("apply %d: seeded PreToolUse = %v, want [echo audit]", i+1, got) + } + if got := firstDef(t, doc, "PreToolUse")["commandRegex"]; got != "^git " { + t.Fatalf("apply %d: seeded commandRegex = %#v; the field agentsync cannot model must survive verbatim", i+1, got) + } + if got := eventCommands(t, doc, "Stop"); len(got) != 1 || got[0] != "echo stop" { + t.Fatalf("apply %d: seeded Stop = %v, want [echo stop]", i+1, got) + } + if got := eventCommands(t, doc, "SessionStart"); len(got) != 1 || got[0] != "echo session" { + t.Fatalf("apply %d: rendered SessionStart = %v, want [echo session]", i+1, got) + } + } + + // The seed is the merge BASE, so only the rendered event may be recorded as + // owned. A seeded pointer recorded as owned would be orphan-deleted by the + // first apply that stops rendering it, destroying a hook the user wrote. + st := state.New() + if err := render.RecordOpsState(st, tmp, "factory", adapter.ScopeUser, "", ops); err != nil { + t.Fatal(err) + } + var owned []string + for key := range st.Keys { + if strings.HasSuffix(key.Path, "hooks.json") { + owned = append(owned, key.Pointer) + } + } + if len(owned) == 0 { + t.Fatal("no hooks.json pointer recorded; the ownership assertion below would be vacuous") + } + for _, ptr := range owned { + if ptr != "/SessionStart" { + t.Fatalf("seeded pointer %q recorded as owned (all owned: %v); only /SessionStart is rendered", ptr, owned) + } + } +} + +func TestRender_Factory_UserMemory(t *testing.T) { + tmp := t.TempDir() + a := generic.New(factorySpec(t), generic.Options{TargetRoot: tmp}) + ops, skips, err := a.Render(secrets.ForRender(source.Canonical{Memory: source.Memory{Body: "policy\n"}}), adapter.ScopeUser, "") + if err != nil { + t.Fatal(err) + } + op := findOp(ops, ".factory/AGENTS.md") + if op == nil || op.Path != filepath.Join(tmp, ".factory", "AGENTS.md") { + t.Fatalf("factory user memory op missing or misplaced: %+v", ops) + } + if source.StripManagedBanner(string(op.Content)) != "policy\n" { + t.Fatalf("factory user memory content = %q", op.Content) + } + for _, s := range skips { + if s.Component == "memory" { + t.Fatalf("factory user memory must not be skipped: %+v", s) + } + } +} + +func TestRender_FactoryHooks_SecondsAndForeignKeys(t *testing.T) { + testenv.RequireContainer(t) + tmp := t.TempDir() + hooksPath := filepath.Join(tmp, ".factory", "hooks.json") + if err := os.MkdirAll(filepath.Dir(hooksPath), 0o755); err != nil { + t.Fatal(err) + } + existing := []byte("{\n \"hooksDisabled\": false,\n \"showHookOutput\": false\n}\n") + if err := os.WriteFile(hooksPath, existing, 0o644); err != nil { + t.Fatal(err) + } + a := generic.New(factorySpec(t), generic.Options{TargetRoot: tmp}) + c := source.Canonical{Hooks: []source.Hook{ + {Event: untrusted.Wrap("PreToolUse"), Matcher: "Execute", Type: "command", Command: "echo hi", Timeout: 30}, + {Event: untrusted.Wrap("PostCompact"), Type: "command", Command: "echo no"}, + }} + ops, skips, err := a.Render(secrets.ForRender(c), adapter.ScopeUser, "") + if err != nil { + t.Fatal(err) + } + var sawPostCompact bool + for _, s := range skips { + if s.Component == "hook" && s.Name == "PostCompact" { + sawPostCompact = true + } + } + if !sawPostCompact { + t.Fatalf("PostCompact must be skipped, not written; skips=%v", skips) + } + if err := a.Apply(ops, adapter.PassThroughWriter{}); err != nil { + t.Fatal(err) + } + raw, err := os.ReadFile(hooksPath) + if err != nil { + t.Fatal(err) + } + var doc map[string]any + if err := json.Unmarshal(raw, &doc); err != nil { + t.Fatalf("parse hooks.json: %v\n%s", err, raw) + } + if doc["hooksDisabled"] != false || doc["showHookOutput"] != false { + t.Fatalf("foreign keys not preserved: %#v", doc) + } + if _, ok := doc["PostCompact"]; ok { + t.Fatal("PostCompact was written; Factory has no such event") + } + groups, _ := doc["PreToolUse"].([]any) + if len(groups) != 1 { + t.Fatalf("PreToolUse = %#v", doc["PreToolUse"]) + } + group := groups[0].(map[string]any) + handlers := group["hooks"].([]any) + handler := handlers[0].(map[string]any) + if handler["timeout"] != float64(30) { + t.Fatalf("timeout = %#v; Factory counts seconds, want 30", handler["timeout"]) + } + if handler["command"] != "echo hi" { + t.Fatalf("command = %#v", handler["command"]) + } +} + +func TestIngest_FactoryHooks_FromSettingsFallback(t *testing.T) { + testenv.RequireContainer(t) + tmp := t.TempDir() + settings := filepath.Join(tmp, ".factory", "settings.json") + if err := os.MkdirAll(filepath.Dir(settings), 0o755); err != nil { + t.Fatal(err) + } + body := `{ + "hooks": { + "SessionStart": [ { "matcher": "", "hooks": [ { "type": "command", "command": "bd prime", "timeout": 3 } ] } ] + } + }` + if err := os.WriteFile(settings, []byte(body), 0o644); err != nil { + t.Fatal(err) + } + a := generic.New(factorySpec(t), generic.Options{TargetRoot: tmp}) + got, err := a.Ingest(adapter.ScopeUser, "") + if err != nil { + t.Fatal(err) + } + if len(got.Hooks) != 1 || got.Hooks[0].Command != "bd prime" || got.Hooks[0].Timeout != 3 { + t.Fatalf("settings.json fallback = %+v", got.Hooks) + } + if got.Hooks[0].Event.Unverified() != "SessionStart" { + t.Fatalf("event = %q", got.Hooks[0].Event.Unverified()) + } +} + +func TestIngest_FactoryHooks_CommandRegexRefusesEvent(t *testing.T) { + testenv.RequireContainer(t) + tmp := t.TempDir() + hooksPath := filepath.Join(tmp, ".factory", "hooks.json") + if err := os.MkdirAll(filepath.Dir(hooksPath), 0o755); err != nil { + t.Fatal(err) + } + body := `{ + "PreToolUse": [ { "matcher": "Execute", "commandRegex": "^git ", "hooks": [ { "type": "command", "command": "echo hi" } ] } ] + }` + if err := os.WriteFile(hooksPath, []byte(body), 0o644); err != nil { + t.Fatal(err) + } + var warn bytes.Buffer + a, g := factoryRegistered(t, generic.Options{TargetRoot: tmp, Stderr: &warn}) + got, err := a.Ingest(adapter.ScopeUser, "") + if err != nil { + t.Fatal(err) + } + if len(got.Hooks) != 0 { + t.Fatalf("commandRegex must leave the event uncaptured, got %+v", got.Hooks) + } + refused, err := g.RefusedHookEvents(adapter.ScopeUser, "") + if err != nil { + t.Fatal(err) + } + if len(refused) != 1 || refused[0] != "PreToolUse" { + t.Fatalf("semantic refusal = %v", refused) + } + if !strings.Contains(warn.String(), "commandRegex") { + t.Fatalf("warning = %q", warn.String()) + } +} + +// TestRefusedHookEvents_PostCompactIsNotRetired pins the shared-file rule. +// PostCompact is not a Droid event, so a native PostCompact array must not be +// refused: import retires every refused event's hooks/.toml for every +// agent, and Claude, Codex, and Grok still use that file. +func TestRefusedHookEvents_PostCompactIsNotRetired(t *testing.T) { + testenv.RequireContainer(t) + tmp := t.TempDir() + hooksPath := filepath.Join(tmp, ".factory", "hooks.json") + if err := os.MkdirAll(filepath.Dir(hooksPath), 0o755); err != nil { + t.Fatal(err) + } + body := `{ + "PostCompact": [ { "hooks": [ { "type": "command", "command": "echo after" } ] } ] + }` + if err := os.WriteFile(hooksPath, []byte(body), 0o644); err != nil { + t.Fatal(err) + } + a, g := factoryRegistered(t, generic.Options{TargetRoot: tmp}) + got, err := a.Ingest(adapter.ScopeUser, "") + if err != nil { + t.Fatal(err) + } + if len(got.Hooks) != 0 { + t.Fatalf("PostCompact must not be captured, got %+v", got.Hooks) + } + refused, err := g.RefusedHookEvents(adapter.ScopeUser, "") + if err != nil { + t.Fatal(err) + } + for _, event := range refused { + if event == "PostCompact" { + t.Fatal("refusing PostCompact would delete the shared canonical hook file") + } + } +} + +// TestIngest_FactoryHooks_HooksJSONWinsOverSettings pins that settings.json is +// only the fallback for an absent hooks.json. Once hooks.json exists, Droid +// ignores settings.json hooks, and so does import. +func TestIngest_FactoryHooks_HooksJSONWinsOverSettings(t *testing.T) { + testenv.RequireContainer(t) + tmp := t.TempDir() + dir := filepath.Join(tmp, ".factory") + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + hooksBody := `{ + "SessionStart": [ { "hooks": [ { "type": "command", "command": "from-hooks-json", "timeout": 5 } ] } ] + }` + settingsBody := `{ + "hooks": { + "SessionStart": [ { "hooks": [ { "type": "command", "command": "from-settings", "timeout": 9 } ] } ] + } + }` + if err := os.WriteFile(filepath.Join(dir, "hooks.json"), []byte(hooksBody), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "settings.json"), []byte(settingsBody), 0o644); err != nil { + t.Fatal(err) + } + got, err := generic.New(factorySpec(t), generic.Options{TargetRoot: tmp}).Ingest(adapter.ScopeUser, "") + if err != nil { + t.Fatal(err) + } + if len(got.Hooks) != 1 || got.Hooks[0].Command != "from-hooks-json" || got.Hooks[0].Timeout != 5 { + t.Fatalf("hooks.json must win over settings.json, got %+v", got.Hooks) + } +} + +// TestRefusedHookEvents_ExplicitZeroTimeout pins that timeout 0 is a semantic +// refusal. Timeout == 0 means "no timeout key", so capturing 0 and re-rendering +// it would drop the key and silently take Droid's 60s default. +func TestRefusedHookEvents_ExplicitZeroTimeout(t *testing.T) { + testenv.RequireContainer(t) + tmp := t.TempDir() + hooksPath := filepath.Join(tmp, ".factory", "hooks.json") + if err := os.MkdirAll(filepath.Dir(hooksPath), 0o755); err != nil { + t.Fatal(err) + } + body := `{ + "PreToolUse": [ { "matcher": "Execute", "hooks": [ { "type": "command", "command": "echo hi", "timeout": 0 } ] } ] + }` + if err := os.WriteFile(hooksPath, []byte(body), 0o644); err != nil { + t.Fatal(err) + } + a, g := factoryRegistered(t, generic.Options{TargetRoot: tmp}) + got, err := a.Ingest(adapter.ScopeUser, "") + if err != nil { + t.Fatal(err) + } + if len(got.Hooks) != 0 { + t.Fatalf("timeout 0 must leave the event uncaptured, got %+v", got.Hooks) + } + refused, err := g.RefusedHookEvents(adapter.ScopeUser, "") + if err != nil { + t.Fatal(err) + } + if len(refused) != 1 || refused[0] != "PreToolUse" { + t.Fatalf("explicit zero must be a semantic refusal, got %v", refused) + } +} + +func TestRender_FactoryHooks_ProjectScope(t *testing.T) { + testenv.RequireContainer(t) + home := t.TempDir() + project := t.TempDir() + a := generic.New(factorySpec(t), generic.Options{TargetRoot: home}) + c := source.Canonical{Hooks: []source.Hook{ + {Event: untrusted.Wrap("PreToolUse"), Matcher: "Execute", Type: "command", Command: "echo proj", Timeout: 8}, + }} + ops, _, err := a.Render(secrets.ForRender(c), adapter.ScopeProject, project) + if err != nil { + t.Fatal(err) + } + want := filepath.Join(project, ".factory", "hooks.json") + op := findOp(ops, ".factory/hooks.json") + if op == nil || op.Path != want { + t.Fatalf("project hooks path = %+v, want %s", ops, want) + } + if strings.Contains(op.Path, filepath.Join(home, ".factory")) { + t.Fatal("project scope wrote the user hooks file") + } + if err := a.Apply(ops, adapter.PassThroughWriter{}); err != nil { + t.Fatal(err) + } + got, err := a.Ingest(adapter.ScopeProject, project) + if err != nil { + t.Fatal(err) + } + if len(got.Hooks) != 1 || got.Hooks[0].Command != "echo proj" || got.Hooks[0].Timeout != 8 { + t.Fatalf("project ingest = %+v", got.Hooks) + } + if _, err := os.Stat(filepath.Join(home, ".factory", "hooks.json")); !os.IsNotExist(err) { + t.Fatalf("user hooks.json should not exist, stat err=%v", err) + } +} diff --git a/internal/adapter/generic/generic.go b/internal/adapter/generic/generic.go index 703110b4..6c4f515a 100644 --- a/internal/adapter/generic/generic.go +++ b/internal/adapter/generic/generic.go @@ -8,11 +8,15 @@ // The DEEP adapters (claude, codex, cursor, gemini, opencode, continuedev, // windsurf, roo, cline) are NOT generic — they have richer, agent-specific // component support (subagents, commands, hooks, …) and bidirectional nuances -// that don't fit a table. The generic tier deliberately covers memory, MCP, and -// Agent Skills (the open SKILL.md spec, where the agent natively scans a skills -// directory), and reports every other component as a skip, so its coverage is -// never overstated. Skills fit the table because their on-disk format is uniform -// across agents — only the scanned directory varies, with no dialect to model. +// that don't fit a table. The generic tier covers memory, MCP, and Agent Skills +// (the open SKILL.md spec, where the agent natively scans a skills directory), +// and reports every other component as a skip, so its coverage is never +// overstated. Factory Droid is the one exception: its spec declares a hooks.json, +// and Register wraps that adapter so only it implements HookIngestGuard. The +// hook dialect (root event map, Droid's event list, settings.json fallback) is +// Factory's. Setting Hooks on another spec would inherit that dialect, which is +// not a generic knob. Skills fit the table because their on-disk format is +// uniform across agents — only the scanned directory varies, with no dialect to model. // Breadth-tier agents still flow through agentsync's normal apply/import // pipeline, so they inherit drift detection, secret resolution, and capture — // which a one-way "rules dump" (ruler/rulesync) does not provide. @@ -24,6 +28,7 @@ package generic import ( + "io" "os" "os/exec" "path/filepath" @@ -115,12 +120,18 @@ type Spec struct { Memory FileTarget // rules/instructions FILE path (plain markdown) MCP MCPTarget // mcpServers JSON Skills FileTarget // Agent-Skills DIRECTORY root (holds /SKILL.md subtrees); empty = unsupported + // Hooks is a hooks.json FILE whose root is the event map (Factory Droid). + // Empty means the agent has no command-hook file agentsync manages. + // The legacy settings.json "hooks" key is an import fallback only. + Hooks FileTarget } // Options configure a generic adapter instance. type Options struct { TargetRoot string LookPath func(file string) (string, error) + // Stderr receives ingest warnings. Nil means os.Stderr. + Stderr io.Writer } // Adapter implements adapter.Adapter for one Spec. @@ -134,6 +145,16 @@ func New(spec Spec, opts Options) *Adapter { return &Adapter{spec: spec, opts: o func (a *Adapter) Name() string { return a.spec.Name } +// SetStderr replaces the ingest warning sink. Nil restores os.Stderr. +func (a *Adapter) SetStderr(w io.Writer) { a.opts.Stderr = w } + +func (a *Adapter) warn() io.Writer { + if a.opts.Stderr != nil { + return a.opts.Stderr + } + return os.Stderr +} + // KeyMergeStrategy is merge-jsonc-keys when the spec has an MCP file (the only // key-merge surface); otherwise "" (memory is a whole-file write). The breadth // tier always uses the JSONC-tolerant merge so a commented settings file (Zed, @@ -210,6 +231,21 @@ func (a *Adapter) skillsPath(scope adapter.Scope, project string) string { return filepath.Join(a.opts.TargetRoot, a.spec.Skills.User) } +// hooksPath resolves the absolute hooks.json for the scope, or "" when the +// spec does not manage hooks there. +func (a *Adapter) hooksPath(scope adapter.Scope, project string) string { + if scope == adapter.ScopeProject { + if a.spec.Hooks.Project == "" { + return "" + } + return filepath.Join(project, a.spec.Hooks.Project) + } + if a.spec.Hooks.User == "" { + return "" + } + return filepath.Join(a.opts.TargetRoot, a.spec.Hooks.User) +} + // agentTargeted reports whether the agents allowlist includes this agent. func agentTargeted(name string, agents []string) bool { if len(agents) == 0 { diff --git a/internal/adapter/generic/generic_test.go b/internal/adapter/generic/generic_test.go index 20b60c77..6ffd9bef 100644 --- a/internal/adapter/generic/generic_test.go +++ b/internal/adapter/generic/generic_test.go @@ -805,7 +805,7 @@ func TestSkills_SpecPathsPinned(t *testing.T) { "qwen": {".qwen/skills", ".qwen/skills"}, "junie": {".junie/skills", ".junie/skills"}, "kiro": {".kiro/skills", ".kiro/skills"}, - "factory": {".factory/skills", ".factory/skills"}, + "factory": {".agents/skills", ".agents/skills"}, "copilot": {".copilot/skills", ".github/skills"}, // deliberately skills-less (verified: no native SKILL.md scan) "jules": {"", ""}, diff --git a/internal/adapter/generic/homedir_test.go b/internal/adapter/generic/homedir_test.go index 0916d9d0..9f731db3 100644 --- a/internal/adapter/generic/homedir_test.go +++ b/internal/adapter/generic/homedir_test.go @@ -23,7 +23,7 @@ func TestVersionRoots_AllSpecs(t *testing.T) { "kiro": {".kiro"}, "kilocode": {".kilo"}, "amazonq": {".aws/amazonq"}, - "factory": {".factory"}, + "factory": {".factory", ".agents/skills"}, "pi": {".pi/agent", ".agents/skills"}, "zed": {".config/zed", ".agents/skills"}, "firebase": {}, diff --git a/internal/adapter/generic/hooks.go b/internal/adapter/generic/hooks.go new file mode 100644 index 00000000..dc04d765 --- /dev/null +++ b/internal/adapter/generic/hooks.go @@ -0,0 +1,308 @@ +package generic + +import ( + "encoding/json" + "fmt" + "io" + "path/filepath" + "sort" + + "github.com/spxrogers/agentsync/internal/adapter" + "github.com/spxrogers/agentsync/internal/jsonkeys" + "github.com/spxrogers/agentsync/internal/source" + "github.com/spxrogers/agentsync/internal/untrusted" +) + +// factoryHookEvents is the event set Factory Droid documents for hooks.json +// (docs.factory.ai/reference/hooks-reference). Names match the canonical +// events, except PostCompact, which Droid does not have. An unsupported +// canonical event is skipped, never refused: refusing it would delete the +// shared hooks/.toml other agents still use. +var factoryHookEvents = map[string]bool{ + "PreToolUse": true, + "PostToolUse": true, + "UserPromptSubmit": true, + "Notification": true, + "Stop": true, + "SubagentStop": true, + "PreCompact": true, + "SessionStart": true, + "SessionEnd": true, +} + +var ( + factoryHookDefModeled = map[string]bool{"matcher": true, "hooks": true} + factoryHookEntryModeled = map[string]bool{"type": true, "command": true, "timeout": true} +) + +// renderHooks writes hooks.json. The file's root IS the event map (not a +// "hooks" wrapper). agentsync owns each event array it renders; foreign +// top-level keys (hooksDisabled, showHookOutput) stay via merge-jsonc-keys. +// Timeout is seconds, the same unit Droid documents. +func (a *Adapter) renderHooks(c source.Canonical, scope adapter.Scope, project string) ([]adapter.FileOp, []adapter.Skip, error) { + path := a.hooksPath(scope, project) + if path == "" || len(c.Hooks) == 0 { + return nil, nil, nil + } + byEvent := map[string][]map[string]any{} + var skips []adapter.Skip + for _, h := range c.Hooks { + event := h.Event.Unverified() + if !factoryHookEvents[event] { + skips = append(skips, adapter.Skip{ + Component: "hook", Name: event, + Reason: fmt.Sprintf("Factory Droid has no %q hook event", event), + Kind: adapter.SkipDropped, + }) + continue + } + if h.Type != "" && h.Type != "command" { + skips = append(skips, adapter.Skip{ + Component: "hook", Name: event, + Reason: fmt.Sprintf("agentsync models only command hooks; type %q is not projected", h.Type), + Kind: adapter.SkipDropped, + }) + continue + } + handler := map[string]any{"type": "command", "command": h.Command} + adapter.SetHookTimeout(handler, h.Timeout) + entry := map[string]any{ + "matcher": h.Matcher, + "hooks": []map[string]any{handler}, + } + byEvent[event] = append(byEvent[event], entry) + } + if len(byEvent) == 0 { + return nil, skips, nil + } + owned := make([]string, 0, len(byEvent)) + for event := range byEvent { + owned = append(owned, "/"+event) + } + sort.Strings(owned) + body, err := json.MarshalIndent(byEvent, "", " ") + if err != nil { + return nil, nil, fmt.Errorf("marshal hooks: %w", err) + } + return []adapter.FileOp{{ + Action: adapter.ActionWrite, + Path: path, + Content: append(body, '\n'), + Mode: 0o644, + SourceID: "hooks/* (multiple)", + MergeStrategy: "merge-jsonc-keys", + OwnedKeys: owned, + }}, skips, nil +} + +// ingestHooks reads hooks.json. When that file is absent, Droid falls back to +// the hooks key of the matching settings.json; import follows that fallback +// so those hooks are not invisible. Apply always writes hooks.json, which is +// the file Droid prefers once it exists. +func (a *Adapter) ingestHooks(scope adapter.Scope, project string) ([]source.Hook, error) { + hooks, _, err := a.readFactoryHooks(scope, project, a.warn()) + return hooks, err +} + +// hookedAdapter is what the registry stores for a spec that declares hooks. +// The method is on this type, not *Adapter, so the other breadth agents do +// not implement HookIngestGuard. Import's type assert would otherwise treat +// all 22 as hook guards, and the registry contract test would try to enrich +// a hooks file none of them write. +type hookedAdapter struct{ *Adapter } + +// RefusedHookEvents implements adapter.HookIngestGuard. Only Factory-documented +// events are refused; an unsupported event must not retire the shared +// canonical file. +func (a *hookedAdapter) RefusedHookEvents(scope adapter.Scope, project string) ([]string, error) { + if err := adapter.RequireProjectRoot(scope, project); err != nil { + return nil, err + } + _, refused, err := a.readFactoryHooks(scope, project, io.Discard) + return refused, err +} + +// Register returns the value the CLI registry stores. Specs that declare a +// hooks file are wrapped so only they implement HookIngestGuard. +func Register(spec Spec, opts Options) adapter.Adapter { + a := New(spec, opts) + if spec.Hooks.User == "" && spec.Hooks.Project == "" { + return a + } + return &hookedAdapter{Adapter: a} +} + +func (a *Adapter) readFactoryHooks(scope adapter.Scope, project string, warn io.Writer) ([]source.Hook, []string, error) { + path := a.hooksPath(scope, project) + if path == "" { + return nil, nil, nil + } + data, present, err := adapter.ReadFileOptional(path) + if err != nil { + return nil, nil, fmt.Errorf("read %s: %w", path, err) + } + var doc map[string]any + if present { + doc, err = jsonkeys.DecodeJSONC(data) + if err != nil { + return nil, nil, fmt.Errorf("parse %s: %w", path, err) + } + } else if doc, err = a.settingsHooks(scope, project, warn); err != nil { + return nil, nil, err + } + if doc == nil { + return nil, nil, nil + } + hooks, refused := parseFactoryHooks(doc, warn) + return hooks, refused, nil +} + +// settingsHooks reads the hooks object from settings.json beside hooks.json. +// A missing file or a missing hooks key is "no hooks", not an error. +func (a *Adapter) settingsHooks(scope adapter.Scope, project string, warn io.Writer) (map[string]any, error) { + hooksFile := a.hooksPath(scope, project) + if hooksFile == "" { + return nil, nil + } + settings := filepath.Join(filepath.Dir(hooksFile), "settings.json") + data, present, err := adapter.ReadFileOptional(settings) + if err != nil { + return nil, fmt.Errorf("read %s: %w", settings, err) + } + if !present { + return nil, nil + } + doc, err := jsonkeys.DecodeJSONC(data) + if err != nil { + return nil, fmt.Errorf("parse %s: %w", settings, err) + } + raw, ok := doc["hooks"] + if !ok { + return nil, nil + } + hooks, ok := raw.(map[string]any) + if !ok { + fmt.Fprintf(warn, "warning: %s hooks value is not an object; hooks not captured\n", settings) + return nil, nil + } + return hooks, nil +} + +func parseFactoryHooks(doc map[string]any, warn io.Writer) (out []source.Hook, refused []string) { + events := make([]string, 0, len(doc)) + for event := range doc { + events = append(events, event) + } + sort.Strings(events) + for _, event := range events { + raw := doc[event] + if !factoryHookEvents[event] { + if _, isArray := raw.([]any); isArray { + fmt.Fprintf(warn, "warning: hook event %q is not a Factory Droid event; event not captured\n", event) + } + continue + } + entries, ok := raw.([]any) + if !ok { + fmt.Fprintf(warn, "warning: hook event %q value is not an array; event not captured\n", event) + continue + } + var captured []source.Hook + representable := true + structural := false + for _, item := range entries { + def, ok := item.(map[string]any) + if !ok { + fmt.Fprintf(warn, "warning: hook event %q has an entry that is not an object; event not captured\n", event) + structural = true + representable = false + break + } + if extra := unmodeledKeys(def, factoryHookDefModeled); extra != "" { + fmt.Fprintf(warn, "warning: hook event %q has %s; event not captured\n", event, extra) + representable = false + break + } + handlers, ok := def["hooks"].([]any) + if !ok { + fmt.Fprintf(warn, "warning: hook event %q entry has no hooks array; event not captured\n", event) + structural = true + representable = false + break + } + matcher, _ := def["matcher"].(string) + if _, present := def["matcher"]; present { + if _, isStr := def["matcher"].(string); !isStr { + fmt.Fprintf(warn, "warning: hook event %q matcher is not a string; event not captured\n", event) + structural = true + representable = false + break + } + } + for _, hraw := range handlers { + h, ok := hraw.(map[string]any) + if !ok { + fmt.Fprintf(warn, "warning: hook event %q has a handler that is not an object; event not captured\n", event) + structural = true + representable = false + break + } + if extra := unmodeledKeys(h, factoryHookEntryModeled); extra != "" { + fmt.Fprintf(warn, "warning: hook event %q has a handler with %s; event not captured\n", event, extra) + representable = false + break + } + typ, _ := h["type"].(string) + if typ != "" && typ != "command" { + fmt.Fprintf(warn, "warning: hook event %q has a %q-type handler agentsync cannot represent; event not captured\n", event, typ) + representable = false + break + } + cmd, cmdOK := h["command"].(string) + if !cmdOK || cmd == "" { + fmt.Fprintf(warn, "warning: hook event %q has a handler without a command; event not captured\n", event) + structural = true + representable = false + break + } + timeout, tres := adapter.ParseHookTimeout(h) + if tres != source.HookTimeoutOK { + fmt.Fprintf(warn, "warning: hook event %q has a handler with %s; event not captured\n", event, tres.Reason()) + if tres.Structural() { + structural = true + } + representable = false + break + } + captured = append(captured, source.Hook{ + Event: untrusted.Wrap(event), Matcher: matcher, Type: "command", Command: cmd, Timeout: timeout, + }) + } + if !representable { + break + } + } + if !representable { + if !structural { + refused = append(refused, event) + } + continue + } + out = append(out, captured...) + } + return out, refused +} + +func unmodeledKeys(obj map[string]any, modeled map[string]bool) string { + var extra []string + for k := range obj { + if !modeled[k] { + extra = append(extra, k) + } + } + if len(extra) == 0 { + return "" + } + sort.Strings(extra) + return fmt.Sprintf("unmodeled field %q", extra[0]) +} diff --git a/internal/adapter/generic/ingest.go b/internal/adapter/generic/ingest.go index dcbd31a9..225f4d7a 100644 --- a/internal/adapter/generic/ingest.go +++ b/internal/adapter/generic/ingest.go @@ -13,8 +13,8 @@ import ( ) // Ingest reads this breadth-tier agent's memory, MCP, and skills back into a -// partial canonical. Inverse of Render (memory + MCP + skills; the components the -// tier never projects are simply not read). +// partial canonical, plus hooks when the spec declares a hooks file (Factory). +// Inverse of Render. Components the tier never projects are simply not read. func (a *Adapter) Ingest(scope adapter.Scope, project string) (source.Canonical, error) { if err := adapter.RequireProjectRoot(scope, project); err != nil { return source.Canonical{}, err @@ -64,6 +64,14 @@ func (a *Adapter) Ingest(scope adapter.Scope, project string) (source.Canonical, c.Skills = append(c.Skills, skills...) } + if a.hooksPath(scope, project) != "" { + hooks, err := a.ingestHooks(scope, project) + if err != nil { + return c, err + } + c.Hooks = append(c.Hooks, hooks...) + } + return c, nil } @@ -77,15 +85,12 @@ func (a *Adapter) Ingest(scope adapter.Scope, project string) (source.Canonical, // directory (permission, transient I/O) is RETURNED as an error rather than read // as "no skills", which drift could misclassify as the whole set being cleared. // -// Per-entry deliberate no-diagnostics stance: unlike the deep adapters (which -// thread an a.stderr() warn sink), the generic tier's Ingest has no diagnostics -// channel, so a per-skill SKILL.md that is missing or unparseable is skipped -// silently — exactly as this tier's MCP ingest silently skips a non-map server -// entry. This is acknowledged (not an accidental silent drop): a stray directory -// in a shared skills root must not break import of the rest, and the lossy case -// is a *native* file the tier can't parse, never a canonical component the tier -// fails to project. Threading a warn sink through the breadth-tier Ingest is a -// deferred tier-wide change, not a skills-specific one. +// Per-entry deliberate no-diagnostics stance for SKILLS: a per-skill SKILL.md +// that is missing or unparseable is skipped silently — exactly as this tier's +// MCP ingest silently skips a non-map server entry. This is acknowledged (not +// an accidental silent drop): a stray directory in a shared skills root must +// not break import of the rest. Hook ingest is the exception: it warns through +// Options.Stderr, because a lossy hook event must not be captured. func (a *Adapter) ingestSkills(skillsDir string) ([]source.Skill, error) { entries, present, err := adapter.ReadDirOptional(skillsDir) if err != nil { diff --git a/internal/adapter/generic/render.go b/internal/adapter/generic/render.go index 8c23aa82..fc0aa665 100644 --- a/internal/adapter/generic/render.go +++ b/internal/adapter/generic/render.go @@ -12,13 +12,12 @@ import ( ) // Render projects the canonical model into this breadth-tier agent's memory -// (rules) file, its mcpServers JSON, and its Agent-Skills directory — each when -// the spec declares a target for it. Every other component (subagents, commands, -// hooks, LSP), and skills/MCP for a spec that declares no target, is reported as -// a skip: the generic tier deliberately covers memory + MCP + skills only. A -// component the spec does not support at the requested scope (e.g. user-scope -// memory for a project-only agent) is likewise reported as a skip rather than -// written somewhere wrong. +// (rules) file, its mcpServers JSON, its Agent-Skills directory, and, when the +// spec declares one, its hooks.json. Subagents, commands, and LSP are always +// reported as skips. Hooks are a skip unless the spec declares a hooks file +// (Factory Droid). A component the spec does not support at the requested +// scope (e.g. user-scope memory for a project-only agent) is likewise reported +// as a skip rather than written somewhere wrong. func (a *Adapter) Render(r secrets.Resolved, scope adapter.Scope, project string) ([]adapter.FileOp, []adapter.Skip, error) { if err := adapter.RequireProjectRoot(scope, project); err != nil { return nil, nil, err @@ -70,6 +69,14 @@ func (a *Adapter) Render(r secrets.Resolved, scope adapter.Scope, project string skips = append(skips, skSkips...) } + // Hooks → hooks.json, only when the spec declares one (Factory Droid). + if hOps, hSkips, err := a.renderHooks(renderC, scope, project); err != nil { + return nil, nil, err + } else { + ops = append(ops, hOps...) + skips = append(skips, hSkips...) + } + // Components the breadth tier does not project — reported, never silent. skips = append(skips, a.unsupportedSkips(renderC)...) return ops, skips, nil @@ -184,8 +191,10 @@ func (a *Adapter) unsupportedSkips(c source.Canonical) []adapter.Skip { for _, cmd := range c.Commands { skips = append(skips, adapter.Skip{Component: "command", Name: cmd.Name, Reason: reason("commands"), Kind: adapter.SkipDropped}) } - for _, h := range c.Hooks { - skips = append(skips, adapter.Skip{Component: "hook", Name: h.Event.String(), Reason: reason("hooks"), Kind: adapter.SkipDropped}) + if a.spec.Hooks.User == "" && a.spec.Hooks.Project == "" { + for _, h := range c.Hooks { + skips = append(skips, adapter.Skip{Component: "hook", Name: h.Event.String(), Reason: reason("hooks"), Kind: adapter.SkipDropped}) + } } for _, l := range c.LSPServers { skips = append(skips, adapter.Skip{Component: "lsp", Name: l.ID, Reason: reason("LSP"), Kind: adapter.SkipDropped}) diff --git a/internal/adapter/generic/specs.go b/internal/adapter/generic/specs.go index 15b3dd4a..c99d4adc 100644 --- a/internal/adapter/generic/specs.go +++ b/internal/adapter/generic/specs.go @@ -87,12 +87,24 @@ func Specs() []Spec { Memory: FileTarget{Project: ".amazonq/rules/agentsync.md"}, MCP: MCPTarget{User: ".aws/amazonq/mcp.json", Project: ".amazonq/mcp.json"}, }, - // Factory Droid — AGENTS.md; MCP `.factory/mcp.json` with explicit `type`. + // Factory Droid: AGENTS.md (project + personal ~/.factory/AGENTS.md, per + // docs.factory.ai/cli/configuration/agents-md); MCP `.factory/mcp.json` + // with explicit `type`. Skills use the shared `.agents/skills` dir, which + // Droid scans as a compatibility source next to its own `.factory/skills` + // (docs.factory.ai/cli/configuration/skills). Writing both would register + // every skill in two sources. Cross-source name clashes are overridden, + // not rejected; agentsync still writes only the shared directory so a + // machine that also enables Codex does not get two copies. + // Hooks are ~/.factory/hooks.json and .factory/hooks.json (event map at + // the root, timeout in seconds). docs.factory.ai/reference/hooks-reference. + // The legacy .factory/hooks/hooks.json path still loads in Droid but is + // not managed; the next Droid save migrates it. { Name: "factory", DetectBin: "droid", DetectDir: ".factory", - Memory: FileTarget{Project: "AGENTS.md"}, + Memory: FileTarget{User: ".factory/AGENTS.md", Project: "AGENTS.md"}, MCP: MCPTarget{User: ".factory/mcp.json", Project: ".factory/mcp.json", TransportKey: "type"}, - Skills: FileTarget{User: ".factory/skills", Project: ".factory/skills"}, + Skills: FileTarget{User: ".agents/skills", Project: ".agents/skills"}, + Hooks: FileTarget{User: ".factory/hooks.json", Project: ".factory/hooks.json"}, }, // Pi Coding Agent — AGENTS.md (project + ~/.pi/agent/AGENTS.md); MCP // `~/.pi/agent/mcp.json` (inferred). No documented project MCP file. diff --git a/internal/cli/hookingestguard_guard_test.go b/internal/cli/hookingestguard_guard_test.go index 6977f894..53a65a06 100644 --- a/internal/cli/hookingestguard_guard_test.go +++ b/internal/cli/hookingestguard_guard_test.go @@ -89,7 +89,11 @@ func TestHookIngestGuard_ReportsCanonicalNames(t *testing.T) { } marshal = func(v map[string]any) ([]byte, error) { return toml.Marshal(v) } } - if _, ok := top["hooks"]; !ok { + // Claude/Codex/Gemini/Grok nest handlers under "hooks". Factory's + // hooks.json is the event map itself, so a top-level array is + // the hook document. Other JSON ops (MCP) are objects of objects + // and are left alone. + if _, ok := top["hooks"]; !ok && !hasTopLevelArray(top) { continue } injectUnmodeledField(top) @@ -121,7 +125,7 @@ func TestHookIngestGuard_ReportsCanonicalNames(t *testing.T) { // Vacuity guard: the adapters whose hook ingests refuse semantically // must implement HookIngestGuard BY NAME — losing one silently reopens the // issue #124 second-order clobber for that agent. - for _, agent := range []string{"claude", "gemini", "cursor", "codex", "grok"} { + for _, agent := range []string{"claude", "gemini", "cursor", "codex", "grok", "factory"} { if !guards[agent] { t.Fatalf("agent %s no longer implements adapter.HookIngestGuard — its native "+ "hook enrichments would silently stop triggering import's stale-hook retirement", agent) @@ -129,6 +133,20 @@ func TestHookIngestGuard_ReportsCanonicalNames(t *testing.T) { } } +// hasTopLevelArray reports whether any top-level value is an array, which is +// how Factory's hooks.json is recognised: it IS the event map, so its values +// are handler arrays, where every other hook-capable adapter nests them under a +// "hooks" key. The other JSON ops this walk sees (MCP) are objects of objects, +// so they do not match. +func hasTopLevelArray(top map[string]any) bool { + for _, v := range top { + if _, ok := v.([]any); ok { + return true + } + } + return false +} + // injectUnmodeledField walks any decoded JSON value and adds an unmodeled // "statusMessage" field to every object that carries a "command" key — the handler // objects, wherever an adapter's native hook shape nests them. diff --git a/internal/cli/import.go b/internal/cli/import.go index 4e539614..21c34169 100644 --- a/internal/cli/import.go +++ b/internal/cli/import.go @@ -457,7 +457,12 @@ func retireRefusedHookEvents(io *importIO, agentsyncHome, srcHome string, a adap continue } for _, alias := range aliases { - if key.Pointer == "/hooks/"+alias { + // "/hooks/" is the nested shape (claude, codex, gemini, + // cursor, grok). Factory's hooks.json is the event map itself, so + // its state pointer is "/". Missing that spelling left the + // key owned after retirement, and the next apply's orphan cleanup + // deleted the native event this retirement exists to protect. + if key.Pointer == "/hooks/"+alias || key.Pointer == "/"+alias { delete(st.Keys, key) changed = true } diff --git a/internal/cli/import_test.go b/internal/cli/import_test.go index 9f6533b4..f1d899df 100644 --- a/internal/cli/import_test.go +++ b/internal/cli/import_test.go @@ -2047,3 +2047,47 @@ func TestImport_RetireStateFailureLeavesCanonicalIntact(t *testing.T) { t.Fatalf("state failure must leave the canonical hook file in place for a retry (disown-first ordering): %v\n%s", err, out) } } + +// TestImport_FactoryRootPointerDisown pins that retiring a Factory event +// clears the "/" state pointer, not only "/hooks/". Factory's +// hooks.json is the event map itself. Leaving "/PreToolUse" owned made the +// next apply's orphan cleanup delete the native commandRegex entry. +func TestImport_FactoryRootPointerDisown(t *testing.T) { + tmp, env := importTestEnv(t) + mustRun(t, env, "agent", "add", "factory") + hooks := filepath.Join(tmp, ".factory", "hooks.json") + if err := os.MkdirAll(filepath.Dir(hooks), 0o755); err != nil { + t.Fatal(err) + } + clean := `{"PreToolUse":[{"matcher":"Execute","hooks":[{"type":"command","command":"echo hi"}]}]}` + if err := os.WriteFile(hooks, []byte(clean), 0o644); err != nil { + t.Fatal(err) + } + if out, err := runCLI(t, env, "import", "factory:hook:PreToolUse"); err != nil { + t.Fatalf("import clean hook: %v\n%s", err, out) + } + if out, err := runCLI(t, env, "apply", "--no-git-backup"); err != nil { + t.Fatalf("apply: %v\n%s", err, out) + } + enriched := `{"PreToolUse":[{"matcher":"Execute","commandRegex":"^git ","hooks":[{"type":"command","command":"echo hi"}]}]}` + if err := os.WriteFile(hooks, []byte(enriched), 0o644); err != nil { + t.Fatal(err) + } + out, err := runCLI(t, env, "import", "factory") + if err != nil { + t.Fatalf("re-import: %v\n%s", err, out) + } + if !strings.Contains(out, "retired canonical hooks/PreToolUse.toml") { + t.Fatalf("import did not retire the shared hook:\n%s", out) + } + if out, err := runCLI(t, env, "apply", "--no-git-backup"); err != nil { + t.Fatalf("apply after retirement: %v\n%s", err, out) + } + got, err := os.ReadFile(hooks) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(got), "commandRegex") || !strings.Contains(string(got), "echo hi") { + t.Fatalf("orphan cleanup deleted Factory's native event:\n%s", got) + } +} diff --git a/internal/cli/registry_internal.go b/internal/cli/registry_internal.go index 353d1793..078ef793 100644 --- a/internal/cli/registry_internal.go +++ b/internal/cli/registry_internal.go @@ -54,7 +54,7 @@ var registryFactory = func() *adapter.Registry { mustRegister(grok.New(grok.Options{TargetRoot: home, GrokHome: paths.AgentHomeOverride(env, "GROK_HOME")})) // Breadth tier: one generic adapter per verified Spec (memory + optional MCP). for _, spec := range generic.Specs() { - mustRegister(generic.New(spec, generic.Options{TargetRoot: home})) + mustRegister(generic.Register(spec, generic.Options{TargetRoot: home})) } return r } diff --git a/website/src/content/docs/help/faq.mdx b/website/src/content/docs/help/faq.mdx index 3748a0fb..7f11808c 100644 --- a/website/src/content/docs/help/faq.mdx +++ b/website/src/content/docs/help/faq.mdx @@ -78,8 +78,8 @@ goose, qwen, warp, jules, junie, openhands, amazonq, zed, kilocode, kiro, trae, jetbrains, firebase, antigravity, augmentcode, copilot, copilot-cli, crush, factory, pi, mistral) is supported via one data-driven generic adapter — **memory** for all, **MCP** wherever the agent reads a JSON server-map agentsync can express, -and **Agent Skills** wherever the agent natively scans a `SKILL.md` directory (18 of -the 22), taking agentsync to **32 agents**. See the +**Agent Skills** wherever the agent natively scans a `SKILL.md` directory (18 of +the 22), and command hooks for Factory only, taking agentsync to **32 agents**. See the [capability matrix → Breadth tier](/reference/capability-matrix/#breadth-tier). ## What about Aider? diff --git a/website/src/content/docs/index.mdx b/website/src/content/docs/index.mdx index 09aeac39..1b61defc 100644 --- a/website/src/content/docs/index.mdx +++ b/website/src/content/docs/index.mdx @@ -151,8 +151,9 @@ That `github` MCP server now lives in **both** `~/.claude.json` and | **Grok Build** | ✓ adapter | MCP (◐, TOML), instructions, complete skills, commands (◐), and command hooks (◐) at user and project scopes. [Supported paths and limits](https://github.com/spxrogers/agentsync/blob/main/docs/grok.md). Subagent/LSP projection and plugin import deferred. | Plus a **breadth tier** of 22 more agents via one data-driven generic adapter -(memory for all, MCP where the agent reads a JSON server-map, and Agent Skills -where the agent natively scans a `SKILL.md` directory): amp, goose, qwen, +(memory for all, MCP where the agent reads a JSON server-map, Agent Skills +where the agent natively scans a `SKILL.md` directory, and command hooks for +Factory only): amp, goose, qwen, warp, jules, junie, openhands, amazonq, zed, kilocode, kiro, trae, jetbrains, firebase, antigravity, augmentcode, copilot, copilot-cli, crush, factory, pi, mistral. See the [capability matrix → Breadth tier](/reference/capability-matrix/#breadth-tier).