Skip to content

fix(hooks): correct Factory Droid hooks file shape #2320

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
RaviTharuma wants to merge 1 commit into from
+68 −27
Closed

fix(hooks): correct Factory Droid hooks file shape #2320

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions src/e2e/e2e-hooks.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,10 @@ import {
* (e.g. claudecode uses PascalCase `Stop`), so checking command paths inside
* the serialized hooks block is the most tool-agnostic assertion.
*/
function assertHookCommandsPreserved(parsed: { hooks?: unknown }): void {
expect(parsed.hooks).toBeDefined();
const serialized = JSON.stringify(parsed.hooks);
function assertHookCommandsPreserved(parsed: { hooks?: unknown }, rootIsHooks = false): void {
const hooks = rootIsHooks ? parsed : parsed.hooks;

@dyoshikawa dyoshikawa Jul 20, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With rootIsHooks = true this assertion becomes near-vacuous: expect(hooks).toBeDefined() is trivially true for any parsed object, and stringifying the whole file then grepping for the command substrings passes regardless of the file's structure. That's how the shape change sailed through the e2e matrix green. The e2e check should pin the documented shape (e.g. assert parsed.hooks.SessionStart exists) rather than degrade to a whole-file substring match.

expect(hooks).toBeDefined();
const serialized = JSON.stringify(hooks);
expect(serialized).toContain(".rulesync/hooks/session-start.sh");
expect(serialized).toContain(".rulesync/hooks/audit.sh");
}
Expand Down Expand Up @@ -176,7 +177,7 @@ describe("E2E: hooks", () => {
} else {
// codexcli, factorydroid, goose: event-name casing/mapping
// varies per tool, so verify the configured hook command paths are preserved.
assertHookCommandsPreserved(parsed);
assertHookCommandsPreserved(parsed, target === "factorydroid");
}
}
});
Expand Down Expand Up @@ -700,7 +701,7 @@ describe("E2E: hooks (global mode)", () => {
expect(JSON.stringify(parsed.hooks)).toContain(".rulesync/hooks/session-start.sh");
expect(JSON.stringify(parsed.hooks)).toContain(".rulesync/hooks/audit.sh");
} else {
assertHookCommandsPreserved(JSON.parse(generatedContent));
assertHookCommandsPreserved(JSON.parse(generatedContent), target === "factorydroid");
}
},
);
Expand Down
76 changes: 58 additions & 18 deletions src/features/hooks/factorydroid-hooks.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,46 @@ describe("FactorydroidHooks", () => {
});

describe("fromRulesyncHooks", () => {
it("should emit a dedicated hooks file that preserves an override command", async () => {
const config = {
version: 1,
hooks: {},
factorydroid: {
hooks: {
sessionStart: [{ type: "command", command: ".rulesync/hooks/notify.sh" }],
},
},
};
const rulesyncHooks = new RulesyncHooks({
outputRoot: testDir,
relativeDirPath: RULESYNC_RELATIVE_DIR_PATH,
relativeFilePath: "hooks.json",
fileContent: JSON.stringify(config),
validate: false,
});

const factorydroidHooks = await FactorydroidHooks.fromRulesyncHooks({
outputRoot: testDir,
rulesyncHooks,
validate: false,
global: true,
});

const parsed = JSON.parse(factorydroidHooks.getFileContent());
expect(parsed).toEqual({

@dyoshikawa dyoshikawa Jul 20, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test doesn't reproduce issue #2317: the issue used an absolute command (/tmp/notify.sh), which the prefix logic in tool-hooks-converter.ts mangles into "$FACTORY_PROJECT_DIR"//tmp/notify.sh — a real pre-existing bug this PR doesn't touch. Substituting a relative path here sidesteps the actual repro. Also note forDeletion and the fromFile fallback in factorydroid-hooks.ts still emit {"hooks": {}}, which contradicts the flat shape this PR introduces.

SessionStart: [
{
hooks: [
{
type: "command",
command: '"$FACTORY_PROJECT_DIR"/.rulesync/hooks/notify.sh',
},
],
},
],
});
});

it("should filter shared hooks to Factory Droid-supported events and convert to PascalCase", async () => {
await ensureDir(join(testDir, ".factory"));
await writeFileContent(join(testDir, ".factory", "settings.json"), JSON.stringify({}));
Expand Down Expand Up @@ -63,9 +103,9 @@ describe("FactorydroidHooks", () => {

const content = factorydroidHooks.getFileContent();
const parsed = JSON.parse(content);
expect(parsed.hooks.SessionStart).toBeDefined();
expect(parsed.hooks.Stop).toBeDefined();
expect(parsed.hooks.afterFileEdit).toBeUndefined();
expect(parsed.SessionStart).toBeDefined();
expect(parsed.Stop).toBeDefined();
expect(parsed.afterFileEdit).toBeUndefined();
});

it("should prefix non-absolute commands with $FACTORY_PROJECT_DIR", async () => {
Expand Down Expand Up @@ -94,7 +134,7 @@ describe("FactorydroidHooks", () => {

const content = factorydroidHooks.getFileContent();
const parsed = JSON.parse(content);
const sessionStartEntry = parsed.hooks.SessionStart[0];
const sessionStartEntry = parsed.SessionStart[0];
expect(sessionStartEntry).toBeDefined();
expect(sessionStartEntry.matcher).toBeUndefined();
expect(sessionStartEntry.hooks[0].command).toContain("$FACTORY_PROJECT_DIR");
Expand Down Expand Up @@ -127,7 +167,7 @@ describe("FactorydroidHooks", () => {

const content = factorydroidHooks.getFileContent();
const parsed = JSON.parse(content);
expect(parsed.hooks.SessionStart[0].hooks[0].command).toBe(
expect(parsed.SessionStart[0].hooks[0].command).toBe(
'"$FACTORY_PROJECT_DIR"/scripts/format.sh --fix --quiet',
);
});
Expand Down Expand Up @@ -160,7 +200,7 @@ describe("FactorydroidHooks", () => {

const content = factorydroidHooks.getFileContent();
const parsed = JSON.parse(content);
expect(parsed.hooks.SessionStart[0].hooks[0].command).toBe(
expect(parsed.SessionStart[0].hooks[0].command).toBe(
"$FACTORY_PROJECT_DIR/.factory/hooks/start.sh",
);
});
Expand Down Expand Up @@ -202,9 +242,9 @@ describe("FactorydroidHooks", () => {

const content = factorydroidHooks.getFileContent();
const parsed = JSON.parse(content);
expect(parsed.hooks.SessionStart[0].hooks[0].command).toContain("factory-override.sh");
expect(parsed.hooks.Notification).toBeDefined();
expect(parsed.hooks.Notification[0].matcher).toBe("permission_prompt");
expect(parsed.SessionStart[0].hooks[0].command).toContain("factory-override.sh");
expect(parsed.Notification).toBeDefined();
expect(parsed.Notification[0].matcher).toBe("permission_prompt");
});

it("should not leak Claude config hooks into Factory Droid output", async () => {
Expand Down Expand Up @@ -245,9 +285,9 @@ describe("FactorydroidHooks", () => {
const content = factorydroidHooks.getFileContent();
const parsed = JSON.parse(content);
// Shared hooks should be present
expect(parsed.hooks.SessionStart[0].hooks[0].command).toContain("shared.sh");
expect(parsed.SessionStart[0].hooks[0].command).toContain("shared.sh");
// Factory Droid-specific override should be present
expect(parsed.hooks.Stop).toBeDefined();
expect(parsed.Stop).toBeDefined();
// Claude-specific hooks must NOT leak into Factory Droid output
expect(JSON.stringify(parsed)).not.toContain("claude-only.sh");
expect(JSON.stringify(parsed)).not.toContain("claude-notify.sh");
Expand Down Expand Up @@ -303,8 +343,8 @@ describe("FactorydroidHooks", () => {
const content = factorydroidHooks.getFileContent();
const parsed = JSON.parse(content);
expect(parsed.otherKey).toBe("preserved");
expect(parsed.hooks).toBeDefined();
expect(parsed.hooks.SessionStart).toBeDefined();
expect(parsed.hooks).toBeUndefined();
expect(parsed.SessionStart).toBeDefined();
});

it("should handle hooks with matcher grouping", async () => {
Expand Down Expand Up @@ -337,15 +377,15 @@ describe("FactorydroidHooks", () => {

const content = factorydroidHooks.getFileContent();
const parsed = JSON.parse(content);
expect(parsed.hooks.PreToolUse).toHaveLength(2);
expect(parsed.PreToolUse).toHaveLength(2);

const writeEntry = parsed.hooks.PreToolUse.find(
const writeEntry = parsed.PreToolUse.find(
(e: Record<string, unknown>) => e.matcher === "Write",
);
expect(writeEntry).toBeDefined();
expect(writeEntry.hooks).toHaveLength(2);

const editEntry = parsed.hooks.PreToolUse.find(
const editEntry = parsed.PreToolUse.find(
(e: Record<string, unknown>) => e.matcher === "Edit",
);
expect(editEntry).toBeDefined();
Expand Down Expand Up @@ -378,7 +418,7 @@ describe("FactorydroidHooks", () => {

const content = factorydroidHooks.getFileContent();
const parsed = JSON.parse(content);
const hookDef = parsed.hooks.PreToolUse[0].hooks[0];
const hookDef = parsed.PreToolUse[0].hooks[0];
expect(hookDef.type).toBe("prompt");
expect(hookDef.prompt).toBe("Check this tool call");
expect(hookDef.timeout).toBe(30000);
Expand Down Expand Up @@ -409,7 +449,7 @@ describe("FactorydroidHooks", () => {
});

const parsed = JSON.parse(factorydroidHooks.getFileContent());
const hookDef = parsed.hooks.PreToolUse[0].hooks[0];
const hookDef = parsed.PreToolUse[0].hooks[0];
expect(hookDef.prompt).toBe("Check this tool call");
expect(hookDef.model).toBeUndefined();
});
Expand Down
8 changes: 4 additions & 4 deletions src/features/hooks/factorydroid-hooks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ export class FactorydroidHooks extends ToolHooks {
converterConfig: FACTORYDROID_CONVERTER_CONFIG,
logger,
});
const merged = { ...settings, hooks: factorydroidHooks };
const merged = { ...settings, ...factorydroidHooks };

@dyoshikawa dyoshikawa Jul 20, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the crux: the Factory hooks reference documents hooks.json as { "hooks": { "EventName": [...] } } — nested under a top-level hooks key, same as what main emits today. Flattening the events to the top level produces a file Droid won't load. Also, switching from hooks: factorydroidHooks (wholesale replacement of the key we own) to a spread means stale event keys from previous generations are never removed — deleting a hook from .rulesync/hooks.jsonc and regenerating leaves the removed event behind, and a file generated by current rulesync keeps its old nested hooks block forever alongside the new top-level keys.

const fileContent = JSON.stringify(merged, null, 2);
return new FactorydroidHooks({
outputRoot,
Expand All @@ -127,9 +127,9 @@ export class FactorydroidHooks extends ToolHooks {
}

toRulesyncHooks(): RulesyncHooks {
let settings: { hooks?: unknown };
let parsed: { hooks?: unknown };
try {
settings = JSON.parse(this.getFileContent());
parsed = JSON.parse(this.getFileContent());
} catch (error) {
throw new Error(
`Failed to parse Factory Droid hooks content in ${join(this.getRelativeDirPath(), this.getRelativeFilePath())}: ${formatError(error)}`,
Expand All @@ -139,7 +139,7 @@ export class FactorydroidHooks extends ToolHooks {
);
}
const hooks = toolHooksToCanonical({

@dyoshikawa dyoshikawa Jul 20, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parsed.hooks ?? parsed prefers a leftover legacy hooks block over the top-level events. Combined with the spread-merge above, regenerating over a v14-format file leaves both shapes in the file, and import then silently canonicalizes the stale nested block instead of the fresh top-level one.

hooks: settings.hooks,
hooks: parsed.hooks ?? parsed,
converterConfig: FACTORYDROID_CONVERTER_CONFIG,
});
return this.toRulesyncHooksDefault({
Expand Down
Loading