From 5a465e4f7335235d43dae772a77683aff0a65e7b Mon Sep 17 00:00:00 2001 From: jeffyxu Date: Fri, 31 Jul 2026 17:33:14 +0800 Subject: [PATCH 1/2] fix(hooks): simplify project-scope injection and dispatch config (#264) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two changes: 1. hook-dispatch-cli now prefers detectProjectConfig(stdin.cwd) over the hardcoded loadLocalConfig(), so filterHandlersForConfig honours project-level repo.kind (e.g. suppressing gitOnly handlers for HTTP projects). 2. resolveHookScopeTargets no longer writes a redundant copy of hook entries into /.claude/settings.json — HOME already covers every cwd. hooksRemove still cleans up legacy projectRoot entries left by older versions. Co-Authored-By: Claude Fable 5 --- src/__tests__/hooks-cmd.test.ts | 73 +++++++++++++++------------------ src/hook-dispatch-cli.ts | 10 +++-- src/hooks-cmd.ts | 44 ++++++++++---------- 3 files changed, 63 insertions(+), 64 deletions(-) diff --git a/src/__tests__/hooks-cmd.test.ts b/src/__tests__/hooks-cmd.test.ts index bf3e1d61..9afb3d35 100644 --- a/src/__tests__/hooks-cmd.test.ts +++ b/src/__tests__/hooks-cmd.test.ts @@ -106,7 +106,7 @@ describe('hooksInject', () => { await expect(hooksInject({})).rejects.toThrow('not initialized'); }); - it('reconciles into project and user base dirs when project config detected', async () => { + it('reconciles only into HOME when project config detected (#264)', async () => { const restoreHome = mockHome('/home/testuser'); mockedAutoDetectInit.mockResolvedValue({ localConfig: { ...mockLocalConfig, scope: 'project', projectRoot: '/path/to/project' }, @@ -118,20 +118,15 @@ describe('hooksInject', () => { restoreHome(); } - expect(mockedReconcile).toHaveBeenCalledTimes(2); - expect(mockedReconcile).toHaveBeenNthCalledWith(1, mockTeamConfig.toolPaths, '/path/to/project', TEAM_DEFS, expect.any(String), { builtinOverride: undefined }); - expect(mockedReconcile).toHaveBeenNthCalledWith(2, mockTeamConfig.toolPaths, '/home/testuser', TEAM_DEFS, expect.any(String), { builtinOverride: undefined }); - - // #85: the user-home target must be reconciled against the USER's own - // manifest, not the project's — otherwise `pull`'s per-scope reconcile - // (which always uses each scope's own manifest) diverges from `inject`, - // causing duplicate injection / wrongful cleanup of the shared file. - const projectManifestPath = mockedReconcile.mock.calls[0][3] as string; - const userManifestPath = mockedReconcile.mock.calls[1][3] as string; - expect(userManifestPath).not.toBe(projectManifestPath); - expect(projectManifestPath).toContain('/path/to/project'); - expect(userManifestPath).toContain('/home/testuser'); - expect(userManifestPath).not.toContain('/path/to/project'); + // #264: project scope no longer writes a redundant copy into projectRoot; + // HOME covers all cwds, and dispatch identifies the project via stdin.cwd. + expect(mockedReconcile).toHaveBeenCalledTimes(1); + expect(mockedReconcile).toHaveBeenCalledWith( + mockTeamConfig.toolPaths, '/home/testuser', TEAM_DEFS, expect.any(String), { builtinOverride: undefined }, + ); + const manifestPath = mockedReconcile.mock.calls[0][3] as string; + expect(manifestPath).toContain('/home/testuser'); + expect(manifestPath).not.toContain('/path/to/project'); }); }); @@ -197,7 +192,7 @@ describe('hooksList', () => { } }); - it('should list project and user base dirs when project config detected', async () => { + it('should list only HOME base dir when project config detected (#264)', async () => { const restoreHome = mockHome('/home/testuser'); const consoleLog = vi.spyOn(console, 'log').mockImplementation(() => undefined); const projectConfig = { @@ -206,17 +201,22 @@ describe('hooksList', () => { projectRoot: '/path/to/project', }; mockedAutoDetectInit.mockResolvedValue({ localConfig: projectConfig, teamConfig: mockTeamConfig }); + mockedGetHookStatus + .mockResolvedValueOnce('installed') + .mockResolvedValueOnce('missing') + .mockResolvedValueOnce('installed'); try { await hooksList({}); - expect(mockedGetHookStatus).toHaveBeenCalledTimes(6); + // #264: project scope only checks HOME, not projectRoot. + expect(mockedGetHookStatus).toHaveBeenCalledTimes(3); expect(mockedGetHookStatus).toHaveBeenCalledWith( - path.join('/path/to/project', '.claude/settings.json'), + path.join('/home/testuser', '.claude/settings.json'), 'claude', ); - expect(mockedGetHookStatus).toHaveBeenCalledWith( - path.join('/home/testuser', '.claude/settings.json'), + expect(mockedGetHookStatus).not.toHaveBeenCalledWith( + path.join('/path/to/project', '.claude/settings.json'), 'claude', ); } finally { @@ -247,7 +247,7 @@ describe('hooksRemove', () => { expect(mockedLog.success).toHaveBeenCalledWith(expect.stringContaining('Hooks removed')); }); - it('removes from project and user base dirs when project config detected', async () => { + it('removes from HOME and cleans up legacy projectRoot entries (#264)', async () => { const restoreHome = mockHome('/home/testuser'); mockedAutoDetectInit.mockResolvedValue({ localConfig: { ...mockLocalConfig, scope: 'project', projectRoot: '/path/to/project' }, @@ -258,27 +258,22 @@ describe('hooksRemove', () => { } finally { restoreHome(); } + // 1 main (HOME) + 1 legacy cleanup (projectRoot) expect(mockedReconcile).toHaveBeenCalledTimes(2); - // #85: same per-scope manifest requirement as `hooksInject`. - const projectManifestPath = mockedReconcile.mock.calls[0][3] as string; - const userManifestPath = mockedReconcile.mock.calls[1][3] as string; - expect(userManifestPath).not.toBe(projectManifestPath); - expect(userManifestPath).not.toContain('/path/to/project'); - }); + // Main removal targets HOME with user manifest. + expect(mockedReconcile).toHaveBeenNthCalledWith(1, + mockTeamConfig.toolPaths, '/home/testuser', [], expect.any(String), { removeAll: true }, + ); + const userManifest = mockedReconcile.mock.calls[0][3] as string; + expect(userManifest).toContain('/home/testuser'); - it('does not duplicate when HOME equals projectRoot', async () => { - const restoreHome = mockHome('/path/to/project'); - mockedAutoDetectInit.mockResolvedValue({ - localConfig: { ...mockLocalConfig, scope: 'project', projectRoot: '/path/to/project' }, - teamConfig: mockTeamConfig, - }); - try { - await hooksRemove({}); - } finally { - restoreHome(); - } - expect(mockedReconcile).toHaveBeenCalledTimes(1); + // Legacy cleanup targets projectRoot with project manifest. + expect(mockedReconcile).toHaveBeenNthCalledWith(2, + mockTeamConfig.toolPaths, '/path/to/project', [], expect.any(String), { removeAll: true }, + ); + const legacyManifest = mockedReconcile.mock.calls[1][3] as string; + expect(legacyManifest).toContain('/path/to/project'); }); it('propagates error when not initialized', async () => { diff --git a/src/hook-dispatch-cli.ts b/src/hook-dispatch-cli.ts index bb97097a..b40667e1 100644 --- a/src/hook-dispatch-cli.ts +++ b/src/hook-dispatch-cli.ts @@ -160,10 +160,12 @@ export async function hookDispatchCli( if (stdin === null) return; // Provider-config gate: HTTP-only teams must not receive git-provider-only - // hook prompts (contribute / mr-hint / votes). Filter keyed on teamai's own - // configured source, independent of the cwd's git remote. - const { loadLocalConfig } = await import('./config.js'); - const localConfig = await loadLocalConfig(); + // hook prompts (contribute / mr-hint / votes). Prefer the project-scope + // config when the host tells us the working directory (#264), so + // filterHandlersForConfig can honour a project-level repo.kind. + const { loadLocalConfig, detectProjectConfig } = await import('./config.js'); + const cwd = typeof stdin.cwd === 'string' ? stdin.cwd : undefined; + const localConfig = (cwd ? await detectProjectConfig(cwd) : null) ?? await loadLocalConfig(); const handlers = filterHandlersForConfig(buildHandlerRegistry(), localConfig); const dispatcher = createDispatcher({ handlers }); diff --git a/src/hooks-cmd.ts b/src/hooks-cmd.ts index 9ea75827..129902b9 100644 --- a/src/hooks-cmd.ts +++ b/src/hooks-cmd.ts @@ -21,31 +21,26 @@ interface HookScopeTarget { } /** - * Resolve every (baseDir, manifestPath) pair that hook reconciliation must - * touch for this config's scope. Project scope also targets the user's home - * directory (so hooks fire from subdirectories, see #44) — but that target - * MUST use the user's own manifest, not the project's. Attributing the - * user-home write to the project's manifest is what let `hooks inject` - * diverge from `pull`'s per-scope reconcile (each scope's own baseDir + - * manifest, see reconcileHooksAllScopes in pull.ts) and caused duplicate - * injection / wrongful cleanup of ~/.cursor/hooks.json et al. (#85). + * Resolve the (baseDir, manifestPath) pair for hook reconciliation. + * + * User scope → HOME + user manifest (unchanged). + * Project scope → HOME + user manifest only (#264). The previous behaviour + * duplicated entries into as well (for #44 subdirectory + * coverage), but HOME already covers every cwd. The dispatch runtime now + * identifies the active project via detectProjectConfig(stdin.cwd), so a + * redundant projectRoot copy is unnecessary. */ function resolveHookScopeTargets(localConfig: LocalConfig): HookScopeTarget[] { - const baseDir = resolveBaseDir(localConfig) ?? ''; - const manifestPath = getManagedHooksPath(localConfig.scope, localConfig.projectRoot); if (localConfig.scope !== 'project') { - return [{ baseDir, manifestPath }]; + return [{ + baseDir: resolveBaseDir(localConfig) ?? '', + manifestPath: getManagedHooksPath(localConfig.scope, localConfig.projectRoot), + }]; } - - const userBaseDir = process.env.HOME ?? ''; - if (!userBaseDir || userBaseDir === baseDir) { - return [{ baseDir, manifestPath }]; - } - - return [ - { baseDir, manifestPath }, - { baseDir: userBaseDir, manifestPath: getManagedHooksPath('user') }, - ]; + return [{ + baseDir: process.env.HOME ?? '', + manifestPath: getManagedHooksPath('user'), + }]; } function formatDisplayPath(settingsPath: string): string { @@ -159,5 +154,12 @@ export async function hooksRemove(_options: GlobalOptions): Promise { await reconcileHooksToAllTools(teamConfig.toolPaths, baseDir, [], manifestPath, { removeAll: true }); } + // Clean up legacy projectRoot entries left by older versions that wrote + // hooks into both and HOME (#264 migration). + if (localConfig.scope === 'project' && localConfig.projectRoot) { + const legacyManifest = getManagedHooksPath('project', localConfig.projectRoot); + await reconcileHooksToAllTools(teamConfig.toolPaths, localConfig.projectRoot, [], legacyManifest, { removeAll: true }); + } + log.success('Hooks removed from all AI tool settings'); } From 563ba372f74917cb15f1a9fcdb3e10b6f298c8b6 Mon Sep 17 00:00:00 2001 From: jeffyxu Date: Fri, 31 Jul 2026 19:52:20 +0800 Subject: [PATCH 2/2] test(e2e): update scope-isolation E2E for #264 hook simplification Align the #85 E2E assertions with the new behaviour: project-scope inject now writes only to HOME, and the redundant projectRoot copy is no longer created. Co-Authored-By: Claude Fable 5 --- .../e2e/scope-isolation-issue85.test.ts | 25 ++++++++----------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/src/__tests__/e2e/scope-isolation-issue85.test.ts b/src/__tests__/e2e/scope-isolation-issue85.test.ts index 3a747652..bc21f750 100644 --- a/src/__tests__/e2e/scope-isolation-issue85.test.ts +++ b/src/__tests__/e2e/scope-isolation-issue85.test.ts @@ -11,9 +11,9 @@ import { fileURLToPath } from 'node:url'; // upvote scope (see scope-isolation-e2e.test.ts). This file drives the real // CLI binary against offline git fixtures to cover the four gaps that were // still open after those landed: -// 1. `hooks inject`/`hooks remove` must track the user-home copy under the -// USER's own manifest, not the project's (else duplicate injection / -// wrongful cleanup of the shared tool settings file). +// 1. `hooks inject`/`hooks remove` must write only to the user's HOME +// directory (#264 simplified this: project scope no longer writes a +// redundant copy into projectRoot). // 2. `tags subscribe`/`unsubscribe` must write to the active scope's // config.yaml, not always ~/.teamai/config.yaml. // 3. `contribute` must make a new learning immediately recallable, without @@ -163,26 +163,23 @@ describe('issue #85 remaining scope-isolation gaps (e2e)', () => { }); }); - describe('hooks inject/remove manifest scoping (item 1)', () => { - it('inject creates BOTH a project manifest and a user manifest, each tracking its own directory', async () => { + describe('hooks inject/remove manifest scoping (item 1, updated by #264)', () => { + it('inject writes only to the user HOME, not to projectRoot (#264)', async () => { const res = await runCLI(['hooks', 'inject'], { HOME: homeDir }, projectRoot); expect(res.code, res.output).toBe(0); + // #264: project scope no longer writes a redundant copy into projectRoot. const projectManifestPath = path.join(projectRoot, '.teamai', 'managed-hooks.json'); const userManifestPath = path.join(homeDir, '.teamai', 'managed-hooks.json'); - expect(fs.existsSync(projectManifestPath)).toBe(true); + expect(fs.existsSync(projectManifestPath)).toBe(false); expect(fs.existsSync(userManifestPath)).toBe(true); - const projectManifest = JSON.parse(fs.readFileSync(projectManifestPath, 'utf-8')); const userManifest = JSON.parse(fs.readFileSync(userManifestPath, 'utf-8')); - expect(projectManifest.claude?.[0]?.command).toContain('teamai-e2e-hook-marker'); expect(userManifest.claude?.[0]?.command).toContain('teamai-e2e-hook-marker'); - // Both settings files actually received the hook (the #44 behavior of - // also writing into the user's home dir must still work). - const projectSettings = fs.readFileSync(path.join(projectRoot, '.claude', 'settings.json'), 'utf-8'); + // Only HOME settings receives the hook; projectRoot is untouched. + expect(fs.existsSync(path.join(projectRoot, '.claude', 'settings.json'))).toBe(false); const userSettings = fs.readFileSync(path.join(homeDir, '.claude', 'settings.json'), 'utf-8'); - expect(projectSettings).toContain('teamai-e2e-hook-marker'); expect(userSettings).toContain('teamai-e2e-hook-marker'); }); @@ -198,13 +195,11 @@ describe('issue #85 remaining scope-isolation gaps (e2e)', () => { expect(afterCount).toBe(beforeCount); }); - it('remove cleans up both the project and user copies', async () => { + it('remove cleans up the user HOME copy', async () => { const res = await runCLI(['hooks', 'remove'], { HOME: homeDir }, projectRoot); expect(res.code, res.output).toBe(0); - const projectSettings = fs.readFileSync(path.join(projectRoot, '.claude', 'settings.json'), 'utf-8'); const userSettings = fs.readFileSync(path.join(homeDir, '.claude', 'settings.json'), 'utf-8'); - expect(projectSettings).not.toContain('teamai-e2e-hook-marker'); expect(userSettings).not.toContain('teamai-e2e-hook-marker'); }); });