diff --git a/docs/designs/multi-project-management.md b/docs/designs/multi-project-management.md index d9d6aab54..82217dfd4 100644 --- a/docs/designs/multi-project-management.md +++ b/docs/designs/multi-project-management.md @@ -439,7 +439,10 @@ entry comes from. - No pull protects a local edit from being overwritten, override transitions included. - A mistyped per-entry key (`role:`) is stripped by the schema and the entry - reaches everyone; `pull --dry-run` prints no hooks or MCP warnings. + reaches everyone. `pull --dry-run` resolves the hooks and MCP entries and + reports their warnings (an unknown id, a deprecated per-entry `roles:`, a file + that does not parse) without writing, so a maintainer can see them before a + real pull applies them. ## Backward compatibility diff --git a/src/__tests__/pull-dry-run-hooks-mcp.test.ts b/src/__tests__/pull-dry-run-hooks-mcp.test.ts new file mode 100644 index 000000000..38681aab2 --- /dev/null +++ b/src/__tests__/pull-dry-run-hooks-mcp.test.ts @@ -0,0 +1,283 @@ +/** + * `pull --dry-run` used to return before the hooks and MCP reconcile stages + * (`if (options.dryRun) return`), so the warnings those stages raise — an + * unknown entry id, a per-entry `roles:` key, a hooks.yaml that does not + * parse — never reached the maintainer who ran the dry run to see exactly + * them (#822, item 3). A dry run must resolve and warn, then skip the write. + * + * MCP already had the capability: `McpReconcileOptions.dryRun` gates every + * write in mcp-reconcile.ts and `teamai mcp inject --dry-run` uses it, so + * `reconcileMcpAllScopes` only had to forward it. Hooks had no dry-run path at + * all, so `reconcileTeamHooksForConfig` gained one. + * + * The tests drive `pull()` rather than the reconcile functions, the same way + * pull-env-shape-warning.test.ts does: the defect was in the orchestration + * layer, so that is where it has to be pinned. + */ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import fse from 'fs-extra'; +import os from 'node:os'; +import path from 'node:path'; + +vi.mock('../config.js', async (importOriginal) => ({ + ...(await importOriginal()), + detectProjectConfig: vi.fn().mockResolvedValue(null), + loadLocalConfigForScope: vi.fn(), + loadStateForScope: vi.fn().mockResolvedValue({ lastPull: null, lastPullRev: null }), + loadTeamConfig: vi.fn(), + requireInit: vi.fn(), + saveStateForScope: vi.fn(), +})); + +vi.mock('../utils/git.js', () => ({ + getHeadRev: vi.fn().mockResolvedValue('abc1234'), + pullRepo: vi.fn().mockResolvedValue('already up to date'), +})); + +vi.mock('../utils/logger.js', () => ({ + log: { + debug: vi.fn(), error: vi.fn(), info: vi.fn(), success: vi.fn(), warn: vi.fn(), dim: vi.fn(), persist: vi.fn(), + }, + spinner: vi.fn(() => ({ + fail: vi.fn().mockReturnThis(), info: vi.fn().mockReturnThis(), + start: vi.fn().mockReturnThis(), stop: vi.fn().mockReturnThis(), + succeed: vi.fn().mockReturnThis(), warn: vi.fn().mockReturnThis(), + })), +})); + +vi.mock('../roles.js', () => ({ + loadRolesManifest: vi.fn().mockResolvedValue({ + version: 1, + roles: [{ + id: 'dev', + name: 'Dev', + description: '', + resources: { knowledge: ['common'], skills: ['common'], learnings: ['common'], agents: [] }, + }], + defaults: { shareTarget: 'primary-role' }, + }), + resolveRoleResourceNamespaces: vi.fn(() => ({ + knowledge: ['common'], skills: ['common'], learnings: ['common'], agents: [], + })), + // The entry resolver asks which roles this member holds, to apply the 0.25.0 + // per-entry `roles:` rule. Without it the mock is incomplete and the + // resolution throws, which pull swallows into a debug line. + activeRoleIds: vi.fn(() => ['dev']), +})); + +// Isolation: pull() takes a real ~/.teamai/.sync-lock. Parallel vitest workers +// sharing that path race and skip/error, so these tests mock the lock. +vi.mock('../update.js', () => ({ + acquireLock: vi.fn().mockResolvedValue(true), + releaseLock: vi.fn().mockResolvedValue(undefined), +})); + +// The end-of-pull checks are exercised in pull-post-checks.test.ts; keep them +// out of the way here so a warning under test is the only thing on the wire. +vi.mock('../doctor.js', async (importOriginal) => ({ + ...await importOriginal(), + resolveDoctorContext: vi.fn(), + buildChecks: vi.fn(), +})); + +// Mocked so the dry-run forwarding can be asserted on the arguments. The real +// implementation is covered by mcp-reconcile.test.ts; this file is about the +// wiring in pull. The spread keeps every other export real, so a symbol this +// file does not know about still resolves. +vi.mock('../mcp-reconcile.js', async (importOriginal) => ({ + ...await importOriginal(), + reconcileMcpForConfig: vi.fn().mockResolvedValue({ changes: [], wrote: false }), +})); + +import { detectProjectConfig, loadLocalConfigForScope, loadStateForScope, loadTeamConfig, saveStateForScope } from '../config.js'; +import { acquireLock } from '../update.js'; +import { buildChecks, resolveDoctorContext, type DoctorContext } from '../doctor.js'; +import { log } from '../utils/logger.js'; +import { pull } from '../pull.js'; +import { reconcileMcpForConfig } from '../mcp-reconcile.js'; +import type { LocalConfig, TeamaiConfig } from '../types.js'; + +const DEPRECATED_ROLES_WARNING = 'per-entry `roles:`'; + +describe('pull --dry-run reports hooks and MCP entry warnings', () => { + let tempDir: string; + let homeDir: string; + let repoPath: string; + + beforeEach(async () => { + tempDir = await fse.mkdtemp(path.join(os.tmpdir(), 'teamai-pull-dry-hooksmcp-')); + homeDir = path.join(tempDir, 'home'); + repoPath = path.join(tempDir, 'team-repo'); + vi.stubEnv('HOME', homeDir); + + await fse.ensureDir(path.join(homeDir, '.claude', 'skills')); + await fse.ensureDir(path.join(repoPath, 'manifest')); + await fse.writeFile(path.join(repoPath, 'manifest', 'roles.yaml'), 'version: 1\n'); + + const localConfig: LocalConfig = { + repo: { localPath: repoPath, remote: 'owner/repo' }, + username: 'tester', + scope: 'user', + primaryRole: 'dev', + additionalRoles: [], + }; + const teamConfig: TeamaiConfig = { + team: 'test', + description: '', + repo: 'owner/repo', + provider: 'github', + reviewers: [], + sharing: { + skills: {}, rules: { enforced: [] }, docs: { localDir: '' }, env: { injectShellProfile: true }, + }, + toolPaths: { + claude: { skills: '.claude/skills', rules: '.claude/rules', settings: '.claude/settings.json' }, + // Pi ships in every team's default toolPaths, so the dry-run Pi-skip + // report has the same reach a real reconcile's per-tool pass has. + pi: { skills: '.pi/skills', rules: '.pi/rules', claudemd: 'AGENTS.md' }, + }, + }; + + vi.mocked(detectProjectConfig).mockResolvedValue(null); + vi.mocked(loadLocalConfigForScope).mockResolvedValue(localConfig); + vi.mocked(loadTeamConfig).mockResolvedValue(teamConfig); + vi.mocked(loadStateForScope).mockResolvedValue({ lastPull: null, lastPullRev: null } as never); + + const ctx: DoctorContext = { + localConfig, + teamConfig, + toolPaths: teamConfig.toolPaths, + hookToolPaths: teamConfig.toolPaths, + baseDir: homeDir, + }; + vi.mocked(resolveDoctorContext).mockResolvedValue(ctx); + vi.mocked(buildChecks).mockResolvedValue([]); + // clearAllMocks resets calls, not implementations, so a test that makes the + // lock contended would otherwise leak into the next one. + vi.mocked(acquireLock).mockResolvedValue(true); + }); + + afterEach(async () => { + vi.unstubAllEnvs(); + vi.clearAllMocks(); + await fse.remove(tempDir); + }); + + /** A team hook scoped with the deprecated per-entry `roles:` key. */ + async function writeHooksWithDeprecatedRoles(): Promise { + await fse.ensureDir(path.join(repoPath, 'hooks')); + await fse.writeFile( + path.join(repoPath, 'hooks', 'hooks.yaml'), + [ + 'hooks:', + ' - id: lint', + ' description: Lint', + ' event: PostToolUse', + ' command: teamai hook-dispatch post-tool-use', + ' roles: [dev]', + '', + ].join('\n'), + ); + } + + it('warns about the deprecated per-entry `roles:` key on a dry run', async () => { + await writeHooksWithDeprecatedRoles(); + + await pull({ dryRun: true, force: true }); + + expect(log.warn).toHaveBeenCalledWith(expect.stringContaining(DEPRECATED_ROLES_WARNING)); + // The debug trail follows the same preview rule: a dry run must not log a + // reconcile that did not happen. + const debugLines = vi.mocked(log.debug).mock.calls.map(([m]) => String(m)); + expect(debugLines.some((l) => l.includes('Would apply 1 team hook(s)'))).toBe(true); + expect(debugLines.some((l) => l.includes('Reconciled'))).toBe(false); + }); + + it('writes no hook settings or manifest on a dry run', async () => { + await writeHooksWithDeprecatedRoles(); + + await pull({ dryRun: true, force: true }); + + // The reconcile stage is the only thing that writes these; a dry run must + // leave them absent even though it resolved and warned. + expect(await fse.pathExists(path.join(homeDir, '.claude', 'settings.json'))).toBe(false); + expect(await fse.pathExists(path.join(homeDir, '.teamai', 'managed-hooks.json'))).toBe(false); + expect(saveStateForScope).not.toHaveBeenCalled(); + }); + + it('still warns on a real pull, so the dry run reports what would happen', async () => { + await writeHooksWithDeprecatedRoles(); + + await pull({ force: true }); + + expect(log.warn).toHaveBeenCalledWith(expect.stringContaining(DEPRECATED_ROLES_WARNING)); + const debugLines = vi.mocked(log.debug).mock.calls.map(([m]) => String(m)); + expect(debugLines.some((l) => l.includes('Reconciled 1 team hook(s)'))).toBe(true); + }); + + it('reports the Pi skip on a dry run, like a real pull would', async () => { + // A hook scoped to Pi is never applied — Pi runs built-in lifecycle hooks + // only — and a real pull says so during the per-tool pass. The dry run + // stops before that pass but must repeat the skip, or its "Would apply" + // line promises hooks no tool will run. + await fse.ensureDir(path.join(repoPath, 'hooks')); + await fse.writeFile( + path.join(repoPath, 'hooks', 'hooks.yaml'), + [ + 'hooks:', + ' - id: pi-note', + ' description: Pi only', + ' event: PostToolUse', + ' command: teamai hook-dispatch post-tool-use', + ' tools: [pi]', + '', + ].join('\n'), + ); + await fse.ensureDir(path.join(homeDir, '.pi')); + + await pull({ dryRun: true, force: true }); + + expect(log.warn).toHaveBeenCalledWith(expect.stringContaining('Pi supports built-in lifecycle hooks only; skipping 1 custom team hook(s)')); + }); + + it('forwards dryRun to the MCP reconcile so its writes are skipped too', async () => { + // The MCP entry resolution runs inside reconcileMcpForConfig, which already + // gates its writes on dryRun; the bug was that pull never passed it. + await pull({ dryRun: true, force: true }); + + expect(reconcileMcpForConfig).toHaveBeenCalledWith( + expect.anything(), + expect.anything(), + expect.objectContaining({ dryRun: true }), + ); + }); + + it('reports MCP changes on a dry run without claiming they were applied', async () => { + // A dry run still returns the changes it *would* make — `wrote: false` is the + // only difference — so pull's summary line must not read as a completed + // apply. Reporting "Restart your AI tool session to load them" after a run + // that wrote nothing is the same class of defect as "Applying" was on the + // hooks side before the preview flag. + vi.mocked(reconcileMcpForConfig).mockResolvedValueOnce({ + changes: [{ tool: 'claude', server: 'team-server', action: 'added' }], + wrote: false, + }); + + await pull({ dryRun: true, force: true }); + + const lines = vi.mocked(log.info).mock.calls.map(([m]) => String(m)); + expect(lines.some((l) => l.includes('[dry-run]') && l.includes('Would make'))).toBe(true); + expect(lines.some((l) => l.includes('Restart your AI tool session'))).toBe(false); + }); + + it('keeps the applied wording on a real pull', async () => { + vi.mocked(reconcileMcpForConfig).mockResolvedValueOnce({ + changes: [{ tool: 'claude', server: 'team-server', action: 'added' }], + wrote: true, + }); + + await pull({ force: true }); + + expect(log.info).toHaveBeenCalledWith(expect.stringContaining('Restart your AI tool session to load them')); + }); +}); diff --git a/src/hooks.ts b/src/hooks.ts index 1a039646a..32b2f245d 100644 --- a/src/hooks.ts +++ b/src/hooks.ts @@ -1383,6 +1383,39 @@ async function isPiInstalled(baseDir: string, installedBaseDir?: string): Promis || await pathExists(path.join(installedBaseDir ?? baseDir, '.pi')); } +/** + * Pi cannot run custom team hooks — it supports built-in lifecycle hooks only — + * so anything the team scoped to Pi is skipped, and a real reconcile says so. + * Extracted from the reconcile loop so a dry run, which stops before the + * per-tool stage, prints the same report without writing anything and its + * "Would apply" line does not promise hooks no tool will run. + * + * Only warns when Pi is actually installed: Pi is in every team's default + * toolPaths, so without this gate teammates who never use Pi see this warning + * on every reconcile whenever the team defines a Pi-targeted hook. + */ +async function reportPiSkippedTeamHooks( + defs: HookDef[], + baseDir: string, + installedBaseDir: string | undefined, + builtinOverride: BuiltinHookOverride | undefined, +): Promise { + if (!await isPiInstalled(baseDir, installedBaseDir)) return; + const applicableTeamDefs = teamDefsForTool(defs, 'pi'); + if (applicableTeamDefs.length > 0) { + log.warn( + `Pi supports built-in lifecycle hooks only; skipping ${applicableTeamDefs.length} custom team hook(s) from hooks/hooks.yaml`, + ); + } + const builtinOverrideCount = (builtinOverride?.disabled?.length ?? 0) + + Object.keys(builtinOverride?.overrides ?? {}).length; + if (builtinOverrideCount > 0) { + log.warn( + `Pi supports built-in lifecycle hooks only; skipping ${builtinOverrideCount} built-in hook override(s) from hooks/hooks.yaml`, + ); + } +} + /** * Reconcile the single TeamAI Pi extension in the user agent directory. Pi * also auto-loads a project extensions dir with absolute-path dedup, so @@ -1626,24 +1659,8 @@ export async function reconcileHooksToAllTools( if (tool === 'pi') { if (opts.settingsOnly) continue; try { - // Only warn about skipped team/override hooks when Pi is actually - // installed — Pi is in every team's default toolPaths, so without this - // gate teammates who never use Pi see this warning on every - // reconcile whenever the team defines a Pi-targeted hook. - if (!opts.removeAll && await isPiInstalled(baseDir, opts.installedBaseDir)) { - const applicableTeamDefs = teamDefsForTool(defs, 'pi'); - if (applicableTeamDefs.length > 0) { - log.warn( - `Pi supports built-in lifecycle hooks only; skipping ${applicableTeamDefs.length} custom team hook(s) from hooks/hooks.yaml`, - ); - } - const builtinOverrideCount = (opts.builtinOverride?.disabled?.length ?? 0) - + Object.keys(opts.builtinOverride?.overrides ?? {}).length; - if (builtinOverrideCount > 0) { - log.warn( - `Pi supports built-in lifecycle hooks only; skipping ${builtinOverrideCount} built-in hook override(s) from hooks/hooks.yaml`, - ); - } + if (!opts.removeAll) { + await reportPiSkippedTeamHooks(defs, baseDir, opts.installedBaseDir, opts.builtinOverride); } await reconcilePiExtension(baseDir, opts.removeAll, opts.installedBaseDir); } catch (e) { @@ -1791,13 +1808,17 @@ export type TeamHooksReconcile = export async function reconcileTeamHooksForConfig( teamConfig: TeamaiConfig, localConfig: LocalConfig, - opts: { removeAll?: boolean; auto?: boolean; silent?: boolean; filterAgents?: string[] } = {}, + opts: { removeAll?: boolean; auto?: boolean; silent?: boolean; filterAgents?: string[]; dryRun?: boolean } = {}, ): Promise { const resolved: Awaited> = opts.removeAll ? { ok: true, defs: [], builtin: undefined } : await resolveTeamHooks(teamConfig, localConfig, { auto: opts.auto, silent: opts.silent, + // Resolve and report, then stop: a dry run must show the entry warnings + // and the hooks it would apply without touching any tool's settings + // (#822). + preview: opts.dryRun, }); // The team's hooks could not be resolved (reported by resolveTeamHooks). // Reconciling the team set now would remove every installed team hook, so @@ -1820,7 +1841,24 @@ export async function reconcileTeamHooksForConfig( // Resolve the tool paths at the scope hooks actually live in, not at the // config's scope: a non-self project scope puts hooks in HOME, so its paths // must be the user-scope ones. - await reconcileHooksToAllTools(scopedToolPaths(teamConfig, { ...localConfig, scope: hookScope }), baseDir, teamDefs, manifestPath, { + // + // A dry run stops here. The resolution above already reported the entry + // warnings and the hooks it would apply; everything below writes a tool's + // settings or the managed-hooks manifest. The result mirrors what a real + // reconcile would report, so a caller cannot tell them apart by the shape. + // + // One report happens below the stop point and a dry run must still make it: + // a tool that cannot run team hooks says so during the per-tool pass, and + // the preview is only honest when the dry run repeats it — for the tools the + // pass would actually reach, which is what hookToolPaths decides below too. + const hookToolPaths = scopedToolPaths(teamConfig, { ...localConfig, scope: hookScope }); + if (opts.dryRun) { + if (!opts.removeAll && 'pi' in hookToolPaths && (!filterAgents || filterAgents.includes('pi'))) { + await reportPiSkippedTeamHooks(teamDefs, baseDir, localConfig.scope === 'project' ? (localConfig.projectRoot ?? baseDir) : undefined, builtin); + } + return resolved.ok ? { ok: true, defs: teamDefs } : { ok: false, builtins: builtinsOnly ?? 'with-overrides' }; + } + await reconcileHooksToAllTools(hookToolPaths, baseDir, teamDefs, manifestPath, { removeAll: opts.removeAll, builtinOverride: builtin, filterAgents, diff --git a/src/pull.ts b/src/pull.ts index 9beddac85..ef99321b6 100644 --- a/src/pull.ts +++ b/src/pull.ts @@ -2108,7 +2108,10 @@ async function reconcileHooksAllScopes( projectConfig: LocalConfig | null, options: GlobalOptions, ): Promise { - if (options.dryRun) return; + // A dry run still resolves the entries, so the warnings a maintainer runs + // `--dry-run` to see — an unknown id, a deprecated per-entry `roles:`, a + // hooks.yaml that does not parse — are reported; only the writes are skipped, + // inside reconcileTeamHooksForConfig (#822). const scopes = [userConfig, projectConfig].filter((c): c is LocalConfig => !!c); for (const localConfig of scopes) { try { @@ -2119,9 +2122,13 @@ async function reconcileHooksAllScopes( auto: true, silent: options.silent, filterAgents: localConfig.enabledAgents, + dryRun: options.dryRun, }); if (reconciled.ok && reconciled.defs.length > 0) { - log.debug(`[${localConfig.scope}] Reconciled ${reconciled.defs.length} team hook(s)`); + // Same preview rule as the user-facing line: a dry run resolved and + // reported the entries but wrote nothing, so the debug trail must not + // claim a reconcile that did not happen. + log.debug(`[${localConfig.scope}] ${options.dryRun ? 'Would apply' : 'Reconciled'} ${reconciled.defs.length} team hook(s)`); } } catch (e) { log.debug(`[${localConfig.scope}] Hook reconcile skipped: ${(e as Error).message}`); @@ -2139,14 +2146,17 @@ async function reconcileMcpAllScopes( projectConfig: LocalConfig | null, options: GlobalOptions, ): Promise { - if (options.dryRun) return; + // Same contract as the hooks stage: resolve and report the entry warnings on + // a dry run, skip the writes. `reconcileMcpForConfig` already gates every + // write on `dryRun` (the `mcp inject --dry-run` path uses it), so this only + // forwards it (#822). const scopes = [userConfig, projectConfig].filter((c): c is LocalConfig => !!c); for (const localConfig of scopes) { try { const teamConfig = await loadTeamConfig(localConfig.repo.localPath); if (!teamConfig) continue; const { reconcileMcpForConfig } = await import('./mcp-reconcile.js'); - const { changes } = await reconcileMcpForConfig(teamConfig, localConfig, { force: options.force }); + const { changes } = await reconcileMcpForConfig(teamConfig, localConfig, { force: options.force, dryRun: options.dryRun }); const applied = changes.filter((c) => c.action !== 'skipped'); for (const c of changes) { @@ -2154,7 +2164,14 @@ async function reconcileMcpAllScopes( } if (applied.length > 0 && !options.silent) { const servers = [...new Set(applied.map((c) => c.server))]; - log.info(`MCP: ${applied.length} change(s) across ${servers.length} server(s). Restart your AI tool session to load them.`); + // A dry run reports the changes it would make (`wrote` stays false), so + // the summary must not read as a completed apply, nor tell the member to + // restart a session that has nothing new to load. + if (options.dryRun) { + log.info(`MCP: [dry-run] Would make ${applied.length} change(s) across ${servers.length} server(s)`); + } else { + log.info(`MCP: ${applied.length} change(s) across ${servers.length} server(s). Restart your AI tool session to load them.`); + } } } catch (e) { log.debug(`[${localConfig.scope}] MCP reconcile skipped: ${(e as Error).message}`); diff --git a/src/resources/hooks.ts b/src/resources/hooks.ts index 4ce741971..3444af26c 100644 --- a/src/resources/hooks.ts +++ b/src/resources/hooks.ts @@ -154,7 +154,7 @@ function isTeamScriptCommand(command: string): boolean { export async function resolveTeamHooks( teamConfig: TeamaiConfig, localConfig: LocalConfig, - opts: { auto?: boolean; silent?: boolean } = {}, + opts: { auto?: boolean; silent?: boolean; preview?: boolean } = {}, ): Promise<{ ok: true; defs: HookDef[]; builtin: BuiltinOverride | undefined } | { ok: false; builtin: BuiltinOverrideRead }> { // Which hooks this member receives: root plus active namespace files, before // the security gates so the transparency print below lists only hooks this @@ -187,7 +187,11 @@ export async function resolveTeamHooks( } if (defs.length > 0 && !opts.silent) { - log.info(`Applying ${defs.length} team hook(s):`); + // A preview must not claim the hooks were applied: `pull --dry-run` resolves + // them only to report what a real pull would write (#822). + log.info(opts.preview + ? `Would apply ${defs.length} team hook(s):` + : `Applying ${defs.length} team hook(s):`); for (const d of defs) log.info(` [${d.key}] ${d.command}`); }