fix: sync global Native Skills during update (#266)

* fix: sync global Native Skills during update

Global scope used to hardcode the Skill workflowSelection to 'classic',
which filtered out comet-native/* and left global Native installs frozen
at their first-install version. Mirror comet init by selecting 'both'
for global Skills so Native files stay in sync. Rules and hooks keep the
Classic selection (their workflowSelection is effectively ignored and
Native write-guarding is routed through comet-hook-router).

Closes #262

* refactor: address review feedback on global Native Skill update

- Extract skillWorkflowSelectionFor helper so the global-vs-project
  Skill selection lives in one place instead of two duplicated ternaries.
- Seed a stale comet-native Skill in the regression test and assert it
  is overwritten, proving existing installs are refreshed, not just
  created.
- Document why the test silences console.log.

Follow-up to #262.

* fix: derive global Skill selection from disk and count filtered Skills

Address maintainer feedback on #262.

Selection by installed workflow:
- skillWorkflowSelectionFor now checks whether comet-native/SKILL.md
  exists on disk for global targets instead of always returning 'both'.
  An existing Native install selects 'both' so it stays current; a
  Classic-only global install keeps 'classic' so update never adds
  Native Skills the user did not choose. Project scope still honors the
  project workflow configuration.

Honest skipped counts:
- copyCometSkillsForPlatform and installSkillsAsSymlink now add the
  number of manifest entries filtered out by the workflow selection to
  the skipped total, so a Classic-only update reports the Native files
  it intentionally skips instead of hiding them.

Tests:
- The Native-sync regression test now asserts zero skipped Skills for a
  Native install (both selection copies everything).
- New test verifies a Classic-only global install stays Native-free
  after update and reports the filtered Native entries as skipped.

* fix: derive all three global Skill selections from disk

Check both comet-native/SKILL.md and comet-classic/SKILL.md so a
Native-only global install (comet init --scope global --workflow native)
selects 'native' and update does not add Classic Skills, mirroring the
Classic-only protection already in place. neither/both markers map to
classic/both respectively.

Tests cover all three derivations:
- both markers -> 'both' (skipped 0, stale Native refreshed)
- classic only -> 'classic' (Native not added, skipped > 0)
- native only -> 'native' (Classic not added, skipped > 0)

* refactor: move workflow marker detection into skill domain

Extract detectInstalledWorkflowSelection into
domains/skill/platform-install.ts so the marker-to-selection mapping
(comet-native/SKILL.md + comet-classic/SKILL.md -> classic|native|both)
lives with the rest of the Skill install domain logic instead of the
command orchestration layer. update.ts now computes the skills root and
delegates the detection, keeping the global/project behavior unchanged.

Follow-up to architecture feedback on #262.

---------

Co-authored-by: zhangshuo12 <zhangshuo12@guazi.com>

Author: hottaiger

app/commands/update.ts

@@ -15,6 +15,7 @@ import {
 import {
   copyCometSkillsForPlatform,
   copyCometRulesForPlatform,
+  detectInstalledWorkflowSelection,
   installCometHooksForPlatform,
   getManifestSkills,
   mergeProjectConfig,
@@ -1542,9 +1543,24 @@ async function updateSingleProject(
     );
   }
 
-  const targetWorkflowSelections = targets.map((target) =>
-    target.scope === 'global' ? 'classic' : projectWorkflowSelection,
-  );
+  // Global scope mirrors `comet init`: `comet update --scope global` must keep
+  // already-installed Skills in sync without expanding the workflow range the
+  // user chose at install time. The selection is derived from what is on disk
+  // by checking the two workflow markers (see detectInstalledWorkflowSelection
+  // in domains/skill/platform-install.ts). Project scope keeps honoring the
+  // project workflow configuration.
+  const skillWorkflowSelectionFor = async (
+    target: InstalledCometTarget,
+  ): Promise<InitWorkflowSelection> => {
+    if (target.scope !== 'global') return projectWorkflowSelection;
+    const skillsRoot = path.join(
+      getBaseDir('global', projectPath),
+      getPlatformSkillsDir(target.platform, 'global'),
+      'skills',
+    );
+    return detectInstalledWorkflowSelection(skillsRoot);
+  };
+  const targetWorkflowSelections = await Promise.all(targets.map(skillWorkflowSelectionFor));
   const updateSkillPaths = new Set(
     (
       await Promise.all(
@@ -1571,8 +1587,7 @@ async function updateSingleProject(
     const languageSkillsDir = languageToSkillsDir(languageId);
     const targetInstallMode = installModeFor(target);
     const nativeProjectTarget = nativeProject && target.scope === 'project';
-    const targetWorkflowSelection =
-      target.scope === 'global' ? 'classic' : projectWorkflowSelection;
+    const targetSkillWorkflowSelection = await skillWorkflowSelectionFor(target);
     if (target.scope === 'project') {
       await assertClassicProjectMutationAllowed?.();
     }
@@ -1581,7 +1596,7 @@ async function updateSingleProject(
         baseDir,
         target.platform,
         target.scope,
-        targetWorkflowSelection,
+        targetSkillWorkflowSelection,
       );
     }
     const { copied, skipped, failed } = await copyCometSkillsForPlatform(
@@ -1591,7 +1606,7 @@ async function updateSingleProject(
       languageSkillsDir,
       target.scope,
       targetInstallMode,
-      targetWorkflowSelection,
+      targetSkillWorkflowSelection,
     );
     const cleanupResult =
       failed === 0

domains/skill/platform-install.ts

@@ -115,6 +115,30 @@ function isManagedSkillPathForSelection(
   );
 }
 
+/**
+ * Derive the workflow selection from the Skills already on disk by checking
+ * the two workflow markers (comet-native/SKILL.md and comet-classic/SKILL.md).
+ * This lets `comet update` keep already-installed workflows in sync without
+ * expanding the range the user chose at install time:
+ *   neither / classic only -> 'classic'  (no Native added)
+ *   native only            -> 'native'   (no Classic added)
+ *   both                   -> 'both'
+ * The caller is responsible for computing `skillsRoot` (e.g. base dir +
+ * platform skills dir + 'skills'); this function only performs the marker
+ * check and mapping.
+ */
+export async function detectInstalledWorkflowSelection(
+  skillsRoot: string,
+): Promise<InitWorkflowSelection> {
+  const [hasNative, hasClassic] = await Promise.all([
+    fileExists(path.join(skillsRoot, 'comet-native', 'SKILL.md')),
+    fileExists(path.join(skillsRoot, 'comet-classic', 'SKILL.md')),
+  ]);
+  if (hasNative && hasClassic) return 'both';
+  if (hasNative) return 'native';
+  return 'classic';
+}
+
 function getManagedSkillPathsForSelection(
   manifest: Manifest,
   workflowSelection: InitWorkflowSelection,
@@ -458,6 +482,9 @@ async function installSkillsAsSymlink(
   let copied = 0;
   let skippedCount = 0;
   let failedCount = 0;
+  // Count manifest entries filtered out by the workflow selection so the
+  // symlink path reports skipped files consistently with copy mode.
+  skippedCount += getManagedSkillPaths(manifest).length - managedSkillPaths.length;
 
   for (const skillRelPath of managedSkillPaths) {
     const isScript = skillRelPath.includes('/scripts/');
@@ -566,6 +593,11 @@ async function copyCometSkillsForPlatform(
   let failedCount = 0;
   const managedSkillPaths = getManagedSkillPathsForSelection(manifest, workflowSelection);
   const userFacingSkillPaths = getUserFacingSkillPathsForSelection(manifest, workflowSelection);
+  // Count manifest entries that the workflow selection filters out so the
+  // update summary stays honest: a Classic-only update reports the Native
+  // files it intentionally skips instead of pretending they do not exist.
+  const filteredCount = getManagedSkillPaths(manifest).length - managedSkillPaths.length;
+  skippedCount += filteredCount;
 
   for (const skillRelPath of managedSkillPaths) {
     const isScript = skillRelPath.includes('/scripts/');

test/app/update.test.ts

@@ -3322,6 +3322,176 @@ describe('update command helpers', () => {
     await expect(fs.stat(path.join(fakeHome, 'docs', 'superpowers'))).rejects.toThrow();
   });
 
+  it('syncs Native Skills during a global update of a Native+Classic install', async () => {
+    // Regression for #262: global scope used to hardcode the Skill
+    // workflowSelection to 'classic', which filtered out comet-native/* and
+    // left global Native installs frozen at their first-install version. With
+    // both comet-native and comet-classic on disk the selection is 'both', so
+    // Native Skills stay current.
+    const fakeHome = path.join(tmpDir, 'fake-home-global-native-sync');
+    await fs.mkdir(path.join(fakeHome, '.codex', 'skills', 'comet'), { recursive: true });
+    await fs.writeFile(
+      path.join(fakeHome, '.codex', 'skills', 'comet', 'SKILL.md'),
+      '# Comet\n',
+      'utf-8',
+    );
+    // Seed a both-workflow install: stale comet-native plus comet-classic so
+    // the derived selection is 'both', and prove the stale Native Skill is
+    // overwritten rather than merely created.
+    await fs.mkdir(path.join(fakeHome, '.agents', 'skills', 'comet-native'), {
+      recursive: true,
+    });
+    await fs.writeFile(
+      path.join(fakeHome, '.agents', 'skills', 'comet-native', 'SKILL.md'),
+      '---\nname: comet-native\n---\n# STALE\n',
+      'utf-8',
+    );
+    await fs.mkdir(path.join(fakeHome, '.agents', 'skills', 'comet-classic'), {
+      recursive: true,
+    });
+    await fs.writeFile(
+      path.join(fakeHome, '.agents', 'skills', 'comet-classic', 'SKILL.md'),
+      '# stale classic\n',
+      'utf-8',
+    );
+    const homeSpy = vi.spyOn(os, 'homedir').mockReturnValue(fakeHome);
+    // Silence update progress logging; this test only asserts file contents.
+    const log = vi.spyOn(console, 'log').mockImplementation(() => undefined);
+    let json: string | undefined;
+
+    try {
+      await updateCommand(tmpDir, {
+        json: true,
+        skipNpm: true,
+        scope: 'global',
+      });
+      json = log.mock.calls.map((call) => call.join(' ')).join('\n');
+    } finally {
+      log.mockRestore();
+      homeSpy.mockRestore();
+    }
+
+    // Native Skills must be copied alongside Classic ones for global installs.
+    // Codex installs land in the canonical `.agents/skills/` directory, and a
+    // previously stale Native Skill must be replaced with the current asset.
+    const nativeSkill = await fs.readFile(
+      path.join(fakeHome, '.agents', 'skills', 'comet-native', 'SKILL.md'),
+      'utf8',
+    );
+    expect(nativeSkill).toContain('name: comet-native');
+    expect(nativeSkill).not.toContain('# STALE');
+    await expect(
+      fs.readFile(path.join(fakeHome, '.agents', 'skills', 'comet', 'SKILL.md'), 'utf8'),
+    ).resolves.toContain('comet workflow resolve');
+
+    // With 'both' selected, no manifest entries are filtered out, so the
+    // Codex target reports zero skipped Skills.
+    const result = json ? JSON.parse(json) : {};
+    const codexSkills = (result.skills?.targets ?? []).find(
+      (t: { platform: string }) => t.platform === 'codex',
+    );
+    expect(codexSkills?.skipped).toBe(0);
+  });
+
+  it('does not add Native Skills to a Classic-only global install during update', async () => {
+    // A global install created with `comet init --scope global --workflow
+    // classic` must not gain comet-native after `comet update --scope global`.
+    const fakeHome = path.join(tmpDir, 'fake-home-global-classic-only');
+    await fs.mkdir(path.join(fakeHome, '.codex', 'skills', 'comet'), { recursive: true });
+    await fs.writeFile(
+      path.join(fakeHome, '.codex', 'skills', 'comet', 'SKILL.md'),
+      '# Comet\n',
+      'utf-8',
+    );
+    // No comet-native directory: this is a Classic-only install.
+    const homeSpy = vi.spyOn(os, 'homedir').mockReturnValue(fakeHome);
+    const log = vi.spyOn(console, 'log').mockImplementation(() => undefined);
+    let json: string | undefined;
+
+    try {
+      await updateCommand(tmpDir, {
+        json: true,
+        skipNpm: true,
+        scope: 'global',
+      });
+      json = log.mock.calls.map((call) => call.join(' ')).join('\n');
+    } finally {
+      log.mockRestore();
+      homeSpy.mockRestore();
+    }
+
+    // Classic Skills update, but comet-native must NOT be introduced.
+    await expect(
+      fs.readFile(path.join(fakeHome, '.agents', 'skills', 'comet', 'SKILL.md'), 'utf8'),
+    ).resolves.toContain('comet workflow resolve');
+    await expect(
+      fs.access(path.join(fakeHome, '.agents', 'skills', 'comet-native', 'SKILL.md')),
+    ).rejects.toMatchObject({ code: 'ENOENT' });
+
+    // The Native manifest entries filtered out by the Classic selection are
+    // reported as skipped rather than hidden.
+    const result = json ? JSON.parse(json) : {};
+    const codexSkills = (result.skills?.targets ?? []).find(
+      (t: { platform: string }) => t.platform === 'codex',
+    );
+    expect(codexSkills?.skipped).toBeGreaterThan(0);
+  });
+
+  it('does not add Classic Skills to a Native-only global install during update', async () => {
+    // A global install created with `comet init --scope global --workflow
+    // native` must not gain comet-classic after `comet update --scope global`.
+    const fakeHome = path.join(tmpDir, 'fake-home-global-native-only');
+    await fs.mkdir(path.join(fakeHome, '.codex', 'skills', 'comet'), { recursive: true });
+    await fs.writeFile(
+      path.join(fakeHome, '.codex', 'skills', 'comet', 'SKILL.md'),
+      '# Comet\n',
+      'utf-8',
+    );
+    // Native-only install: comet-native present, comet-classic absent.
+    await fs.mkdir(path.join(fakeHome, '.agents', 'skills', 'comet-native'), {
+      recursive: true,
+    });
+    await fs.writeFile(
+      path.join(fakeHome, '.agents', 'skills', 'comet-native', 'SKILL.md'),
+      '---\nname: comet-native\n---\n# STALE\n',
+      'utf-8',
+    );
+    const homeSpy = vi.spyOn(os, 'homedir').mockReturnValue(fakeHome);
+    const log = vi.spyOn(console, 'log').mockImplementation(() => undefined);
+    let json: string | undefined;
+
+    try {
+      await updateCommand(tmpDir, {
+        json: true,
+        skipNpm: true,
+        scope: 'global',
+      });
+      json = log.mock.calls.map((call) => call.join(' ')).join('\n');
+    } finally {
+      log.mockRestore();
+      homeSpy.mockRestore();
+    }
+
+    // Native Skills update, but comet-classic must NOT be introduced.
+    const nativeSkill = await fs.readFile(
+      path.join(fakeHome, '.agents', 'skills', 'comet-native', 'SKILL.md'),
+      'utf8',
+    );
+    expect(nativeSkill).toContain('name: comet-native');
+    expect(nativeSkill).not.toContain('# STALE');
+    await expect(
+      fs.access(path.join(fakeHome, '.agents', 'skills', 'comet-classic', 'SKILL.md')),
+    ).rejects.toMatchObject({ code: 'ENOENT' });
+
+    // The Classic manifest entries filtered out by the Native selection are
+    // reported as skipped rather than hidden.
+    const result = json ? JSON.parse(json) : {};
+    const codexSkills = (result.skills?.targets ?? []).find(
+      (t: { platform: string }) => t.platform === 'codex',
+    );
+    expect(codexSkills?.skipped).toBeGreaterThan(0);
+  });
+
   it('preserves installed language for an explicit global platform update', async () => {
     const fakeHome = path.join(tmpDir, 'fake-home-explicit-global-language');
     await fs.mkdir(path.join(fakeHome, '.codex', 'skills', 'comet'), { recursive: true });