From e17920f7606c30423de97d0cbae8e2656b81d47e Mon Sep 17 00:00:00 2001
From: ydflow <314143294+ydflow@users.noreply.github.com>
Date: Wed, 23 Sep 2026 18:50:55 +0800
Subject: [PATCH 1/2] fix(pull): gate the generic sync report on a tool that
 can receive it
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The generic branch of the sync loop reported the team repo's item count
for every resource type: `Synced N skills` counted what the repo holds,
not what landed. Skills are written per tool into that tool's own
directory, and a brand-new member has none of them yet — the handler
skips such a tool by design and only logs at debug. So the first pull
after `init` printed a success while nothing was on disk, which is the
phantom-success half of #585.

`hasInstalledTargetFor` asks the same question `getInstalledResourceTargets`
already asks, for one resource field, and the generic branch now gates
its report on it. Docs need no gate: they are copied to the team's own
docs directory, which the copy creates, so that report was already
truthful.

Only the report is gated. The writes still run, so a tool root created
later — Cursor makes `.cursor/` on first launch — is filled by the next
pull, and `pull --force` fills it now.

Fixes #585 (the generic branch). #597 fixed the same shape for rules
and left this one open; this closes it for skills.
---
 src/__tests__/pull-sync-truth.test.ts | 157 ++++++++++++++++++++++++++
 src/pull.ts                           |  41 ++++++-
 2 files changed, 194 insertions(+), 4 deletions(-)
 create mode 100644 src/__tests__/pull-sync-truth.test.ts

diff --git a/src/__tests__/pull-sync-truth.test.ts b/src/__tests__/pull-sync-truth.test.ts
new file mode 100644
index 00000000..f2be02e1
--- /dev/null
+++ b/src/__tests__/pull-sync-truth.test.ts
@@ -0,0 +1,157 @@
+import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
+import path from 'node:path';
+import os from 'node:os';
+import fse from 'fs-extra';
+
+// #585 / #574: `pull` counted the team repo's items and printed `Synced N`
+// whatever reached the tool's directory. #597 fixed that for rules and left the
+// generic branch open. These tests pin the generic branch: skills, docs and
+// agents must not claim a success the disk does not show.
+
+vi.mock('../config.js', async (importOriginal) => ({
+  ...(await importOriginal<typeof import('../config.js')>()),
+  requireInit: vi.fn(),
+  loadState: vi.fn(),
+  saveState: vi.fn(),
+  loadLocalConfigForScope: vi.fn(),
+  loadTeamConfig: vi.fn(),
+  detectProjectConfig: vi.fn().mockResolvedValue(null),
+  loadStateForScope: vi.fn().mockResolvedValue({ lastPull: null, lastPullRev: null }),
+  saveStateForScope: vi.fn(),
+}));
+
+vi.mock('../utils/git.js', () => ({
+  pullRepo: vi.fn().mockResolvedValue('already up to date'),
+  getHeadRev: vi.fn().mockResolvedValue('abc1234'),
+}));
+
+vi.mock('../utils/logger.js', () => ({
+  log: {
+    info: vi.fn(),
+    success: vi.fn(),
+    warn: vi.fn(),
+    error: vi.fn(),
+    debug: vi.fn(),
+    dim: vi.fn(),
+  },
+  spinner: vi.fn(() => ({
+    start: vi.fn().mockReturnThis(),
+    succeed: vi.fn().mockReturnThis(),
+    fail: vi.fn().mockReturnThis(),
+    warn: vi.fn().mockReturnThis(),
+    info: vi.fn().mockReturnThis(),
+    stop: vi.fn().mockReturnThis(),
+  })),
+}));
+
+vi.mock('../source.js', () => ({ pullSources: vi.fn().mockResolvedValue(undefined) }));
+vi.mock('../hooks.js', () => ({
+  injectHooksToAllTools: vi.fn().mockResolvedValue(undefined),
+  reconcileTeamHooksForConfig: vi.fn().mockResolvedValue([]),
+}));
+vi.mock('../mcp-reconcile.js', () => ({
+  reconcileMcpForConfig: vi.fn().mockResolvedValue({ changes: [], wrote: false }),
+}));
+vi.mock('../team-push.js', () => ({ reportUsageToTeam: vi.fn().mockResolvedValue(true) }));
+vi.mock('../usage-tracker.js', () => ({
+  readUsageEvents: vi.fn().mockResolvedValue([]),
+  truncateUsageAfterReport: vi.fn().mockResolvedValue(undefined),
+}));
+vi.mock('../update.js', () => ({
+  acquireLock: vi.fn().mockResolvedValue(true),
+  releaseLock: vi.fn().mockResolvedValue(undefined),
+}));
+
+import { pull } from '../pull.js';
+import { loadLocalConfigForScope, loadTeamConfig } from '../config.js';
+import { log } from '../utils/logger.js';
+import type { TeamaiConfig, LocalConfig } from '../types.js';
+
+describe('pull reports what reached the tool directory (#585)', () => {
+  let tmpDir: string;
+  let homeDir: string;
+  let repoPath: string;
+  let teamConfig: TeamaiConfig;
+  let localConfig: LocalConfig;
+
+  beforeEach(async () => {
+    vi.mocked(log.success).mockClear();
+    vi.mocked(log.warn).mockClear();
+    vi.mocked(log.info).mockClear();
+    tmpDir = await fse.mkdtemp(path.join(os.tmpdir(), 'teamai-sync-truth-'));
+    homeDir = path.join(tmpDir, 'home');
+    repoPath = path.join(tmpDir, 'repo');
+
+    // The team repo really holds one skill and one doc.
+    await fse.ensureDir(path.join(repoPath, 'skills', 'org-review'));
+    await fse.writeFile(
+      path.join(repoPath, 'skills', 'org-review', 'SKILL.md'),
+      '---\nname: org-review\ndescription: review workflow\n---\n',
+    );
+    await fse.ensureDir(path.join(repoPath, 'docs'));
+    await fse.writeFile(path.join(repoPath, 'docs', 'guide.md'), '# Guide\n');
+
+    // The tool's own directory is absent — a brand-new member who never ran
+    // the tool once. Nothing can land on disk.
+    await fse.ensureDir(homeDir);
+    vi.stubEnv('HOME', homeDir);
+
+    teamConfig = {
+      team: 'test',
+      description: '',
+      repo: 'https://git.woa.com/test/repo.git',
+      provider: 'tgit' as const,
+      reviewers: [],
+      sharing: {
+        skills: {},
+        rules: { enforced: [] },
+        docs: { localDir: 'docs' },
+        env: { injectShellProfile: true },
+      },
+      toolPaths: {
+        claude: { skills: '.claude/skills', rules: '.claude/rules' },
+      },
+    };
+
+    localConfig = {
+      repo: { localPath: repoPath, remote: 'https://git.woa.com/test/repo.git' },
+      username: 'member',
+      updatePolicy: 'auto',
+      additionalRoles: [],
+      scope: 'user',
+    };
+
+    vi.mocked(loadTeamConfig).mockResolvedValue(teamConfig);
+    vi.mocked(loadLocalConfigForScope).mockResolvedValue(localConfig);
+  });
+
+  afterEach(async () => {
+    vi.unstubAllEnvs();
+    vi.resetModules();
+    await fse.remove(tmpDir);
+  });
+
+  /** Every success line this run printed. Read fresh so a prior case cannot leak in. */
+  function successLines(): string[] {
+    return vi.mocked(log.success).mock.calls.map(([msg]) => String(msg));
+  }
+
+  it('claims no skills synced when no tool directory exists', async () => {
+    await pull({ silent: true });
+
+    expect(successLines().filter((msg) => /Synced \d+ skills/.test(msg))).toEqual([]);
+    expect(await fse.pathExists(path.join(homeDir, '.claude', 'skills', 'org-review'))).toBe(false);
+    // Docs are not gated: they are copied to the team's own docs directory,
+    // which the copy creates, so that report stays truthful.
+    expect(successLines().filter((msg) => /Synced \d+ docs/.test(msg)).length).toBeGreaterThan(0);
+    expect(await fse.pathExists(path.join(homeDir, 'docs', 'guide.md'))).toBe(true);
+  });
+
+  it('still claims skills synced once the tool directory exists', async () => {
+    await fse.ensureDir(path.join(homeDir, '.claude', 'skills'));
+    await pull({ silent: true });
+
+    expect(successLines().filter((msg) => /Synced \d+ skills/.test(msg)).length).toBeGreaterThan(0);
+    expect(await fse.pathExists(path.join(homeDir, '.claude', 'skills', 'org-review', 'SKILL.md'))).toBe(true);
+  });
+});
diff --git a/src/pull.ts b/src/pull.ts
index eabdd31e..3340e84d 100644
--- a/src/pull.ts
+++ b/src/pull.ts
@@ -575,6 +575,29 @@ function logSyncDetail(
   }
 }
 
+/**
+ * True when at least one enabled tool can receive `field`'s resources.
+ *
+ * Used to gate a "Synced N" claim: that count describes what the team repo
+ * holds, while this describes what could actually land. A tool whose root does
+ * not exist yet receives nothing — its handler skips the write by design and
+ * only logs at debug — so a fresh member would otherwise be shown a success the
+ * disk contradicts (#585).
+ */
+async function hasInstalledTargetFor(
+  teamConfig: TeamaiConfig,
+  localConfig: LocalConfig,
+  field: 'skills' | 'rules' | 'agents',
+): Promise<boolean> {
+  for (const [tool, toolPath] of Object.entries(scopedToolPaths(teamConfig, localConfig))) {
+    if (isAgentExcluded(localConfig, tool)) continue;
+    const resourcePath = toolPath[field];
+    if (!resourcePath) continue;
+    if (await isToolInstalledForConfig(tool, resourcePath, localConfig)) return true;
+  }
+  return false;
+}
+
 /**
  * Return the installed tool targets that can receive team-owned resources.
  *
@@ -1182,14 +1205,24 @@ async function pullForScope(
         }
       }
     } else {
+      // Skills land in a tool's own directory, which a brand-new member may not
+      // have yet. The handler skips such a tool by design and only logs at
+      // debug, so counting the team repo's items here would report a success the
+      // disk contradicts (#585). Docs need no gate: they are copied to the
+      // team's own docs directory, which the copy creates.
+      const canReceive = type !== 'skills'
+        || await hasInstalledTargetFor(freshConfig, localConfig, 'skills');
+
       for (const item of items) {
         await handler.pullItem(item, freshConfig, localConfig);
       }
 
-      if (type === 'skills') {
-        logSyncDetail(type, items, existingNames, !!options.verbose, scopeLabel, skippedByTags);
-      } else {
-        log.success(`[${scopeLabel}] Synced ${items.length} ${type}`);
+      if (canReceive) {
+        if (type === 'skills') {
+          logSyncDetail(type, items, existingNames, !!options.verbose, scopeLabel, skippedByTags);
+        } else {
+          log.success(`[${scopeLabel}] Synced ${items.length} ${type}`);
+        }
       }
     }
 

From c61ff5f42823a97c7a51d9469ea360debf2ab277 Mon Sep 17 00:00:00 2001
From: ydflow <314143294+ydflow@users.noreply.github.com>
Date: Wed, 23 Sep 2026 21:29:24 +0800
Subject: [PATCH 2/2] fix(pull): gate the agents sync report on a receiving
 tool too
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The gate only covered skills, so the generic branch still printed
`Synced N agents` when no installed tool could receive them — the same
phantom-success shape #585 describes, one resource type over.
AgentsHandler then resolves no destinations and writes nothing.

`hasInstalledTargetFor` also duplicated the walk that
`getInstalledResourceTargets` already performs. That function now takes
an optional `field`, and the gate calls it, so reporting and writing share
one resolver instead of two that can drift — an external HERMES_HOME or an
unresolvable workspace no longer disagrees between them.

Docs, rules and env never reach this branch; hooks and mcp have no
tool-path field to probe, so they keep reporting unconditionally.

Tests add the agent case the file's header already claimed: no tool
directory suppresses the claim and nothing lands, and the claim returns
once the directory exists. The suppression case is RED without the gate.
---
 src/__tests__/pull-sync-truth.test.ts | 36 ++++++++++++++++++++
 src/pull.ts                           | 48 ++++++++++-----------------
 2 files changed, 53 insertions(+), 31 deletions(-)

diff --git a/src/__tests__/pull-sync-truth.test.ts b/src/__tests__/pull-sync-truth.test.ts
index f2be02e1..bf1d93ca 100644
--- a/src/__tests__/pull-sync-truth.test.ts
+++ b/src/__tests__/pull-sync-truth.test.ts
@@ -154,4 +154,40 @@ describe('pull reports what reached the tool directory (#585)', () => {
     expect(successLines().filter((msg) => /Synced \d+ skills/.test(msg)).length).toBeGreaterThan(0);
     expect(await fse.pathExists(path.join(homeDir, '.claude', 'skills', 'org-review', 'SKILL.md'))).toBe(true);
   });
+
+  it('claims no agents synced when no tool directory can receive them', async () => {
+    // The same phantom-success shape as skills, on the agents branch: the team
+    // repo holds an agent, the tool root is absent, so the handler skips the
+    // write and the report must not claim otherwise.
+    await fse.ensureDir(path.join(repoPath, 'agents'));
+    await fse.writeFile(
+      path.join(repoPath, 'agents', 'reviewer.md'),
+      '---\nname: reviewer\ndescription: reviews code\n---\nReview things.\n',
+    );
+    teamConfig.toolPaths = {
+      claude: { skills: '.claude/skills', rules: '.claude/rules', agents: '.claude/agents' },
+    };
+
+    await pull({ silent: true });
+
+    expect(successLines().filter((msg) => /Synced \d+ agents/.test(msg))).toEqual([]);
+    expect(await fse.pathExists(path.join(homeDir, '.claude', 'agents', 'reviewer.md'))).toBe(false);
+  });
+
+  it('still claims agents synced once the tool directory exists', async () => {
+    await fse.ensureDir(path.join(repoPath, 'agents'));
+    await fse.writeFile(
+      path.join(repoPath, 'agents', 'reviewer.md'),
+      '---\nname: reviewer\ndescription: reviews code\n---\nReview things.\n',
+    );
+    teamConfig.toolPaths = {
+      claude: { skills: '.claude/skills', rules: '.claude/rules', agents: '.claude/agents' },
+    };
+    await fse.ensureDir(path.join(homeDir, '.claude', 'agents'));
+
+    await pull({ silent: true });
+
+    expect(successLines().filter((msg) => /Synced \d+ agents/.test(msg)).length).toBeGreaterThan(0);
+    expect(await fse.pathExists(path.join(homeDir, '.claude', 'agents', 'reviewer.md'))).toBe(true);
+  });
 });
diff --git a/src/pull.ts b/src/pull.ts
index 3340e84d..2d265176 100644
--- a/src/pull.ts
+++ b/src/pull.ts
@@ -575,35 +575,17 @@ function logSyncDetail(
   }
 }
 
-/**
- * True when at least one enabled tool can receive `field`'s resources.
- *
- * Used to gate a "Synced N" claim: that count describes what the team repo
- * holds, while this describes what could actually land. A tool whose root does
- * not exist yet receives nothing — its handler skips the write by design and
- * only logs at debug — so a fresh member would otherwise be shown a success the
- * disk contradicts (#585).
- */
-async function hasInstalledTargetFor(
-  teamConfig: TeamaiConfig,
-  localConfig: LocalConfig,
-  field: 'skills' | 'rules' | 'agents',
-): Promise<boolean> {
-  for (const [tool, toolPath] of Object.entries(scopedToolPaths(teamConfig, localConfig))) {
-    if (isAgentExcluded(localConfig, tool)) continue;
-    const resourcePath = toolPath[field];
-    if (!resourcePath) continue;
-    if (await isToolInstalledForConfig(tool, resourcePath, localConfig)) return true;
-  }
-  return false;
-}
-
 /**
  * Return the installed tool targets that can receive team-owned resources.
  *
  * Tools in `disabledAgents`, and tools outside `enabledAgents` when that
  * whitelist is set, are omitted — the same gate resource handlers use.
  *
+ * Pass `field` to ask about one resource type instead of "any of them": the
+ * generic sync loop needs that to decide whether a "Synced N" claim describes
+ * anything that could land, and a tool whose skills root is absent while its
+ * agents root exists must answer differently for each.
+ *
  * The revision cache is shared by a scope, while tool roots can appear later
  * (for example, when Cursor creates `.cursor/` on its first launch). Persisting
  * this set alongside the revision prevents a pull for one tool from suppressing
@@ -612,13 +594,14 @@ async function hasInstalledTargetFor(
 async function getInstalledResourceTargets(
   teamConfig: TeamaiConfig,
   localConfig: LocalConfig,
+  field?: 'skills' | 'rules' | 'agents',
 ): Promise<string[]> {
   const targets: string[] = [];
 
   for (const [tool, toolPath] of Object.entries(scopedToolPaths(teamConfig, localConfig))) {
     if (isAgentExcluded(localConfig, tool)) continue;
 
-    const resourcePaths = [toolPath.skills, toolPath.rules, toolPath.agents]
+    const resourcePaths = (field ? [toolPath[field]] : [toolPath.skills, toolPath.rules, toolPath.agents])
       .filter((resourcePath): resourcePath is string => !!resourcePath);
     for (const resourcePath of resourcePaths) {
       if (await isToolInstalledForConfig(tool, resourcePath, localConfig)) {
@@ -1205,13 +1188,16 @@ async function pullForScope(
         }
       }
     } else {
-      // Skills land in a tool's own directory, which a brand-new member may not
-      // have yet. The handler skips such a tool by design and only logs at
-      // debug, so counting the team repo's items here would report a success the
-      // disk contradicts (#585). Docs need no gate: they are copied to the
-      // team's own docs directory, which the copy creates.
-      const canReceive = type !== 'skills'
-        || await hasInstalledTargetFor(freshConfig, localConfig, 'skills');
+      // Skills and agents land in a tool's own directory, which a brand-new
+      // member may not have yet. The handler skips such a tool by design and
+      // only logs at debug, so counting the team repo's items here would report
+      // a success the disk contradicts (#585). Docs, rules and env are excluded
+      // from this branch entirely — they are written to team-owned locations
+      // that the copy creates. hooks/mcp have no tool-path field to probe, so
+      // they keep reporting unconditionally.
+      const needsToolRoot = type === 'skills' || type === 'agents';
+      const canReceive = !needsToolRoot
+        || (await getInstalledResourceTargets(freshConfig, localConfig, type)).length > 0;
 
       for (const item of items) {
         await handler.pullItem(item, freshConfig, localConfig);