From 161147decba70914b70b5d007c45640ece11b522 Mon Sep 17 00:00:00 2001 From: Jack McIntyre Date: Mon, 29 Jun 2026 22:43:31 +1000 Subject: [PATCH] feat(01KW5WPD): computeSkillEffectiveness resolves retired skills correctly (re-rooted path + injected IO seam) so the retro analyst stops proposing to retire already-deleted or moved skills (#423, #403) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #423. Closes #403 (duplicate of #423). ## What The retro analyst was proposing to "retire" skills that had already been deleted (`flow:author`, `flow:judge`, `flow:status`, `flow:board`, `flow:accept-proposal`) because `computeSkillEffectiveness` had no way to filter out retired commands — it tallied every `skill.invoke` event in the 50-story window regardless of whether the skill still exists on disk. ## How - **Injected IO seam**: Added `existsImpl?: (absolutePath: string) => boolean` and `pluginRoot?: string` to `ComputeSkillEffectivenessOptions`. When `existsImpl` is absent the helper is fully backward-compatible (no filtering). Tests inject a fake that never touches the real filesystem (AC2). - **Re-rooted path resolution**: Added `extractSkillCommand` (regex `/[/\\]skills[/\\]([^/\\]+)[/\\]SKILL\.md$/i`) to pull the command name from an absolute, invoke-time-captured `skill_path`. Added `checkInstalled` that probes two candidate dirs: (1) `{targetRepoRoot}/plugins/flow/skills/{cmd}/SKILL.md` (dev/monorepo) and (2) `{pluginRoot}/skills/{cmd}/SKILL.md` (installed-plugin). A live skill whose path moved is NOT misclassified as retired (AC1, AC3). - **Ambiguous-path default**: A `skill_path` matching neither `/skills/{cmd}/SKILL.md` pattern (other-plugin, relative path) defaults to `installed = true` — it is never wrongly dropped (AC4). - **`gatherRetroInputs` wired up**: Now passes `existsImpl: existsSync` and `pluginRoot: getPluginRoot()` (wrapped in try/catch for edge cases), activating the check in production. - **`retro-skill-effectiveness.test.ts` updated**: Existing tests were using retired skills (`flow:author`, `flow:judge`); replaced with live ones (`flow:run`) so they continue to pass after `existsImpl` is active. - **dist rebuilt**: `pnpm build` re-run; `dist/index.js` and `dist/cli.js` committed in the same change (CI fails on drift). --- plugins/flow/mcp-server/dist/cli.js | 65 +++++-- plugins/flow/mcp-server/dist/index.js | 65 +++++-- .../compute-skill-effectiveness.test.ts | 178 ++++++++++++++++++ .../retro-skill-effectiveness.test.ts | 30 +-- .../src/tools/compute-skill-effectiveness.ts | 128 ++++++++++++- .../src/tools/gather-retro-inputs.ts | 23 ++- 6 files changed, 451 insertions(+), 38 deletions(-) diff --git a/plugins/flow/mcp-server/dist/cli.js b/plugins/flow/mcp-server/dist/cli.js index 54a0690b..b3e5433d 100644 --- a/plugins/flow/mcp-server/dist/cli.js +++ b/plugins/flow/mcp-server/dist/cli.js @@ -27850,7 +27850,7 @@ async function logTelemetryEvent(opts) { // src/tools/gather-retro-inputs.ts var import_yaml11 = __toESM(require_dist(), 1); -import { promises as fs19 } from "node:fs"; +import { promises as fs19, existsSync as existsSync4 } from "node:fs"; import * as path26 from "node:path"; // src/schemas/risk-tiering-spec.ts @@ -28588,8 +28588,33 @@ var SkillEffectivenessResultSchema = external_exports.object({ */ attribution: SkillEffectivenessAttribution }).strict(); +function extractSkillCommand(skillPath) { + const match = skillPath.match(/[/\\]skills[/\\]([^/\\]+)[/\\]SKILL\.md$/i); + return match ? match[1] ?? null : null; +} +function checkInstalled(skillPath, targetRepoRoot, pluginRoot, existsImpl) { + const command = extractSkillCommand(skillPath); + if (command === null) { + return true; + } + const candidate1 = path11.join(targetRepoRoot, "plugins", "flow", "skills", command, "SKILL.md"); + if (existsImpl(candidate1)) return true; + if (pluginRoot) { + const candidate2 = path11.join(pluginRoot, "skills", command, "SKILL.md"); + if (existsImpl(candidate2)) return true; + } + return false; +} async function computeSkillEffectiveness(opts) { - const { targetRepoRoot, window: rawWindow, readTelemetryDirImpl, readFileImpl, readDoneRefsImpl } = opts; + const { + targetRepoRoot, + window: rawWindow, + readTelemetryDirImpl, + readFileImpl, + readDoneRefsImpl, + existsImpl, + pluginRoot + } = opts; const window2 = rawWindow ?? DEFAULT_SKILL_EFFECTIVENESS_WINDOW; if (!Number.isFinite(window2) || !Number.isInteger(window2) || window2 <= 0) { throw new SkillEffectivenessWindowInvalidError({ @@ -28703,6 +28728,12 @@ async function computeSkillEffectiveness(opts) { const tally = /* @__PURE__ */ new Map(); let anyNonExecutionInvoke = false; for (const inv of windowedInvokes) { + if (existsImpl !== void 0) { + const installed = checkInstalled(inv.data.skill_path, targetRepoRoot, pluginRoot, existsImpl); + if (!installed) { + continue; + } + } const skill = inv.data.skill_name; const tier = getSkillTier(skill); const entry = tally.get(skill) ?? { invoke: 0, useful: 0, tier }; @@ -32095,7 +32126,17 @@ async function gatherRetroInputs(opts) { }; } const recurringFriction = computeRecurringFriction(telemetrySummary.events); - const skillEffectiveness = await computeSkillEffectiveness({ targetRepoRoot }); + let retroPluginRoot; + try { + retroPluginRoot = getPluginRoot(); + } catch { + retroPluginRoot = void 0; + } + const skillEffectiveness = await computeSkillEffectiveness({ + targetRepoRoot, + existsImpl: (p) => existsSync4(p), + pluginRoot: retroPluginRoot + }); const threshold = opts.mechanicalFailureThreshold ?? MECHANICAL_FAILURE_THRESHOLD; const mechanicalFailuresDrafted = await draftHardeningStories( targetRepoRoot, @@ -41768,7 +41809,7 @@ async function loadRolePermissions(opts) { var import_yaml23 = __toESM(require_dist(), 1); import * as path50 from "node:path"; import { - existsSync as existsSync4, + existsSync as existsSync5, readFileSync as readFileSync5, readdirSync as readdirSync3 } from "node:fs"; @@ -41826,14 +41867,14 @@ function splitCommand(cmd) { } function detectPackageManagerAt(cwd) { for (const [lockfile, pm2] of LOCKFILE_TO_PACKAGE_MANAGER) { - if (existsSync4(path50.join(cwd, lockfile))) { + if (existsSync5(path50.join(cwd, lockfile))) { return { pm: pm2, assumed: false }; } } return { pm: "npm", assumed: true }; } function hasKnipConfig(cwd) { - return KNIP_CONFIG_FILENAMES.some((name) => existsSync4(path50.join(cwd, name))); + return KNIP_CONFIG_FILENAMES.some((name) => existsSync5(path50.join(cwd, name))); } function findWorkspaceMemberWithBuild(workspaceDir) { let packages; @@ -41895,7 +41936,7 @@ function findWorkspaceYamlDir(root, maxDepth) { for (let depth = 0; depth <= maxDepth; depth++) { const next = []; for (const dir of frontier) { - if (existsSync4(path50.join(dir, "pnpm-workspace.yaml"))) return dir; + if (existsSync5(path50.join(dir, "pnpm-workspace.yaml"))) return dir; let entries; try { entries = readdirSync3(dir, { withFileTypes: true }); @@ -42405,7 +42446,7 @@ async function runDevTerminalAction(opts) { // src/tools/run-reviewer-session.ts import * as path54 from "node:path"; import * as fs37 from "node:fs/promises"; -import { existsSync as existsSync6 } from "node:fs"; +import { existsSync as existsSync7 } from "node:fs"; // src/lib/slugify-standards-criterion.ts function slugifyStandardsCriterion(name) { @@ -42581,7 +42622,7 @@ async function materialisePrBranchWorktree(opts) { // src/lib/prepare-review-worktree.ts import * as path53 from "node:path"; -import { existsSync as existsSync5 } from "node:fs"; +import { existsSync as existsSync6 } from "node:fs"; var DEFAULT_INSTALL_TIMEOUT_MS = 10 * 60 * 1e3; function frozenInstallInvocation(pm2) { switch (pm2) { @@ -42605,7 +42646,7 @@ function resolveWorktreeInstallPlan(worktreeRoot) { } while (dir === stop || dir.startsWith(stop + path53.sep)) { for (const [lockfile, pm2] of LOCKFILE_TO_PACKAGE_MANAGER) { - if (existsSync5(path53.join(dir, lockfile))) { + if (existsSync6(path53.join(dir, lockfile))) { const { command, args } = frozenInstallInvocation(pm2); return { installRoot: dir, packageManager: pm2, command, args }; } @@ -42724,7 +42765,7 @@ function countExecutedTests(output) { } function resolveVitestInvocation(packageRoot) { const localBin = path54.join(packageRoot, "node_modules", ".bin", "vitest"); - if (existsSync6(localBin)) { + if (existsSync7(localBin)) { return { command: localBin, args: [] }; } const pm2 = resolveProjectToolchain({ targetRepoRoot: packageRoot }).packageManager; @@ -43017,7 +43058,7 @@ ${installResult.stderr.slice(0, 2e3)}` : ""); epic: bmadDerivation.epic, story: bmadDerivation.story, slug: bmadDerivation.slug, - exists: (relPath) => existsSync6(path54.join(worktreePath, relPath)) + exists: (relPath) => existsSync7(path54.join(worktreePath, relPath)) }) : void 0; if (derived?.type === "artifact") { acResults[ac.index] = await runArtifactCheck( diff --git a/plugins/flow/mcp-server/dist/index.js b/plugins/flow/mcp-server/dist/index.js index 4c88803e..ac1e727c 100644 --- a/plugins/flow/mcp-server/dist/index.js +++ b/plugins/flow/mcp-server/dist/index.js @@ -49373,7 +49373,7 @@ function dedupePaths(paths) { // src/tools/gather-retro-inputs.ts var import_yaml26 = __toESM(require_dist2(), 1); -import { promises as fs41 } from "node:fs"; +import { promises as fs41, existsSync as existsSync4 } from "node:fs"; import * as path52 from "node:path"; // src/schemas/cycle-state.ts @@ -49525,8 +49525,33 @@ var SkillEffectivenessResultSchema = external_exports.object({ */ attribution: SkillEffectivenessAttribution }).strict(); +function extractSkillCommand(skillPath) { + const match = skillPath.match(/[/\\]skills[/\\]([^/\\]+)[/\\]SKILL\.md$/i); + return match ? match[1] ?? null : null; +} +function checkInstalled(skillPath, targetRepoRoot, pluginRoot, existsImpl) { + const command = extractSkillCommand(skillPath); + if (command === null) { + return true; + } + const candidate1 = path39.join(targetRepoRoot, "plugins", "flow", "skills", command, "SKILL.md"); + if (existsImpl(candidate1)) return true; + if (pluginRoot) { + const candidate2 = path39.join(pluginRoot, "skills", command, "SKILL.md"); + if (existsImpl(candidate2)) return true; + } + return false; +} async function computeSkillEffectiveness(opts) { - const { targetRepoRoot, window: rawWindow, readTelemetryDirImpl, readFileImpl, readDoneRefsImpl } = opts; + const { + targetRepoRoot, + window: rawWindow, + readTelemetryDirImpl, + readFileImpl, + readDoneRefsImpl, + existsImpl, + pluginRoot + } = opts; const window2 = rawWindow ?? DEFAULT_SKILL_EFFECTIVENESS_WINDOW; if (!Number.isFinite(window2) || !Number.isInteger(window2) || window2 <= 0) { throw new SkillEffectivenessWindowInvalidError({ @@ -49640,6 +49665,12 @@ async function computeSkillEffectiveness(opts) { const tally = /* @__PURE__ */ new Map(); let anyNonExecutionInvoke = false; for (const inv of windowedInvokes) { + if (existsImpl !== void 0) { + const installed = checkInstalled(inv.data.skill_path, targetRepoRoot, pluginRoot, existsImpl); + if (!installed) { + continue; + } + } const skill = inv.data.skill_name; const tier = getSkillTier(skill); const entry = tally.get(skill) ?? { invoke: 0, useful: 0, tier }; @@ -52806,7 +52837,17 @@ async function gatherRetroInputs(opts) { }; } const recurringFriction = computeRecurringFriction(telemetrySummary.events); - const skillEffectiveness = await computeSkillEffectiveness({ targetRepoRoot }); + let retroPluginRoot; + try { + retroPluginRoot = getPluginRoot(); + } catch { + retroPluginRoot = void 0; + } + const skillEffectiveness = await computeSkillEffectiveness({ + targetRepoRoot, + existsImpl: (p) => existsSync4(p), + pluginRoot: retroPluginRoot + }); const threshold = opts.mechanicalFailureThreshold ?? MECHANICAL_FAILURE_THRESHOLD; const mechanicalFailuresDrafted = await draftHardeningStories( targetRepoRoot, @@ -55908,7 +55949,7 @@ async function loadRolePermissions(opts) { var import_yaml35 = __toESM(require_dist2(), 1); import * as path73 from "node:path"; import { - existsSync as existsSync4, + existsSync as existsSync5, readFileSync as readFileSync5, readdirSync as readdirSync3 } from "node:fs"; @@ -55966,14 +56007,14 @@ function splitCommand(cmd) { } function detectPackageManagerAt(cwd) { for (const [lockfile, pm2] of LOCKFILE_TO_PACKAGE_MANAGER) { - if (existsSync4(path73.join(cwd, lockfile))) { + if (existsSync5(path73.join(cwd, lockfile))) { return { pm: pm2, assumed: false }; } } return { pm: "npm", assumed: true }; } function hasKnipConfig(cwd) { - return KNIP_CONFIG_FILENAMES.some((name) => existsSync4(path73.join(cwd, name))); + return KNIP_CONFIG_FILENAMES.some((name) => existsSync5(path73.join(cwd, name))); } function findWorkspaceMemberWithBuild(workspaceDir) { let packages; @@ -56035,7 +56076,7 @@ function findWorkspaceYamlDir(root, maxDepth) { for (let depth = 0; depth <= maxDepth; depth++) { const next = []; for (const dir of frontier) { - if (existsSync4(path73.join(dir, "pnpm-workspace.yaml"))) return dir; + if (existsSync5(path73.join(dir, "pnpm-workspace.yaml"))) return dir; let entries; try { entries = readdirSync3(dir, { withFileTypes: true }); @@ -56545,7 +56586,7 @@ async function runDevTerminalAction(opts) { // src/tools/run-reviewer-session.ts import * as path77 from "node:path"; import * as fs58 from "node:fs/promises"; -import { existsSync as existsSync6 } from "node:fs"; +import { existsSync as existsSync7 } from "node:fs"; // src/lib/materialise-pr-branch-worktree.ts import * as path75 from "node:path"; @@ -56716,7 +56757,7 @@ async function materialisePrBranchWorktree(opts) { // src/lib/prepare-review-worktree.ts import * as path76 from "node:path"; -import { existsSync as existsSync5 } from "node:fs"; +import { existsSync as existsSync6 } from "node:fs"; var DEFAULT_INSTALL_TIMEOUT_MS = 10 * 60 * 1e3; function frozenInstallInvocation(pm2) { switch (pm2) { @@ -56740,7 +56781,7 @@ function resolveWorktreeInstallPlan(worktreeRoot) { } while (dir === stop || dir.startsWith(stop + path76.sep)) { for (const [lockfile, pm2] of LOCKFILE_TO_PACKAGE_MANAGER) { - if (existsSync5(path76.join(dir, lockfile))) { + if (existsSync6(path76.join(dir, lockfile))) { const { command, args } = frozenInstallInvocation(pm2); return { installRoot: dir, packageManager: pm2, command, args }; } @@ -56859,7 +56900,7 @@ function countExecutedTests(output) { } function resolveVitestInvocation(packageRoot) { const localBin = path77.join(packageRoot, "node_modules", ".bin", "vitest"); - if (existsSync6(localBin)) { + if (existsSync7(localBin)) { return { command: localBin, args: [] }; } const pm2 = resolveProjectToolchain({ targetRepoRoot: packageRoot }).packageManager; @@ -57152,7 +57193,7 @@ ${installResult.stderr.slice(0, 2e3)}` : ""); epic: bmadDerivation.epic, story: bmadDerivation.story, slug: bmadDerivation.slug, - exists: (relPath) => existsSync6(path77.join(worktreePath, relPath)) + exists: (relPath) => existsSync7(path77.join(worktreePath, relPath)) }) : void 0; if (derived?.type === "artifact") { acResults[ac.index] = await runArtifactCheck( diff --git a/plugins/flow/mcp-server/src/tools/__tests__/compute-skill-effectiveness.test.ts b/plugins/flow/mcp-server/src/tools/__tests__/compute-skill-effectiveness.test.ts index d032279d..8aec195c 100644 --- a/plugins/flow/mcp-server/src/tools/__tests__/compute-skill-effectiveness.test.ts +++ b/plugins/flow/mcp-server/src/tools/__tests__/compute-skill-effectiveness.test.ts @@ -682,3 +682,181 @@ describe("computeSkillEffectiveness — tier-aware scoring", () => { expect(result.attribution).toBe("attributed"); }); }); + +// --------------------------------------------------------------------------- +// Retired-skill installed-check — Story native:01KW5WPDPJY5DK6JV810307E0J +// AC1: re-rooted path, retired excluded, moved/renamed live skill kept. +// AC2: injected IO seam (no raw fs — fake ROOT=/abs/repo never touched). +// AC3: live skill counts unaffected by the filter. +// AC4: ambiguous path (matches neither candidate dir) defaults to installed. +// --------------------------------------------------------------------------- + +describe("computeSkillEffectiveness — retired-skill installed-check (native:01KW5WPDPJY5DK6JV810307E0J)", () => { + /** + * Commands that are "currently installed" in this test suite. + * Corresponds to the flow plugin's current skill palette. + * Any command NOT in this set is treated as retired. + */ + const liveCommands = new Set([ + "run", "plan", "retro", "ready", "hire", "ask", "dashboard", "help", "init", + ]); + + /** + * Build a fake existsImpl that examines the re-rooted candidate-1 path + * ({ROOT}/plugins/flow/skills/{command}/SKILL.md) and returns true only when + * the extracted command is in `liveCommands`. + * + * ROOT = "/abs/repo" is non-existent on disk — this seam ensures NO real fs + * access happens inside the helper, which is the whole point of AC2. + * + * @param onCheck Optional spy callback fired on every existsImpl call. + */ + function makeExistsImpl( + onCheck?: (path: string) => void, + ): (filePath: string) => boolean { + return (filePath: string): boolean => { + onCheck?.(filePath); + const m = filePath.match(/[/\\]skills[/\\]([^/\\]+)[/\\]SKILL\.md$/i); + if (!m) return false; + return liveCommands.has(m[1] ?? ""); + }; + } + + it("AC1: retired skill is excluded; live skill whose path was captured at an old install location is NOT misclassified", async () => { + // flow:run is live → kept in per_skill with its READY FOR MERGE verdict counted. + // flow:author is retired (removed from the plugin) → excluded from per_skill. + // The skill_paths are captured at an OLD install location (/abs/plugins/flow/...) + // that no longer matches the current install — but the re-rooting logic extracts + // the command and checks the candidate dir, so a live skill is never wrong excluded. + const events = [ + makeInvoke({ ts: makeTs(1000), session_id: "s-ret-1", skill_name: "flow:run", story_id: "native:01RET001" }), + makeInvoke({ ts: makeTs(2000), session_id: "s-ret-1", skill_name: "flow:author", story_id: "native:01RET002" }), + makeVerdict({ ts: makeTs(3000), session_id: "run-ret-1", pr_number: 1, verdict: "READY FOR MERGE", story_id: "native:01RET001" }), + ].map(assertValid); + + const result = await computeSkillEffectiveness({ + targetRepoRoot: ROOT, + ...seams({ "2026-05.jsonl": events }), + existsImpl: makeExistsImpl(), + }); + + expect(SkillEffectivenessResultSchema.safeParse(result).success).toBe(true); + // flow:run is live → present in per_skill with its verdict-joined useful fire. + expect(result.per_skill).toHaveProperty("flow:run"); + expect(result.per_skill["flow:run"]).toMatchObject({ + invoke_count: 1, + useful_fire_count: 1, + effectiveness_ratio: 1, + skill_tier: "execution", + }); + // flow:author is retired → excluded from the scored set (zero-ratio active set). + expect(result.per_skill).not.toHaveProperty("flow:author"); + }); + + it("AC2: installed-check routes through the injected IO seam; no raw fs is used", async () => { + // ROOT = "/abs/repo" does not exist on disk. If the helper called raw existsSync + // internally, the check would always return false (no skills dir at /abs/repo) and + // every skill would be wrongly filtered. The injected seam is the ONLY IO for the + // installed-check — this test is a structural proof that raw fs is forbidden. + let seamCallCount = 0; + const trackingExistsImpl = (p: string): boolean => { + seamCallCount++; + return makeExistsImpl()(p); + }; + + const events = [ + makeInvoke({ ts: makeTs(1000), session_id: "s-seam-1", skill_name: "flow:run", story_id: "native:01SEAM001" }), + ].map(assertValid); + + const result = await computeSkillEffectiveness({ + targetRepoRoot: ROOT, + ...seams({ "2026-05.jsonl": events }), + existsImpl: trackingExistsImpl, + }); + + // The seam was called — confirming the installed-check executed. + expect(seamCallCount).toBeGreaterThanOrEqual(1); + // flow:run is live (seam returned true for its candidate) → in per_skill. + expect(result.per_skill).toHaveProperty("flow:run"); + // Because ROOT is a fake non-existent path, only the injected seam can return + // true here. If the test passes, the helper did NOT fall back to raw fs. + }); + + it("AC3: a skill still installed on disk is unaffected — invoke_count and effectiveness_ratio unchanged by the filter", async () => { + // flow:plan is a live planning-tier skill. With existsImpl active, the + // installed-check must NOT alter its invoke_count or effectiveness_ratio. + // Planning tier is presence-scored → every invocation is a useful fire → ratio 1. + const events = [ + makeInvoke({ ts: makeTs(1000), session_id: "s-live-1", skill_name: "flow:plan", story_id: "native:01LIVE001" }), + makeInvoke({ ts: makeTs(2000), session_id: "s-live-1", skill_name: "flow:plan", story_id: "native:01LIVE001" }), + ].map(assertValid); + + const result = await computeSkillEffectiveness({ + targetRepoRoot: ROOT, + ...seams({ "2026-05.jsonl": events }), + existsImpl: makeExistsImpl(), + }); + + // Both invocations are kept — the installed-check does NOT skip live skills. + expect(result.per_skill["flow:plan"]).toEqual({ + invoke_count: 2, + useful_fire_count: 2, + effectiveness_ratio: 1, + skill_tier: "planning", + }); + }); + + it("AC4 (integration): retired skill excluded; ambiguous other-plugin path defaults to installed and is never wrongly dropped", async () => { + // Scenario verbatim from the story AC4: + // - flow:author is retired (skill_path matches the canonical /skills/{cmd}/SKILL.md + // pattern, existsImpl returns false for its candidate) → excluded from per_skill. + // - "flow:relpath" has a skill_path that matches NEITHER candidate dir (no + // /skills/{command}/SKILL.md suffix at all) → ambiguous → treated as installed, + // so it is NOT filtered out even though it is not in liveCommands. + // + // The READY FOR MERGE verdict for the ambiguous skill confirms it is correctly + // scored (execution tier, verdict join) — the retro sees a real signal, not a + // silently-dropped skill that still exists in some other plugin or configuration. + const ambiguousPath = "other-plugin/relpath/something.md"; // no /skills/{cmd}/SKILL.md + + const eventsRaw = [ + // Retired skill in the window. + assertValid(makeInvoke({ ts: makeTs(1000), session_id: "s-ac4-ret", skill_name: "flow:author", story_id: "native:01AC4RET" })), + // Ambiguous-path skill: manually override skill_path to a non-canonical value. + assertValid({ + ts: makeTs(2000), + session_id: "s-ac4-amb", + agent: "user", + story_id: "native:01AC4AMB", + type: "skill.invoke", + data: { + skill_name: "flow:relpath", + skill_path: ambiguousPath, + skill_version: "0.1.0", + skill_scope: "plugin" as const, + invocation_source: "user-slash-command" as const, + }, + }), + // Verdict for the ambiguous skill → it IS a useful fire. + assertValid(makeVerdict({ ts: makeTs(3000), session_id: "run-ac4-amb", pr_number: 1, verdict: "READY FOR MERGE", story_id: "native:01AC4AMB" })), + ]; + + const result = await computeSkillEffectiveness({ + targetRepoRoot: ROOT, + ...seams({ "2026-05.jsonl": eventsRaw }), + existsImpl: makeExistsImpl(), + }); + + expect(SkillEffectivenessResultSchema.safeParse(result).success).toBe(true); + // Retired skill: excluded — does not appear as a 0-ratio active skill. + expect(result.per_skill).not.toHaveProperty("flow:author"); + // Ambiguous skill: treated as installed — never wrongly dropped. + expect(result.per_skill).toHaveProperty("flow:relpath"); + expect(result.per_skill["flow:relpath"]).toEqual({ + invoke_count: 1, + useful_fire_count: 1, + effectiveness_ratio: 1, + skill_tier: "execution", + }); + }); +}); diff --git a/plugins/flow/mcp-server/src/tools/__tests__/retro-skill-effectiveness.test.ts b/plugins/flow/mcp-server/src/tools/__tests__/retro-skill-effectiveness.test.ts index fd1ad111..77218c51 100644 --- a/plugins/flow/mcp-server/src/tools/__tests__/retro-skill-effectiveness.test.ts +++ b/plugins/flow/mcp-server/src/tools/__tests__/retro-skill-effectiveness.test.ts @@ -101,13 +101,18 @@ describe("AC1 — gatherRetroInputs includes skillEffectiveness.per_skill with c // flow:plan invoked twice in story S1; the story reaches READY FOR MERGE // after both invocations → both are useful fires → ratio 1. - // flow:author invoked once in story S2 whose verdict is NEEDS CHANGES → + // flow:run invoked once in story S2 whose verdict is NEEDS CHANGES → // not a useful fire → ratio 0. + // + // NOTE: both skills must be currently installed (exist at the plugin root) + // because gatherRetroInputs now passes existsImpl: existsSync to the scorer. + // flow:plan and flow:run are live skills; retired commands like flow:author + // would be filtered out by the installed-check and excluded from per_skill. const lines = [ makeSkillInvokeLine({ skill_name: "flow:plan", session_id: "01KSESSAAAAAAAAAAAAAAAAAA1", story_id: "native:01KSTORY00000000000000001", ts: "2026-06-01T10:00:00.000Z" }), makeSkillInvokeLine({ skill_name: "flow:plan", session_id: "01KSESSAAAAAAAAAAAAAAAAAA1", story_id: "native:01KSTORY00000000000000001", ts: "2026-06-01T10:01:00.000Z" }), makeVerdictLine({ session_id: "01KSESSAAAAAAAAAAAAAAAAAA1", story_id: "native:01KSTORY00000000000000001", verdict: "READY FOR MERGE", pr_number: 101, ts: "2026-06-01T10:05:00.000Z" }), - makeSkillInvokeLine({ skill_name: "flow:author", session_id: "01KSESSBBBBBBBBBBBBBBBBBB2", story_id: "native:01KSTORY00000000000000002", ts: "2026-06-01T11:00:00.000Z" }), + makeSkillInvokeLine({ skill_name: "flow:run", session_id: "01KSESSBBBBBBBBBBBBBBBBBB2", story_id: "native:01KSTORY00000000000000002", ts: "2026-06-01T11:00:00.000Z" }), makeVerdictLine({ session_id: "01KSESSBBBBBBBBBBBBBBBBBB2", story_id: "native:01KSTORY00000000000000002", verdict: "NEEDS CHANGES", pr_number: 102, ts: "2026-06-01T11:05:00.000Z" }), ]; await fs.writeFile( @@ -130,9 +135,8 @@ describe("AC1 — gatherRetroInputs includes skillEffectiveness.per_skill with c skill_tier: "planning", }); - // flow:author — execution tier (not in tier table); 1 invocation, no READY - // FOR MERGE → ratio 0 (never NaN). - expect(perSkill["flow:author"]).toEqual({ + // flow:run — execution tier; 1 invocation with NEEDS CHANGES → ratio 0 (never NaN). + expect(perSkill["flow:run"]).toEqual({ invoke_count: 1, useful_fire_count: 0, effectiveness_ratio: 0, @@ -144,14 +148,18 @@ describe("AC1 — gatherRetroInputs includes skillEffectiveness.per_skill with c const telemetryDir = path.join(tmpRoot, ".flow", "telemetry"); await fs.mkdir(telemetryDir, { recursive: true }); - // flow:judge fired 3 times; only 1 invocation is followed by a READY FOR + // flow:run fired 3 times; only 1 invocation is followed by a READY FOR // MERGE in its own story flow → useful_fire_count 1 / invoke_count 3. + // + // NOTE: flow:run must be a currently-installed skill (live at the plugin root) + // because gatherRetroInputs now passes existsImpl: existsSync to the scorer. + // Retired commands like flow:judge would be excluded by the installed-check. const lines = [ - makeSkillInvokeLine({ skill_name: "flow:judge", session_id: "01KSESSCCCCCCCCCCCCCCCCCC1", story_id: "native:01KSTORY00000000000000010", ts: "2026-06-02T09:00:00.000Z" }), + makeSkillInvokeLine({ skill_name: "flow:run", session_id: "01KSESSCCCCCCCCCCCCCCCCCC1", story_id: "native:01KSTORY00000000000000010", ts: "2026-06-02T09:00:00.000Z" }), makeVerdictLine({ session_id: "01KSESSCCCCCCCCCCCCCCCCCC1", story_id: "native:01KSTORY00000000000000010", verdict: "READY FOR MERGE", pr_number: 201, ts: "2026-06-02T09:10:00.000Z" }), - makeSkillInvokeLine({ skill_name: "flow:judge", session_id: "01KSESSDDDDDDDDDDDDDDDDDD2", story_id: "native:01KSTORY00000000000000011", ts: "2026-06-02T10:00:00.000Z" }), + makeSkillInvokeLine({ skill_name: "flow:run", session_id: "01KSESSDDDDDDDDDDDDDDDDDD2", story_id: "native:01KSTORY00000000000000011", ts: "2026-06-02T10:00:00.000Z" }), makeVerdictLine({ session_id: "01KSESSDDDDDDDDDDDDDDDDDD2", story_id: "native:01KSTORY00000000000000011", verdict: "NEEDS CHANGES", pr_number: 202, ts: "2026-06-02T10:10:00.000Z" }), - makeSkillInvokeLine({ skill_name: "flow:judge", session_id: "01KSESSEEEEEEEEEEEEEEEEEE3", story_id: "native:01KSTORY00000000000000012", ts: "2026-06-02T11:00:00.000Z" }), + makeSkillInvokeLine({ skill_name: "flow:run", session_id: "01KSESSEEEEEEEEEEEEEEEEEE3", story_id: "native:01KSTORY00000000000000012", ts: "2026-06-02T11:00:00.000Z" }), // No verdict for the third invocation's story → not useful. ]; await fs.writeFile( @@ -161,9 +169,9 @@ describe("AC1 — gatherRetroInputs includes skillEffectiveness.per_skill with c ); const bundle = await gatherRetroInputs({ targetRepoRoot: tmpRoot }); - const entry = bundle.skillEffectiveness.per_skill["flow:judge"]; + const entry = bundle.skillEffectiveness.per_skill["flow:run"]; - // The analyst can write: "flow:judge fired 3 times (invoke_count) with an + // The analyst can write: "flow:run fired 3 times (invoke_count) with an // effectiveness_ratio of 0.333 — a candidate to revise." expect(entry).toBeDefined(); expect(entry!.invoke_count).toBe(3); diff --git a/plugins/flow/mcp-server/src/tools/compute-skill-effectiveness.ts b/plugins/flow/mcp-server/src/tools/compute-skill-effectiveness.ts index 0a311c68..7afd6964 100644 --- a/plugins/flow/mcp-server/src/tools/compute-skill-effectiveness.ts +++ b/plugins/flow/mcp-server/src/tools/compute-skill-effectiveness.ts @@ -237,12 +237,116 @@ export interface ComputeSkillEffectivenessOptions { * operator merge, etc.). */ readDoneRefsImpl?: (doneDir: string) => Promise>; + /** + * Test seam / production installed-check: inject a function that returns + * whether an absolute path exists on disk (Story native:01KW5WPDPJY5DK6JV810307E0J). + * + * When provided, the helper re-roots each `skill.invoke`'s captured `skill_path` + * against candidate dirs derived from `targetRepoRoot` and `pluginRoot`, then + * calls `existsImpl` to determine if the skill is still installed. Invocations + * from skills that fail all candidate checks are excluded from the scored set — + * retiring skills that were already deleted, without wrongly filtering live skills + * whose install path changed. + * + * When absent (the default), all skills are treated as installed — backward- + * compatible with callers that do not need installed-check filtering. + * + * Raw fs is forbidden inside this helper — callers supply this seam for the + * installed-check to be active. Production callers pass `(p) => existsSync(p)`; + * tests pass a fake that returns `true`/`false` for known paths without touching + * the real filesystem (AC2 of Story native:01KW5WPDPJY5DK6JV810307E0J). + */ + existsImpl?: (absolutePath: string) => boolean; + /** + * Optional current plugin root (absolute path) for the re-rooted installed-check + * (Story native:01KW5WPDPJY5DK6JV810307E0J). + * + * When `existsImpl` is provided and a `skill_path` matches the canonical pattern + * (`…/skills/{command}/SKILL.md`), the helper checks two candidate paths: + * + * 1. `{targetRepoRoot}/plugins/flow/skills/{command}/SKILL.md` — covers the + * dev/mono-repo case where the plugin source lives inside the target repo. + * 2. `{pluginRoot}/skills/{command}/SKILL.md` — covers the installed-plugin + * case where skills live at the current plugin installation root. + * + * If either candidate exists (per `existsImpl`), the skill is treated as + * installed. Providing `pluginRoot` (typically `getPluginRoot()`) makes the + * check correct for external users whose projects do NOT contain the plugin + * source. When omitted, only candidate 1 is checked. + * + * Production callers pass `pluginRoot: getPluginRoot()`; tests typically omit + * it (their fake `existsImpl` already returns `true`/`false` for candidate-1 + * shaped paths). + */ + pluginRoot?: string; } // --------------------------------------------------------------------------- // Implementation // --------------------------------------------------------------------------- +// --------------------------------------------------------------------------- +// Installed-check helpers (Story native:01KW5WPDPJY5DK6JV810307E0J) +// --------------------------------------------------------------------------- + +/** + * Extract the skill command name from an absolute `skill_path` using the + * canonical pattern `…/skills/{command}/SKILL.md`. Returns `null` for paths + * that do not match (other-plugin paths, relative paths, bare skill names) — + * the caller treats `null` as "ambiguous" and defaults to installed (AC4). + * + * Raw fs is never called here — this is pure string parsing. + */ +function extractSkillCommand(skillPath: string): string | null { + const match = skillPath.match(/[/\\]skills[/\\]([^/\\]+)[/\\]SKILL\.md$/i); + return match ? (match[1] ?? null) : null; +} + +/** + * Determine whether a skill is currently installed, using two re-rooted + * candidate paths and the injected `existsImpl` seam. + * + * Returns `true` (installed) when: + * - The `skill_path` is ambiguous (no canonical `…/skills/{cmd}/SKILL.md` + * pattern match) — default-kept so no live skill is wrongly dropped (AC4). + * - `existsImpl` confirms at least one candidate path exists. + * + * Candidate 1 — re-rooted under `{targetRepoRoot}/plugins/flow/skills/`: + * Covers the dev/mono-repo case where the plugin source lives inside the + * target repo (the flow plugin is at `plugins/flow/` within the repo). + * + * Candidate 2 — re-rooted under `{pluginRoot}/skills/` (when provided): + * Covers the installed-plugin case where skills live at the current plugin + * installation root (e.g. `~/.claude/plugins/cache/flow/0.1.0/skills/`). + * This is the path `getPluginRoot()` resolves. + * + * Returns `false` (retired) only when the path matches the pattern AND + * `existsImpl` returns `false` for ALL candidate paths. + */ +function checkInstalled( + skillPath: string, + targetRepoRoot: string, + pluginRoot: string | undefined, + existsImpl: (absolutePath: string) => boolean, +): boolean { + const command = extractSkillCommand(skillPath); + if (command === null) { + return true; // ambiguous path — default to installed (AC4) + } + + // Candidate 1: re-root under targetRepoRoot/plugins/flow/skills/ (dev/mono-repo case). + const candidate1 = path.join(targetRepoRoot, "plugins", "flow", "skills", command, "SKILL.md"); + if (existsImpl(candidate1)) return true; + + // Candidate 2: re-root under pluginRoot/skills/ (installed-plugin case). + if (pluginRoot) { + const candidate2 = path.join(pluginRoot, "skills", command, "SKILL.md"); + if (existsImpl(candidate2)) return true; + } + + return false; // skill is genuinely not installed — excluded from the scored set +} + /** * Compute per-skill effectiveness from `skill.invoke` events joined to * downstream `READY FOR MERGE` reviewer verdicts. @@ -256,7 +360,15 @@ export interface ComputeSkillEffectivenessOptions { export async function computeSkillEffectiveness( opts: ComputeSkillEffectivenessOptions, ): Promise { - const { targetRepoRoot, window: rawWindow, readTelemetryDirImpl, readFileImpl, readDoneRefsImpl } = opts; + const { + targetRepoRoot, + window: rawWindow, + readTelemetryDirImpl, + readFileImpl, + readDoneRefsImpl, + existsImpl, + pluginRoot, + } = opts; // ------------------------------------------------------------------ // Step 1: Validate window (mirrors computeAgreement's AC2c guard). @@ -467,6 +579,20 @@ export async function computeSkillEffectiveness( let anyNonExecutionInvoke = false; for (const inv of windowedInvokes) { + // Installed-check (Story native:01KW5WPDPJY5DK6JV810307E0J — AC1/AC2/AC3/AC4). + // Only active when existsImpl is injected; when absent, all skills are treated + // as installed for backward-compatibility. Re-roots the telemetry's absolute + // skill_path against candidate dirs, calls existsImpl, and skips invocations + // from skills that are genuinely not installed (retired/removed commands). + // Ambiguous paths (no canonical /skills/{cmd}/SKILL.md pattern) default to + // installed so no live skill is ever wrongly excluded (AC4). + if (existsImpl !== undefined) { + const installed = checkInstalled(inv.data.skill_path, targetRepoRoot, pluginRoot, existsImpl); + if (!installed) { + continue; // skip this invocation — skill is retired/not installed + } + } + const skill = inv.data.skill_name; const tier = getSkillTier(skill); const entry = tally.get(skill) ?? { invoke: 0, useful: 0, tier }; diff --git a/plugins/flow/mcp-server/src/tools/gather-retro-inputs.ts b/plugins/flow/mcp-server/src/tools/gather-retro-inputs.ts index c7669e36..4569ded9 100644 --- a/plugins/flow/mcp-server/src/tools/gather-retro-inputs.ts +++ b/plugins/flow/mcp-server/src/tools/gather-retro-inputs.ts @@ -59,7 +59,7 @@ * All other reads are side-effect-free. No network. No unguarded clock dependency. */ -import { promises as fs } from "node:fs"; +import { promises as fs, existsSync } from "node:fs"; import * as path from "node:path"; import { parse as yamlParse } from "yaml"; import { readCycleState, type CycleState } from "../schemas/cycle-state.js"; @@ -100,6 +100,7 @@ import { parsePersonaFile } from "../lib/persona-file.js"; import { recordMaintainerFeedback } from "./record-maintainer-feedback.js"; import { MaintainerFeedbackItemSchema } from "../schemas/maintainer-feedback.js"; import { analyzeTeamFit, type AnalyzeTeamFitResult } from "./analyze-team-fit.js"; +import { getPluginRoot } from "../lib/plugin-root.js"; /** Month-bucket filename pattern matching the Story 1.5 logger contract. */ const TELEMETRY_FILE_REGEX = /\.jsonl$/; @@ -438,7 +439,25 @@ export async function gatherRetroInputs( // to a downstream READY FOR MERGE verdict. It always returns a safe shape // (an empty per_skill map when no telemetry exists), so no null-guard is // needed and the retro never fails on an absent signal. - const skillEffectiveness = await computeSkillEffectiveness({ targetRepoRoot }); + // + // Story native:01KW5WPDPJY5DK6JV810307E0J: pass the existsImpl seam + + // pluginRoot so the helper re-roots each skill's captured absolute path and + // excludes skills that are no longer installed (retired/removed commands), + // stopping the retro from proposing to retire already-deleted skills. + // getPluginRoot() is wrapped in a try/catch — if the plugin root cannot be + // located (edge-case: detached test environment) we fall back to `undefined` + // and only candidate-1 (targetRepoRoot-based) is checked. + let retroPluginRoot: string | undefined; + try { + retroPluginRoot = getPluginRoot(); + } catch { + retroPluginRoot = undefined; + } + const skillEffectiveness = await computeSkillEffectiveness({ + targetRepoRoot, + existsImpl: (p) => existsSync(p), + pluginRoot: retroPluginRoot, + }); // Draft hardening stories for recurring mechanical failures // (Story native:01KT6RHTE3YME1ZAD5VRQAKDSW). This is a write side-effect of