fix(plugins): correct install progress bounds, skill identity, and stage accuracy

Addresses review feedback on the marketplace installed-state change. - Progress: byte-derived progress no longer has time-based drift layered on top, and fallback drift is clamped to the current stage range, so the bar cannot exceed the download band. 90/100 bytes at 40s elapsed used to report 61% against a declared 5-45% band; it now reports 41%. Drift is measured from when the stage was entered rather than from task start. - Skills: a marketplace skill is no longer reported installed from a bare skill name. The backend names skills from their own SKILL.md and records no publisher, so alice/review and bob/review install identically; matching the bare name marked every publisher's skill as installed. Skill cards now resolve to not-installed until the installed skill carries a publisher. - Stages: "installing plugin dependencies" and "launching plugin" described work this task context cannot observe (installation persistence ran under the former; the runtime installs dependencies and starts the plugin inside apply_plugin_installation under the latter). They become "persisting the installation" and "installing or starting plugin", and the frontend maps that combined step to the dependency stage rather than the launch stage. - Removed the per-dependency progress fields: the backend never populated them, so the UI could never have displayed them. The stage mapping and progress maths move to install-progress.ts, and the installed-state matching to a React-free marketplace-installed.ts, so both are covered by executable tests (+9). Verified: ruff, tsc --noEmit, prettier --check, eslint (0 errors), 98/98 unit tests.
2026-09-23 01:46:37 +08:00 · 2026-09-21 00:40:23 +08:00
parent f998e475e3
commit 163dac48b0
11 changed files with 494 additions and 249 deletions
@@ -0,0 +1,133 @@
+import assert from 'node:assert/strict';
+import fs from 'node:fs';
+import path from 'node:path';
+import test from 'node:test';
+import { createRequire } from 'node:module';
+import { fileURLToPath } from 'node:url';
+import ts from 'typescript';
+
+const currentDirectory = path.dirname(fileURLToPath(import.meta.url));
+const sourcePath = path.resolve(
+  currentDirectory,
+  '../../src/app/home/plugins/components/plugin-install-task/install-progress.ts',
+);
+const source = fs.readFileSync(sourcePath, 'utf8');
+const compiled = ts.transpileModule(source, {
+  compilerOptions: { module: ts.ModuleKind.CommonJS },
+}).outputText;
+const sourceRequire = createRequire(sourcePath);
+const loadedModule = { exports: {} };
+new Function('require', 'module', 'exports', compiled)(
+  sourceRequire,
+  loadedModule,
+  loadedModule.exports,
+);
+
+const {
+  InstallStage,
+  INSTALL_PROGRESS_CAP,
+  STAGE_PROGRESS_RANGE,
+  computeStageProgress,
+  mapActionToStage,
+} = loadedModule.exports;
+
+test('maps the connector stage strings the runtime actually emits', () => {
+  assert.equal(
+    mapActionToStage('preparing plugin install'),
+    InstallStage.DOWNLOADING,
+    '"preparing plugin install" must not be read as the dependency stage',
+  );
+  assert.equal(
+    mapActionToStage('downloading plugin package'),
+    InstallStage.DOWNLOADING,
+  );
+  assert.equal(
+    mapActionToStage('inspecting plugin package'),
+    InstallStage.INSTALLING_DEPS,
+  );
+  assert.equal(
+    mapActionToStage('storing plugin package'),
+    InstallStage.INSTALLING_DEPS,
+  );
+  assert.equal(
+    mapActionToStage('persisting the installation'),
+    InstallStage.INSTALLING_DEPS,
+  );
+  assert.equal(mapActionToStage('launching plugin'), InstallStage.LAUNCHING);
+  assert.equal(
+    mapActionToStage('waiting for plugin to become ready'),
+    InstallStage.LAUNCHING,
+    'the readiness wait is still an active stage, not completion',
+  );
+});
+
+test('the combined install-and-start stage is not reported as launching', () => {
+  // The runtime installs dependencies and starts the plugin in one step; the
+  // wording contains "starting" and must not be mapped to the launch stage.
+  assert.equal(
+    mapActionToStage('installing or starting plugin'),
+    InstallStage.INSTALLING_DEPS,
+  );
+});
+
+test('measured byte counts stay inside the download stage range', () => {
+  const [start, end] = STAGE_PROGRESS_RANGE[InstallStage.DOWNLOADING];
+
+  // 90 of 100 bytes is 90% of the download range, even after 40s elapsed.
+  const progress = computeStageProgress({
+    stage: InstallStage.DOWNLOADING,
+    downloadCurrent: 90,
+    downloadTotal: 100,
+    stageElapsedSeconds: 40,
+  });
+
+  assert.ok(
+    progress >= start && progress <= end,
+    `expected progress within [${start}, ${end}], received ${progress}`,
+  );
+  assert.equal(
+    progress,
+    41,
+    'drift must not be layered on top of a measured byte ratio',
+  );
+});
+
+test('fallback drift never spills into the next stage range', () => {
+  for (const stage of [
+    InstallStage.DOWNLOADING,
+    InstallStage.INSTALLING_DEPS,
+    InstallStage.INITIALIZING,
+    InstallStage.LAUNCHING,
+  ]) {
+    const [start, end] = STAGE_PROGRESS_RANGE[stage];
+    const progress = computeStageProgress({
+      stage,
+      stageElapsedSeconds: 100000,
+    });
+
+    assert.ok(
+      progress >= start && progress <= end,
+      `stage ${stage} produced ${progress}, outside [${start}, ${end}]`,
+    );
+    assert.ok(
+      progress < INSTALL_PROGRESS_CAP,
+      `stage ${stage} must not reach the completion cap on drift alone`,
+    );
+  }
+});
+
+test('a missing or zero download total falls back to bounded drift', () => {
+  const [start, end] = STAGE_PROGRESS_RANGE[InstallStage.DOWNLOADING];
+
+  const progress = computeStageProgress({
+    stage: InstallStage.DOWNLOADING,
+    downloadCurrent: 10,
+    downloadTotal: 0,
+    stageElapsedSeconds: 100000,
+  });
+
+  assert.ok(
+    progress >= start && progress <= end,
+    `expected progress within [${start}, ${end}], received ${progress}`,
+  );
+});
@@ -0,0 +1,121 @@
+import assert from 'node:assert/strict';
+import fs from 'node:fs';
+import path from 'node:path';
+import test from 'node:test';
+import { createRequire } from 'node:module';
+import { fileURLToPath } from 'node:url';
+import ts from 'typescript';
+
+const currentDirectory = path.dirname(fileURLToPath(import.meta.url));
+const sourcePath = path.resolve(
+  currentDirectory,
+  '../../src/app/home/plugins/components/plugin-market/marketplace-installed.ts',
+);
+const source = fs.readFileSync(sourcePath, 'utf8');
+const compiled = ts.transpileModule(source, {
+  compilerOptions: { module: ts.ModuleKind.CommonJS },
+}).outputText;
+const sourceRequire = createRequire(sourcePath);
+const loadedModule = { exports: {} };
+new Function('require', 'module', 'exports', compiled)(
+  sourceRequire,
+  loadedModule,
+  loadedModule.exports,
+);
+
+const { buildInstalledIndex, resolveInstalledState, installedExtensionKey } =
+  loadedModule.exports;
+
+test('matches installed plugins by author and name', () => {
+  const index = buildInstalledIndex(
+    [{ id: 'alice/review', hasUpdate: true }],
+    [],
+    [],
+  );
+
+  assert.deepEqual(
+    resolveInstalledState(index, {
+      type: 'plugin',
+      author: 'alice',
+      pluginName: 'review',
+    }),
+    { installed: true, hasUpdate: true },
+  );
+  assert.deepEqual(
+    resolveInstalledState(index, {
+      type: 'plugin',
+      author: 'bob',
+      pluginName: 'review',
+    }),
+    { installed: false, hasUpdate: false },
+    'a different publisher must not match',
+  );
+});
+
+test('normalises MCP servers from `author__name` to `author/name`', () => {
+  const index = buildInstalledIndex([], [{ id: 'acme__search' }], []);
+
+  assert.equal(
+    resolveInstalledState(index, {
+      type: 'mcp',
+      author: 'acme',
+      pluginName: 'search',
+    }).installed,
+    true,
+  );
+  assert.equal(
+    resolveInstalledState(index, {
+      type: 'mcp',
+      author: 'other',
+      pluginName: 'search',
+    }).installed,
+    false,
+  );
+});
+
+test('does not mark skills installed from a bare name', () => {
+  // Two publishers ship a skill that both install as the plain name
+  // `review`; the sidebar records no publisher for either.
+  const index = buildInstalledIndex([], [], [{ id: 'review' }]);
+
+  const alice = resolveInstalledState(index, {
+    type: 'skill',
+    author: 'alice',
+    pluginName: 'review',
+  });
+  const bob = resolveInstalledState(index, {
+    type: 'skill',
+    author: 'bob',
+    pluginName: 'review',
+  });
+
+  assert.equal(
+    alice.installed,
+    false,
+    'alice/review must not be reported installed from a bare skill name',
+  );
+  assert.equal(
+    bob.installed,
+    false,
+    'bob/review must not be reported installed from a bare skill name',
+  );
+});
+
+test('keeps extension kinds separate for identical identities', () => {
+  const index = buildInstalledIndex([{ id: 'alice/toolkit' }], [], []);
+
+  assert.equal(
+    resolveInstalledState(index, {
+      type: 'mcp',
+      author: 'alice',
+      pluginName: 'toolkit',
+    }).installed,
+    false,
+    'a plugin must not mark the same-named MCP server as installed',
+  );
+  assert.equal(
+    installedExtensionKey(undefined, 'alice', 'toolkit'),
+    'plugin:alice/toolkit',
+    'a missing type must default to plugin',
+  );
+});