From 6c4fe2affe45e0f8331fe4cf14120515444817bf Mon Sep 17 00:00:00 2001 From: ydflow <314143294+ydflow@users.noreply.github.com> Date: Fri, 25 Sep 2026 23:21:30 +0800 Subject: [PATCH 1/4] fix(pull): report hooks and MCP entry warnings on a dry run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `pull --dry-run` returned before the hooks and MCP reconcile stages, so the warnings those stages raise never reached the maintainer who ran the dry run to see exactly them: an unknown entry id, a per-entry `roles:` key, a hooks.yaml that does not parse. A dry run must resolve and warn, then skip the write (#822, item 3). 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 forwards it. Hooks had no dry-run path at all, so `reconcileTeamHooksForConfig` gained one: it resolves the entries, reports what it found, and stops before the first write. `resolveTeamHooks` takes a `preview` flag so the transparency line reads "Would apply N team hook(s)" instead of claiming they were applied. The tests drive `pull()` rather than the reconcile functions, the way pull-env-shape-warning.test.ts does: the defect was in the orchestration layer, so that is where it has to be pinned. They cover the dry-run warning, the zero writes, the unchanged real-pull behavior, and the dryRun forwarding. --- src/__tests__/pull-dry-run-hooks-mcp.test.ts | 217 +++++++++++++++++++ src/hooks.ts | 14 +- src/pull.ts | 13 +- src/resources/hooks.ts | 8 +- 4 files changed, 246 insertions(+), 6 deletions(-) create mode 100644 src/__tests__/pull-dry-run-hooks-mcp.test.ts 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..7a425d772 --- /dev/null +++ b/src/__tests__/pull-dry-run-hooks-mcp.test.ts @@ -0,0 +1,217 @@ +/** + * `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' } }, + }; + + 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)); + }); + + 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)); + }); + + 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 }), + ); + }); +}); diff --git a/src/hooks.ts b/src/hooks.ts index 1a039646a..6eb9b9405 100644 --- a/src/hooks.ts +++ b/src/hooks.ts @@ -1791,13 +1791,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,6 +1824,14 @@ 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. + // + // 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. + if (opts.dryRun) { + return resolved.ok ? { ok: true, defs: teamDefs } : { ok: false, builtins: builtinsOnly ?? 'with-overrides' }; + } await reconcileHooksToAllTools(scopedToolPaths(teamConfig, { ...localConfig, scope: hookScope }), baseDir, teamDefs, manifestPath, { removeAll: opts.removeAll, builtinOverride: builtin, diff --git a/src/pull.ts b/src/pull.ts index 9beddac85..c0e7bc3a4 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,6 +2122,7 @@ 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)`); @@ -2139,14 +2143,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) { 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}`); } From 39c00171b9e9721196457fa9ed82c6ce5730684f Mon Sep 17 00:00:00 2001 From: ydflow <314143294+ydflow@users.noreply.github.com> Date: Sat, 26 Sep 2026 13:22:28 +0800 Subject: [PATCH 2/4] fix(pull): preview-word the MCP dry-run summary and update the design doc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up: with dryRun forwarded, the MCP summary still read as a completed apply — "N change(s) ... Restart your AI tool session to load them" after a run that wrote nothing. A dry run now reads "Would make N change(s)" and omits the restart instruction; a real pull keeps the applied wording. docs/designs/multi-project-management.md still said `pull --dry-run` prints no hooks or MCP warnings; it now describes the resolve-and-warn behavior this PR ships. The new tests pin the wording on each side of the dry-run boundary: the dry-run case failed on the previous commit, the real-pull case passed. --- docs/designs/multi-project-management.md | 5 +++- src/__tests__/pull-dry-run-hooks-mcp.test.ts | 29 ++++++++++++++++++++ src/pull.ts | 9 +++++- 3 files changed, 41 insertions(+), 2 deletions(-) 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 index 7a425d772..89548df40 100644 --- a/src/__tests__/pull-dry-run-hooks-mcp.test.ts +++ b/src/__tests__/pull-dry-run-hooks-mcp.test.ts @@ -214,4 +214,33 @@ describe('pull --dry-run reports hooks and MCP entry warnings', () => { 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/pull.ts b/src/pull.ts index c0e7bc3a4..3b414aa22 100644 --- a/src/pull.ts +++ b/src/pull.ts @@ -2161,7 +2161,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}`); From 256090be7b9677349fa53a55aa9681fe39c344a5 Mon Sep 17 00:00:00 2001 From: ydflow <314143294+ydflow@users.noreply.github.com> Date: Sat, 26 Sep 2026 13:38:10 +0800 Subject: [PATCH 3/4] fix(pull): preview-word the hooks debug trail on a dry run The [scope] "Reconciled N team hook(s)" debug line still claimed a reconcile after a run that wrote nothing. It now reads "Would apply N team hook(s)" on a dry run and keeps "Reconciled" on a real pull; both wordings are pinned by the dry-run tests. --- src/__tests__/pull-dry-run-hooks-mcp.test.ts | 7 +++++++ src/pull.ts | 5 ++++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/__tests__/pull-dry-run-hooks-mcp.test.ts b/src/__tests__/pull-dry-run-hooks-mcp.test.ts index 89548df40..91550ea7f 100644 --- a/src/__tests__/pull-dry-run-hooks-mcp.test.ts +++ b/src/__tests__/pull-dry-run-hooks-mcp.test.ts @@ -181,6 +181,11 @@ describe('pull --dry-run reports hooks and MCP entry warnings', () => { 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 () => { @@ -201,6 +206,8 @@ describe('pull --dry-run reports hooks and MCP entry warnings', () => { 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('forwards dryRun to the MCP reconcile so its writes are skipped too', async () => { diff --git a/src/pull.ts b/src/pull.ts index 3b414aa22..ef99321b6 100644 --- a/src/pull.ts +++ b/src/pull.ts @@ -2125,7 +2125,10 @@ async function reconcileHooksAllScopes( 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}`); From ea398297c5915f343735d5013105d2b43069c4e2 Mon Sep 17 00:00:00 2001 From: ydflow <314143294+ydflow@users.noreply.github.com> Date: Sat, 26 Sep 2026 14:13:08 +0800 Subject: [PATCH 4/4] fix(hooks): report the Pi skip on a dry run, like a real reconcile does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The per-entry `tools: [pi]` case: a real pull warns "Pi supports built-in lifecycle hooks only; skipping N custom team hook(s)" during the per-tool pass, but the dry run returned before that pass, so its "Would apply" line promised hooks no tool will run. Extract the Pi report from the loop so both paths print the same warning — the dry run gated on the same toolPaths and agent filters the real pass uses — and pin it with a test that is RED without the fix. --- src/__tests__/pull-dry-run-hooks-mcp.test.ts | 32 +++++++++- src/hooks.ts | 64 ++++++++++++++------ 2 files changed, 76 insertions(+), 20 deletions(-) diff --git a/src/__tests__/pull-dry-run-hooks-mcp.test.ts b/src/__tests__/pull-dry-run-hooks-mcp.test.ts index 91550ea7f..38681aab2 100644 --- a/src/__tests__/pull-dry-run-hooks-mcp.test.ts +++ b/src/__tests__/pull-dry-run-hooks-mcp.test.ts @@ -130,7 +130,12 @@ describe('pull --dry-run reports hooks and MCP entry warnings', () => { sharing: { skills: {}, rules: { enforced: [] }, docs: { localDir: '' }, env: { injectShellProfile: true }, }, - toolPaths: { claude: { skills: '.claude/skills', rules: '.claude/rules', settings: '.claude/settings.json' } }, + 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); @@ -210,6 +215,31 @@ describe('pull --dry-run reports hooks and MCP entry warnings', () => { 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. diff --git a/src/hooks.ts b/src/hooks.ts index 6eb9b9405..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) { @@ -1829,10 +1846,19 @@ export async function reconcileTeamHooksForConfig( // 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(scopedToolPaths(teamConfig, { ...localConfig, scope: hookScope }), baseDir, teamDefs, manifestPath, { + await reconcileHooksToAllTools(hookToolPaths, baseDir, teamDefs, manifestPath, { removeAll: opts.removeAll, builtinOverride: builtin, filterAgents,