Selaa lähdekoodia

fix(marketplace): simplify removal and model inheritance

Alvin Unreal 4 päivää sitten
vanhempi
sitoutus
bdf0ab529d

+ 3 - 0
README.md

@@ -140,6 +140,9 @@ selects the highest compatible version and enables it in the active preset,
 while `ID@version` selects exactly that version. Registry update requires an
 installed package and is strictly monotonic. `enable` activates an already
 installed agent package in the active preset as a separately named agent.
+`remove` first clears that package from every user and project preset activation
+list, then deletes the local package; no separate `disable` or `--force` step
+is needed. `disable` remains available when keeping the package installed.
 Registry installs try v3 first and fall back to v2 only when v3 is unavailable
 or lacks the requested package; malformed or integrity-invalid v3 data is not
 downgraded.

+ 3 - 0
docs/marketplace.md

@@ -39,6 +39,9 @@ Registry `update` requires an installed package and selects only a strictly
 newer compatible version. `enable` activates an already installed agent package
 in the active preset as a separately named agent. An agent may optionally extend one
 built-in specialist.
+`remove` clears the package from every user and project preset activation list
+before deleting it locally, so a separate `disable` command is unnecessary.
+The old unsafe `--force` removal path is not supported.
 
 `status` reports installed packages, configured activation, live-session
 agents when used from the in-session tool, diagnostics, and whether a

+ 25 - 1
src/agents/index.ts

@@ -52,6 +52,7 @@ import {
 } from './orchestrator';
 import {
   ROLE_DEFINITIONS,
+  type RoleDefinition,
   renderRoleRoutingBlock,
   SUPPORTED_SPECIALIST_ROLES,
 } from './role-definitions';
@@ -617,6 +618,27 @@ export function resolveAgentConfigModel(
   );
 }
 
+function resolveMarketplaceBuiltinModel(
+  runtime: RuntimeConfig,
+  role: RoleDefinition,
+): string | undefined {
+  const override = runtime.agent(role.id);
+  if (override?.model !== undefined) {
+    return resolvePrimaryModelValue(override.model);
+  }
+  if (override?.inheritModelFrom === 'session') {
+    return undefined;
+  }
+  if (override?.inheritModelFrom === 'orchestrator') {
+    return resolveAgentConfigModel(runtime, 'orchestrator');
+  }
+  return (
+    runtime.primaryModel ??
+    role.defaultModel ??
+    (DEFAULT_MODELS as Record<string, string | undefined>)[role.id]
+  );
+}
+
 /**
  * Apply model inheritance to the final host agent config after the host layer
  * has been merged. This clears stale host models for `session` inheritance,
@@ -1434,7 +1456,9 @@ export function createAgents(
             ? policy.candidates[0]
             : policy.candidates[0]?.id
           : policy.source === 'builtin'
-            ? role?.defaultModel
+            ? role
+              ? resolveMarketplaceBuiltinModel(runtime, role)
+              : undefined
             : policy.source === 'orchestrator'
               ? (configuredOrchestratorModel ?? primaryModel)
               : undefined;

+ 2 - 1
src/cli/index.ts

@@ -83,7 +83,8 @@ Usage:
   bunx oh-my-opencode-slim marketplace list
   bunx oh-my-opencode-slim marketplace show <package-id> [--json]
   bunx oh-my-opencode-slim marketplace verify [package-id] [--json]
-  bunx oh-my-opencode-slim marketplace remove <package-id> [--force]
+  bunx oh-my-opencode-slim marketplace remove <package-id>
+                            Deactivate and remove the package
   bunx oh-my-opencode-slim marketplace enable <package-id>
                            Enable an already installed package
   bunx oh-my-opencode-slim marketplace disable <package-id>

+ 0 - 6
src/cli/marketplace.test.ts

@@ -48,7 +48,6 @@ describe('marketplace CLI parsing', () => {
     expect(parseMarketplaceArgs(['import', './package.json'])).toEqual({
       command: 'import',
       value: './package.json',
-      force: false,
       json: false,
     });
     expect(
@@ -56,7 +55,6 @@ describe('marketplace CLI parsing', () => {
     ).toEqual({
       command: 'import',
       value: './package-v2.json',
-      force: false,
       json: false,
       update: true,
     });
@@ -65,7 +63,6 @@ describe('marketplace CLI parsing', () => {
     ).toEqual({
       command: 'install',
       value: 'community/example@1.2.3',
-      force: false,
       json: false,
     });
   });
@@ -76,7 +73,6 @@ describe('marketplace CLI parsing', () => {
     expect(parseMarketplaceArgs(['list'])).toEqual({
       command: 'list',
       value: undefined,
-      force: false,
       json: false,
     });
     expect(parseMarketplaceArgs(['update', 'community/example']).command).toBe(
@@ -91,13 +87,11 @@ describe('marketplace CLI parsing', () => {
     expect(parseMarketplaceArgs(['enable', 'community/example'])).toEqual({
       command: 'enable',
       value: 'community/example',
-      force: false,
       json: false,
     });
     expect(parseMarketplaceArgs(['status', '--json'])).toEqual({
       command: 'status',
       value: undefined,
-      force: false,
       json: true,
     });
   });

+ 6 - 19
src/cli/marketplace.ts

@@ -28,7 +28,6 @@ export type MarketplaceCommandName =
 export interface MarketplaceArgs {
   command: MarketplaceCommandName;
   value?: string;
-  force: boolean;
   json: boolean;
   update?: boolean;
 }
@@ -56,18 +55,15 @@ export function parseMarketplaceArgs(args: string[]): MarketplaceArgs {
     );
   }
   const command = rawCommand as MarketplaceCommandName;
-  const force = args.includes('--force');
   const json = args.includes('--json');
   const update = args.includes('--update');
   const options = args.filter((arg) => arg.startsWith('--'));
   const allowedOptions = new Set(
-    command === 'remove'
-      ? ['--force']
-      : command === 'verify' || command === 'show' || command === 'status'
-        ? ['--json']
-        : command === 'import'
-          ? ['--update']
-          : [],
+    command === 'verify' || command === 'show' || command === 'status'
+      ? ['--json']
+      : command === 'import'
+        ? ['--update']
+        : [],
   );
   for (const option of options) {
     if (!allowedOptions.has(option)) {
@@ -96,21 +92,12 @@ export function parseMarketplaceArgs(args: string[]): MarketplaceArgs {
   if (needsValue && !positional[0]) {
     throw new Error(`marketplace ${rawCommand} requires a value`);
   }
-  if (
-    (command === 'install' || command === 'update' || command === 'import') &&
-    force
-  ) {
-    throw new Error(
-      `Option --force is not valid for marketplace ${rawCommand}`,
-    );
-  }
   if (command !== 'import' && update) {
     throw new Error('Option --update is only valid for marketplace import');
   }
   return {
     command,
     value: positional[0],
-    force,
     json,
     ...(command === 'import' && update ? { update: true } : {}),
   };
@@ -187,7 +174,7 @@ export async function marketplaceCommand(
         return results.every((result) => result.valid) ? 0 : 1;
       }
       case 'remove':
-        service.remove(parsed.value as string, { force: parsed.force });
+        service.remove(parsed.value as string);
         console.log(mutationReloadNotice(`Removed ${parsed.value}`, 'unknown'));
         return 0;
       case 'enable':

+ 58 - 0
src/marketplace/agents-only.test.ts

@@ -140,6 +140,64 @@ describe('agents-only marketplace contract', () => {
     }
   });
 
+  test('v3 builtin model policies inherit the active built-in role model', () => {
+    const root = mkdtempSync(join(tmpdir(), 'marketplace-v3-model-'));
+    try {
+      const store = new MarketplaceStore({
+        rootDir: root,
+        pluginVersion: '3.0.0-beta.3',
+      });
+      store.install(
+        bundle({
+          schemaVersion: 3,
+          id: 'community/interface-critic',
+          agentName: 'interface-critic',
+          description: 'A derived designer.',
+          routing: {
+            lane: 'Interface critique',
+            stats: ['Visual review'],
+            delegateWhen: ['UI review is needed'],
+            avoid: ['Backend implementation'],
+          },
+          extends: { builtin: 'designer', promptMode: 'append' },
+          model: { source: 'builtin' },
+          skills: [],
+          mcps: [],
+          tools: ['read'],
+        }),
+      );
+      RuntimeConfig.reset(root);
+      const runtime = RuntimeConfig.init(root, {
+        preset: 'work',
+        presets: {
+          work: {
+            agents: {
+              orchestrator: { model: 'omniroute/orchestrator' },
+              designer: { model: 'omniroute/ag-farm' },
+              'interface-critic': {
+                variant: 'package-variant',
+                options: { reasoningEffort: 'high' },
+              },
+            },
+            marketplace: { agents: ['community/interface-critic'] },
+          },
+        },
+      });
+
+      const registry = buildResolvedAgentRegistry(runtime, {
+        marketplaceStore: store,
+        availableMcpNames: [],
+      });
+      expect(registry.sdkConfigs['interface-critic']).toMatchObject({
+        model: 'omniroute/ag-farm',
+        variant: 'package-variant',
+        options: { reasoningEffort: 'high' },
+      });
+    } finally {
+      rmSync(root, { recursive: true, force: true });
+    }
+  });
+
   test('owner model overrides replace package fallback chains', () => {
     const cases = [
       {

+ 56 - 57
src/marketplace/config-references.test.ts

@@ -1,12 +1,14 @@
 import { afterEach, describe, expect, test } from 'bun:test';
-import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs';
+import {
+  mkdirSync,
+  mkdtempSync,
+  readFileSync,
+  rmSync,
+  writeFileSync,
+} from 'node:fs';
 import { tmpdir } from 'node:os';
 import { join } from 'node:path';
-import {
-  MarketplaceActivationReferenceError,
-  MarketplaceService,
-  readMarketplaceConfigReferences,
-} from './index';
+import { MarketplaceService } from './index';
 import type { MarketplacePackageBundle } from './schemas';
 
 const previousConfigHome = process.env.XDG_CONFIG_HOME;
@@ -43,85 +45,82 @@ afterEach(() => {
   else process.env.XDG_CONFIG_HOME = previousConfigHome;
 });
 
-describe('marketplace activation references', () => {
-  test('reads user/project preset references instead of CLI claims', () => {
+describe('marketplace activation cleanup', () => {
+  test('removes every package activation across user and project presets', () => {
     const root = mkdtempSync(join(tmpdir(), 'marketplace-config-'));
     const configHome = join(root, 'config');
     const project = join(root, 'project');
+    const userConfigPath = join(
+      configHome,
+      'opencode',
+      'oh-my-opencode-slim.json',
+    );
+    const projectConfigPath = join(
+      project,
+      '.opencode',
+      'oh-my-opencode-slim.json',
+    );
     try {
       process.env.XDG_CONFIG_HOME = configHome;
+      mkdirSync(join(configHome, 'opencode'), { recursive: true });
       mkdirSync(join(project, '.opencode'), { recursive: true });
-      const configPath = join(project, '.opencode', 'oh-my-opencode-slim.json');
       writeFileSync(
-        configPath,
+        userConfigPath,
         JSON.stringify({
           preset: 'work',
+          unrelated: { keep: true },
           presets: {
             work: {
+              agents: {},
+              marketplace: {
+                agents: ['community/referenced', 'community/user-keep'],
+              },
+            },
+            unused: {
               agents: {},
               marketplace: { agents: ['community/referenced'] },
             },
           },
         }),
       );
-      expect(readMarketplaceConfigReferences(project)).toEqual([
-        {
-          packageId: 'community/referenced',
-          configPath,
-          presetName: 'work',
-          target: 'agent',
-        },
-      ]);
-
-      const service = new MarketplaceService({
-        rootDir: join(root, 'store'),
-        projectDir: project,
-      });
-      service.install(bundle);
-      expect(() => service.remove('community/referenced')).toThrow(
-        MarketplaceActivationReferenceError,
-      );
-      expect(() =>
-        service.remove('community/referenced', { force: true }),
-      ).not.toThrow();
-    } finally {
-      rmSync(root, { recursive: true, force: true });
-    }
-  });
-
-  test('resolves a project-selected preset defined in user config', () => {
-    const root = mkdtempSync(join(tmpdir(), 'marketplace-config-'));
-    const configHome = join(root, 'config');
-    const project = join(root, 'project');
-    const userConfigPath = join(configHome, 'opencode');
-    try {
-      process.env.XDG_CONFIG_HOME = configHome;
-      mkdirSync(userConfigPath, { recursive: true });
-      mkdirSync(join(project, '.opencode'), { recursive: true });
       writeFileSync(
-        join(userConfigPath, 'oh-my-opencode-slim.json'),
+        projectConfigPath,
         JSON.stringify({
+          preset: 'work',
           presets: {
-            shared: {
+            work: {
+              agents: {},
+              marketplace: {
+                agents: ['community/referenced', 'community/project-keep'],
+              },
+            },
+            unused: {
               agents: {},
               marketplace: { agents: ['community/referenced'] },
             },
           },
         }),
       );
-      writeFileSync(
-        join(project, '.opencode', 'oh-my-opencode-slim.json'),
-        JSON.stringify({ preset: 'shared' }),
-      );
 
-      expect(readMarketplaceConfigReferences(project)).toEqual([
-        {
-          packageId: 'community/referenced',
-          configPath: join(project, '.opencode', 'oh-my-opencode-slim.json'),
-          presetName: 'shared',
-          target: 'agent',
-        },
+      const service = new MarketplaceService({
+        rootDir: join(root, 'store'),
+        projectDir: project,
+      });
+      service.install(bundle);
+      service.remove('community/referenced');
+
+      expect(service.list()).toEqual([]);
+      const userConfig = JSON.parse(readFileSync(userConfigPath, 'utf8'));
+      const projectConfig = JSON.parse(readFileSync(projectConfigPath, 'utf8'));
+      expect(userConfig.unrelated).toEqual({ keep: true });
+      expect(userConfig.presets.work.marketplace.agents).toEqual([
+        'community/user-keep',
+      ]);
+      expect(userConfig.presets.unused.marketplace.agents).toEqual([]);
+      expect(projectConfig.presets.work.marketplace.agents).toEqual([
+        'community/project-keep',
       ]);
+      expect(projectConfig.presets.unused.marketplace.agents).toEqual([]);
     } finally {
       rmSync(root, { recursive: true, force: true });
     }

+ 38 - 48
src/marketplace/config-references.ts

@@ -1,11 +1,5 @@
-import { findPluginConfigPaths, loadPluginConfig } from '../config/loader';
-
-export interface MarketplaceConfigReference {
-  packageId: string;
-  configPath: string;
-  presetName: string;
-  target: 'agent';
-}
+import { mutateJsonFile } from '../cli/config-io';
+import { findPluginConfigPaths } from '../config/loader';
 
 interface UnknownRecord {
   [key: string]: unknown;
@@ -15,50 +9,46 @@ function isRecord(value: unknown): value is UnknownRecord {
   return typeof value === 'object' && value !== null && !Array.isArray(value);
 }
 
-function addReference(
-  references: MarketplaceConfigReference[],
-  value: unknown,
-  configPath: string,
-  presetName: string,
-  target: 'agent',
-): void {
-  if (typeof value !== 'string' || value.trim().length === 0) return;
-  references.push({
-    packageId: value.trim().toLowerCase(),
-    configPath,
-    presetName,
-    target,
-  });
-}
+function removeFromConfig(config: unknown, packageId: string): unknown {
+  if (!isRecord(config) || !isRecord(config.presets)) return config;
 
-function readReferencesFromMergedConfig(
-  config: unknown,
-  configPath: string,
-): MarketplaceConfigReference[] {
-  if (!isRecord(config)) return [];
-  const presetName = config.preset;
-  if (typeof presetName !== 'string') return [];
-  const presets = config.presets;
-  if (!isRecord(presets)) return [];
-
-  const references: MarketplaceConfigReference[] = [];
-  const presetValue = presets[presetName];
-  if (!isRecord(presetValue) || !isRecord(presetValue.marketplace)) return [];
-  const marketplace = presetValue.marketplace;
-  if (Array.isArray(marketplace.agents)) {
-    for (const packageId of marketplace.agents) {
-      addReference(references, packageId, configPath, presetName, 'agent');
-    }
+  let changed = false;
+  const presets = { ...config.presets };
+  for (const [presetName, presetValue] of Object.entries(presets)) {
+    if (!isRecord(presetValue) || !isRecord(presetValue.marketplace)) continue;
+    const agents = presetValue.marketplace.agents;
+    if (!Array.isArray(agents)) continue;
+    const filtered = agents.filter(
+      (value) =>
+        typeof value !== 'string' || value.trim().toLowerCase() !== packageId,
+    );
+    if (filtered.length === agents.length) continue;
+    changed = true;
+    presets[presetName] = {
+      ...presetValue,
+      marketplace: {
+        ...presetValue.marketplace,
+        agents: filtered,
+      },
+    };
   }
-  return references;
+  return changed ? { ...config, presets } : config;
 }
 
-/** Read persisted references from both user and project plugin configs. */
-export function readMarketplaceConfigReferences(
+/** Remove a package from every user and project preset activation list. */
+export function removeMarketplaceConfigReferences(
   directory: string,
-): MarketplaceConfigReference[] {
+  packageId: string,
+): void {
   const paths = findPluginConfigPaths(directory);
-  const configPath = paths.projectConfigPath ?? paths.userConfigPath;
-  const config = loadPluginConfig(directory, { silent: true });
-  return readReferencesFromMergedConfig(config, configPath ?? directory);
+  const normalizedId = packageId.trim().toLowerCase();
+  for (const configPath of new Set(
+    [paths.userConfigPath, paths.projectConfigPath].filter(
+      (value): value is string => value !== null,
+    ),
+  )) {
+    mutateJsonFile(configPath, (config) =>
+      removeFromConfig(config, normalizedId),
+    );
+  }
 }

+ 0 - 7
src/marketplace/errors.ts

@@ -64,13 +64,6 @@ export class MarketplaceRetiredError extends MarketplaceError {
   }
 }
 
-export class MarketplaceActivationReferenceError extends MarketplaceError {
-  constructor(message: string) {
-    super(message, 'activation-reference');
-    this.name = 'MarketplaceActivationReferenceError';
-  }
-}
-
 export class MarketplaceActivationError extends MarketplaceError {
   constructor(message: string) {
     super(message, 'activation');

+ 4 - 40
src/marketplace/service.ts

@@ -2,12 +2,8 @@ import { readFileSync, realpathSync } from 'node:fs';
 import { DEFAULT_MARKETPLACE_REGISTRY_URL } from '../marketplace-contract';
 import { readPluginPackageVersion } from '../utils/package-metadata';
 import type { MarketplaceCompatibilityOptions } from './compatibility';
+import { removeMarketplaceConfigReferences } from './config-references';
 import {
-  type MarketplaceConfigReference,
-  readMarketplaceConfigReferences,
-} from './config-references';
-import {
-  MarketplaceActivationReferenceError,
   MarketplaceConflictError,
   MarketplaceRegistryNotFoundError,
   MarketplaceRegistryUnavailableError,
@@ -43,10 +39,6 @@ export type MarketplaceRegistryDownloadClient = Pick<
 > &
   Partial<Pick<MarketplaceRegistryClient, 'downloadV3'>>;
 
-export interface MarketplaceRemoveOptions {
-  force?: boolean;
-}
-
 function parseBundle(value: unknown): MarketplacePackageBundle {
   const candidate =
     typeof value === 'object' && value !== null && 'manifest' in value
@@ -65,19 +57,6 @@ function sourceForImport(filePath: string): MarketplaceSource {
   return { kind: 'local', path: realpathSync(filePath) };
 }
 
-function referenceMessage(
-  packageId: string,
-  references: MarketplaceConfigReference[],
-): string {
-  const locations = references
-    .filter((reference) => reference.packageId === packageId)
-    .map(
-      (reference) =>
-        `${reference.configPath} (preset ${reference.presetName}, ${reference.target})`,
-    );
-  return `Cannot remove referenced package ${packageId}: ${locations.join('; ')}. Deactivate it first or use --force.`;
-}
-
 export class MarketplaceService {
   readonly store: MarketplaceStore;
   readonly projectDir: string;
@@ -225,25 +204,10 @@ export class MarketplaceService {
     return this.store.verifyAll();
   }
 
-  remove(id: string, options: MarketplaceRemoveOptions = {}): void {
+  remove(id: string): void {
     const normalizedId = normalizeMarketplacePackageId(id);
-    this.store.remove(
-      normalizedId,
-      options.force
-        ? undefined
-        : () => {
-            const references = readMarketplaceConfigReferences(this.projectDir);
-            if (
-              references.some(
-                (reference) => reference.packageId === normalizedId,
-              )
-            ) {
-              throw new MarketplaceActivationReferenceError(
-                referenceMessage(normalizedId, references),
-              );
-            }
-          },
-    );
+    removeMarketplaceConfigReferences(this.projectDir, normalizedId);
+    this.store.remove(normalizedId);
   }
 }
 

+ 1 - 2
src/tools/marketplace.test.ts

@@ -277,7 +277,7 @@ describe('marketplace tool', () => {
     }
   });
 
-  test('reports reload required after force-removing a live package', async () => {
+  test('removes a live package through the safe lifecycle path', async () => {
     const { root, project, service, marketplace, context, live } =
       setupHarness();
     try {
@@ -301,7 +301,6 @@ describe('marketplace tool', () => {
           {
             action: 'remove',
             packageId: 'community/docs-researcher',
-            force: true,
           },
           context,
         ),

+ 2 - 10
src/tools/marketplace.ts

@@ -61,11 +61,7 @@ const MarketplaceToolRequestSchema = z.discriminatedUnion('action', [
     })
     .strict(),
   z
-    .object({
-      action: z.literal('remove'),
-      packageId: z.string().min(1),
-      force: z.boolean().optional(),
-    })
+    .object({ action: z.literal('remove'), packageId: z.string().min(1) })
     .strict(),
   z.object({ action: z.literal('list') }).strict(),
   z.object({ action: z.literal('status') }).strict(),
@@ -166,10 +162,6 @@ Action-specific fields: packageId for install/update/show/enable/disable/remove;
         .describe(
           'Canonical package ID for registry install/update or local actions',
         ),
-      force: toolZ
-        .boolean()
-        .optional()
-        .describe('Force remove even if the package is still referenced'),
       update: toolZ
         .boolean()
         .optional()
@@ -254,7 +246,7 @@ Action-specific fields: packageId for install/update/show/enable/disable/remove;
             .join('\n');
         }
         case 'remove':
-          service.remove(request.packageId, { force: request.force });
+          service.remove(request.packageId);
           return mutationResult(options, `Removed ${request.packageId}`);
         case 'enable':
           enableMarketplaceAgent(projectDir, request.packageId, service.store);