Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
25 changes: 10 additions & 15 deletions src/__tests__/e2e/scope-isolation-issue85.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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');
});

Expand All @@ -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');
});
});
Expand Down
73 changes: 34 additions & 39 deletions src/__tests__/hooks-cmd.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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' },
Expand All @@ -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');
});
});

Expand Down Expand Up @@ -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 = {
Expand All @@ -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 {
Expand Down Expand Up @@ -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' },
Expand All @@ -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 () => {
Expand Down
10 changes: 6 additions & 4 deletions src/hook-dispatch-cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 });

Expand Down
44 changes: 23 additions & 21 deletions src/hooks-cmd.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 <projectRoot> 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 {
Expand Down Expand Up @@ -159,5 +154,12 @@ export async function hooksRemove(_options: GlobalOptions): Promise<void> {
await reconcileHooksToAllTools(teamConfig.toolPaths, baseDir, [], manifestPath, { removeAll: true });
}

// Clean up legacy projectRoot entries left by older versions that wrote
// hooks into both <projectRoot> 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');
}
Loading