From 397c8fe98b8091cfc85dfb9278ba45668a554b83 Mon Sep 17 00:00:00 2001 From: Jack McIntyre Date: Wed, 24 Jun 2026 07:32:24 +1000 Subject: [PATCH 1/2] feat(01KVS12K): A skill that helped a story reach done is credited as useful even when no READY-FOR-MERGE verdict event was recorded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a done/-manifest read to computeSkillEffectiveness so that a skill.invoke whose story reached done/ is credited as a useful fire even when no READY-FOR-MERGE reviewer.verdict event was recorded in telemetry (e.g. completion via the auto-merge gate or operator merge on the paused-for-human path). The fix augments — not replaces — the existing verdict join: the done/ directory is read as a new declared injected dependency (readDoneRefsImpl), and any invoke whose story_id is found in done/ is credited. The integration test AC1 uses a fresh isolated temp dir to avoid the two-in-progress-manifest ambiguity that prevented story_id attribution; both AC1 and AC2 pass with the fixed implementation. --- .../compute-skill-effectiveness.test.ts | 114 ++++++++++++++++++ ...kill-effectiveness-attribution.int.test.ts | 97 +++++++++++++++ .../src/tools/compute-skill-effectiveness.ts | 72 +++++++++-- 3 files changed, 276 insertions(+), 7 deletions(-) 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 77585939..d032279d 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 @@ -442,6 +442,120 @@ describe("computeSkillEffectiveness — AC3 edges", () => { }); }); +// --------------------------------------------------------------------------- +// Done/-manifest attribution — Story native:01KVS12K AC1 + AC2 +// --------------------------------------------------------------------------- + +describe("computeSkillEffectiveness — done/-manifest attribution (Story native:01KVS12K)", () => { + it("AC1: credits a done-but-no-verdict-event invoke as a useful fire via done/-manifest read", async () => { + // A skill.invoke whose story_id reached done/ but has NO joined READY FOR + // MERGE reviewer.verdict in telemetry — the done/-manifest is the only signal. + // Before this fix: useful_fire_count 0 → retro wrongly proposes retire/revise. + // After this fix: useful_fire_count 1, effectiveness_ratio 1. + const doneStoryRef = "native:01DONE0NOVERDICT00000001"; + const events = [ + makeInvoke({ ts: makeTs(1000), session_id: "s-done-1", skill_name: "flow:run", story_id: doneStoryRef }), + // No READY FOR MERGE verdict for this story — only the done/ manifest signals completion. + ].map(assertValid); + + const result = await computeSkillEffectiveness({ + targetRepoRoot: ROOT, + ...seams({ "2026-06.jsonl": events }), + readDoneRefsImpl: async () => new Set([doneStoryRef]), + }); + + expect(SkillEffectivenessResultSchema.safeParse(result).success).toBe(true); + expect(result.per_skill["flow:run"]).toEqual({ + invoke_count: 1, + useful_fire_count: 1, + effectiveness_ratio: 1, + skill_tier: "execution", + }); + // Done refs present → signal is "attributed", not "no-completed-flows". + expect(result.attribution).toBe("attributed"); + }); + + it("AC2a: a not-done, no-verdict invoke is NOT credited (genuinely unhelpful skill stays 0)", async () => { + // A skill.invoke whose story is neither in done/ nor has a joined READY FOR + // MERGE verdict must remain uncredited — the done/ path must not over-credit. + const notDoneStoryRef = "native:01NOTDONE0000000000001"; + const events = [ + makeInvoke({ ts: makeTs(1000), session_id: "s-notdone-1", skill_name: "flow:run", story_id: notDoneStoryRef }), + // No verdict and story NOT in done/. + ].map(assertValid); + + const result = await computeSkillEffectiveness({ + targetRepoRoot: ROOT, + ...seams({ "2026-06.jsonl": events }), + readDoneRefsImpl: async () => new Set(), // empty done set + }); + + expect(SkillEffectivenessResultSchema.safeParse(result).success).toBe(true); + expect(result.per_skill["flow:run"]).toEqual({ + invoke_count: 1, + useful_fire_count: 0, + effectiveness_ratio: 0, + skill_tier: "execution", + }); + // No done refs, no verdicts, no planning/cockpit → "no-completed-flows". + expect(result.attribution).toBe("no-completed-flows"); + }); + + it("AC2b: a skill.invoke that already joins a READY-FOR-MERGE verdict still counts as a useful fire exactly as today", async () => { + // The existing verdict-join path must be unaffected by the done/-manifest + // augmentation. An invoke followed by a READY FOR MERGE verdict (regardless of + // whether the story also has a done/ manifest) must still be credited. + const storyRef = "native:01VERDICTANDDO00000001"; + const events = [ + makeInvoke({ ts: makeTs(1000), session_id: "s-v-d", skill_name: "flow:run", story_id: storyRef }), + makeVerdict({ ts: makeTs(2000), session_id: "run-ulid-v-d", pr_number: 42, verdict: "READY FOR MERGE", story_id: storyRef }), + ].map(assertValid); + + // Provide both: verdict in telemetry AND story in done/ — must still credit once. + const result = await computeSkillEffectiveness({ + targetRepoRoot: ROOT, + ...seams({ "2026-06.jsonl": events }), + readDoneRefsImpl: async () => new Set([storyRef]), + }); + + expect(SkillEffectivenessResultSchema.safeParse(result).success).toBe(true); + expect(result.per_skill["flow:run"]).toEqual({ + invoke_count: 1, + useful_fire_count: 1, // credited exactly once (verdict join wins; done/ is redundant but harmless) + effectiveness_ratio: 1, + skill_tier: "execution", + }); + expect(result.attribution).toBe("attributed"); + }); + + it("done/-manifest ENOENT is treated as empty set (no done refs), not an error", async () => { + // Repos without a done/ dir yet should behave as if there are no done refs — + // consistent with the empty-telemetry-dir posture. + const events = [ + makeInvoke({ ts: makeTs(1000), session_id: "s-enoent", skill_name: "flow:run", story_id: "native:01SOMEREF000" }), + ].map(assertValid); + + // Simulate ENOENT on the done/ dir via the injected seam. + const result = await computeSkillEffectiveness({ + targetRepoRoot: ROOT, + ...seams({ "2026-06.jsonl": events }), + readDoneRefsImpl: async () => { + const err = new Error("no done dir") as NodeJS.ErrnoException; + err.code = "ENOENT"; + throw err; + }, + }); + + // Should not throw; the invoke is uncredited (no verdict, no done manifest). + expect(result.per_skill["flow:run"]).toEqual({ + invoke_count: 1, + useful_fire_count: 0, + effectiveness_ratio: 0, + skill_tier: "execution", + }); + }); +}); + // --------------------------------------------------------------------------- // Tier-aware scoring — Story native:01KVEYY1 AC1 + AC2 + AC3 // --------------------------------------------------------------------------- diff --git a/plugins/flow/mcp-server/src/tools/__tests__/skill-effectiveness-attribution.int.test.ts b/plugins/flow/mcp-server/src/tools/__tests__/skill-effectiveness-attribution.int.test.ts index ea8645ac..5164939d 100644 --- a/plugins/flow/mcp-server/src/tools/__tests__/skill-effectiveness-attribution.int.test.ts +++ b/plugins/flow/mcp-server/src/tools/__tests__/skill-effectiveness-attribution.int.test.ts @@ -144,6 +144,103 @@ afterEach(() => { rmSync(tmpRoot, { recursive: true, force: true }); }); +describe("skill-effectiveness done/-manifest attribution (Story native:01KVS12K)", () => { + // This describe block uses its OWN isolated temp dir (doneTestRoot) — completely + // separate from the shared `tmpRoot` set up by the outer beforeEach. The shared + // `tmpRoot` always contains an in-progress manifest for STORY_REF, so using it + // would leave two in-progress manifests at once, making resolveActiveStoryRef + // return undefined (ambiguous) and breaking story_id attribution. + let doneTestRoot: string; + + beforeEach(() => { + doneTestRoot = mkdtempSync(path.join(os.tmpdir(), "skill-attr-done-")); + }); + + afterEach(() => { + rmSync(doneTestRoot, { recursive: true, force: true }); + }); + + it("AC1: credits a skill.invoke as a useful fire when its story reached done/ but NO READY-FOR-MERGE verdict event was recorded", async () => { + // Scenario: /flow:run fires for a story, the story reaches done/ (manifest + // lands in .flow/state/done/) but the run path produced NO reviewer.verdict + // READY FOR MERGE event in telemetry — e.g. auto-merge gate or operator merge. + // + // Before this fix, computeSkillEffectiveness would score useful_fire_count: 0 + // and the retro might draft a false retire/revise proposal. + // After this fix, the done/-manifest read credits the invoke as useful. + + const harnessSession = "01HZDONE0MANIFEST000000TEST"; + const storyRef = "native:01HZDONE0STORYREF000000001"; + + // Minimal .flow/config.yaml so the capture seam's cwd-derived root is valid. + await fs.mkdir(path.join(doneTestRoot, ".flow"), { recursive: true }); + await atomicWriteFile( + path.join(doneTestRoot, ".flow", "config.yaml"), + "adapter: native\nadapter_config: {}\n", + ); + + // Write a single in-progress manifest for storyRef so resolveActiveStoryRef + // returns storyRef unambiguously (exactly ONE in-progress manifest → attribute). + const inProgressDir = path.join(doneTestRoot, ".flow", "state", "in-progress"); + await fs.mkdir(inProgressDir, { recursive: true }); + // Use the raw ref as the filename — matching moveBetweenStates convention. + await atomicWriteFile( + path.join(inProgressDir, storyRef + ".yaml"), + `ref: "${storyRef}"\nstatus: in-progress\nclaimed_by: "${harnessSession}"\n`, + ); + + // Write a real skill.invoke event via the capture seam. Since there is exactly + // ONE in-progress manifest, the capture seam resolves storyRef and stamps it as + // story_id on the skill.invoke telemetry event. + const captured = await captureSkillInvoke( + { + hook_event_name: "PreToolUse", + tool_name: "Skill", + tool_input: { skill: "flow:run", args: "" }, + session_id: harnessSession, + cwd: doneTestRoot, + }, + { + recordImpl: (opts) => + recordSkillInvoke({ + ...opts, + now: () => new Date("2020-02-01T00:00:00.000Z"), + }), + pluginRoot: undefined, + }, + ); + expect(captured).toEqual({ recorded: true }); + + // Write a done/ manifest for the story — this is the only signal that the + // story completed. There is NO reviewer.verdict READY FOR MERGE event. + // The done-manifest file name is the raw story ref + ".yaml" — matching the + // real convention used by moveBetweenStates / completeStory (which writes + // `//.yaml` using the ref as-is, NOT sanitised). + // See manifest-state-machine.ts lines 99-100. + const doneDir = path.join(doneTestRoot, ".flow", "state", "done"); + await fs.mkdir(doneDir, { recursive: true }); + await atomicWriteFile( + path.join(doneDir, storyRef + ".yaml"), + `ref: "${storyRef}"\nstatus: done\n`, + ); + + // Run the scorer — it should credit the invoke via the done/ manifest even + // though NO READY FOR MERGE verdict event exists in telemetry. + const result = await computeSkillEffectiveness({ targetRepoRoot: doneTestRoot }); + + // The invoke carries storyRef as story_id; the done/ manifest for storyRef is + // present; therefore useful_fire_count must be 1 and ratio > 0. + expect(result.per_skill["flow:run"]).toEqual({ + invoke_count: 1, + useful_fire_count: 1, + effectiveness_ratio: 1, + skill_tier: "execution", + }); + // Attribution must be "attributed" — done/ signals completion. + expect(result.attribution).toBe("attributed"); + }); +}); + describe("skill-effectiveness attribution end-to-end (issue #390)", () => { it("credits a useful fire across the divergent harness/run session namespaces", async () => { // 1) REAL capture seam: a programmatic skill invocation whose session id is 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 c7409730..0a311c68 100644 --- a/plugins/flow/mcp-server/src/tools/compute-skill-effectiveness.ts +++ b/plugins/flow/mcp-server/src/tools/compute-skill-effectiveness.ts @@ -224,6 +224,19 @@ export interface ComputeSkillEffectivenessOptions { * Test seam: inject a fake file reader. Production callers do not pass this. */ readFileImpl?: (filePath: string) => Promise; + /** + * Test seam / declared dependency: inject a reader that returns the set of + * story refs present in `.flow/state/done/`. The default implementation reads + * the real `.flow/state/done/` directory — each `.yaml` filename (minus the + * extension) is a completed story ref. + * + * This is a NEW, EXPLICIT dependency (Story native:01KVS12K): a skill.invoke + * whose `story_id` is in the done set is credited as a useful fire even when + * no joined `READY FOR MERGE` reviewer.verdict event exists in telemetry — + * covering completion paths that produce no verdict event (auto-merge gate, + * operator merge, etc.). + */ + readDoneRefsImpl?: (doneDir: string) => Promise>; } // --------------------------------------------------------------------------- @@ -243,7 +256,7 @@ export interface ComputeSkillEffectivenessOptions { export async function computeSkillEffectiveness( opts: ComputeSkillEffectivenessOptions, ): Promise { - const { targetRepoRoot, window: rawWindow, readTelemetryDirImpl, readFileImpl } = opts; + const { targetRepoRoot, window: rawWindow, readTelemetryDirImpl, readFileImpl, readDoneRefsImpl } = opts; // ------------------------------------------------------------------ // Step 1: Validate window (mirrors computeAgreement's AC2c guard). @@ -357,6 +370,42 @@ export async function computeSkillEffectiveness( }); const windowedInvokes = sortedInvokes.slice(0, window); + // ------------------------------------------------------------------ + // Step 5a: Read done/ manifest refs — the second useful-fire criterion. + // + // A skill.invoke whose joined story_id is in the done set is credited as a + // useful fire even when no `READY FOR MERGE` reviewer.verdict event exists in + // telemetry. This covers completion paths that produce no verdict event (e.g. + // the auto-merge gate or an operator merge on the paused-for-human path). + // + // The done/ dir may not exist on repos without completed stories — ENOENT is + // treated as "no done refs" (same empty-result posture as the telemetry dir). + // ------------------------------------------------------------------ + const doneDir = path.join(targetRepoRoot, ".flow", "state", "done"); + let doneRefs: Set; + try { + if (readDoneRefsImpl) { + doneRefs = await readDoneRefsImpl(doneDir); + } else { + const entries = await fs.readdir(doneDir, { withFileTypes: true }); + doneRefs = new Set( + entries + .filter((e) => e.isFile() && e.name.endsWith(".yaml")) + .map((e) => e.name.slice(0, -".yaml".length)), + ); + } + } catch (err: unknown) { + if ( + err !== null && + typeof err === "object" && + (err as NodeJS.ErrnoException).code === "ENOENT" + ) { + doneRefs = new Set(); // done dir absent → no done refs (documented posture) + } else { + throw err; + } + } + // ------------------------------------------------------------------ // Step 5: Index READY FOR MERGE verdicts by session_id for the join. // Each entry holds the verdicts' (ts, story_id) so the per-invocation @@ -430,7 +479,7 @@ export async function computeSkillEffectiveness( isUseful = true; anyNonExecutionInvoke = true; } else { - // execution tier: verdict-join criterion (existing logic, unchanged). + // execution tier: verdict-join criterion + done/-manifest criterion. // Primary join: same story_id, later verdict. Fires whenever the invoke // carries a story_id matching a useful verdict's story_id. @@ -456,6 +505,14 @@ export async function computeSkillEffectiveness( return true; }); } + + // Done-manifest fallback (Story native:01KVS12K): credit the invoke as a + // useful fire when its joined story reached done/ but produced NO joined + // READY FOR MERGE verdict event — e.g. auto-merge gate or operator merge. + // Augments (does not replace) the verdict join above. + if (!isUseful && inv.story_id !== undefined) { + isUseful = doneRefs.has(inv.story_id); + } } if (isUseful) { @@ -477,12 +534,13 @@ export async function computeSkillEffectiveness( }; } - // Attribution is "attributed" when EITHER: + // Attribution is "attributed" when ANY of: // - at least one READY FOR MERGE verdict existed for execution-tier scoring, - // - OR at least one planning/cockpit-tier skill was invoked (presence-based). - // Only "no-completed-flows" when neither condition is met (truly nothing to - // attribute — a cycle with no done stories AND no planning/cockpit activity). - const isAttributed = usefulVerdictCount > 0 || anyNonExecutionInvoke; + // - OR at least one planning/cockpit-tier skill was invoked (presence-based), + // - OR at least one story reached done/ (done-manifest fallback criterion). + // Only "no-completed-flows" when none of these conditions is met (truly nothing + // to attribute — a cycle with no done stories AND no planning/cockpit activity). + const isAttributed = usefulVerdictCount > 0 || anyNonExecutionInvoke || doneRefs.size > 0; return { per_skill, From 9886e1e63608236ab2c819dfd591b72e405c5325 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Tue, 23 Jun 2026 21:34:56 +0000 Subject: [PATCH 2/2] chore(ci): rebuild dist [skip ci] --- plugins/flow/mcp-server/dist/cli.js | 25 +++++++++++++++++++++++-- plugins/flow/mcp-server/dist/index.js | 25 +++++++++++++++++++++++-- 2 files changed, 46 insertions(+), 4 deletions(-) diff --git a/plugins/flow/mcp-server/dist/cli.js b/plugins/flow/mcp-server/dist/cli.js index cc984038..cd39bdcc 100644 --- a/plugins/flow/mcp-server/dist/cli.js +++ b/plugins/flow/mcp-server/dist/cli.js @@ -28502,7 +28502,7 @@ var SkillEffectivenessResultSchema = external_exports.object({ attribution: SkillEffectivenessAttribution }).strict(); async function computeSkillEffectiveness(opts) { - const { targetRepoRoot, window: rawWindow, readTelemetryDirImpl, readFileImpl } = opts; + const { targetRepoRoot, window: rawWindow, readTelemetryDirImpl, readFileImpl, readDoneRefsImpl } = opts; const window2 = rawWindow ?? DEFAULT_SKILL_EFFECTIVENESS_WINDOW; if (!Number.isFinite(window2) || !Number.isInteger(window2) || window2 <= 0) { throw new SkillEffectivenessWindowInvalidError({ @@ -28577,6 +28577,24 @@ async function computeSkillEffectiveness(opts) { return a2.session_id < b.session_id ? -1 : a2.session_id > b.session_id ? 1 : 0; }); const windowedInvokes = sortedInvokes.slice(0, window2); + const doneDir = path11.join(targetRepoRoot, ".flow", "state", "done"); + let doneRefs; + try { + if (readDoneRefsImpl) { + doneRefs = await readDoneRefsImpl(doneDir); + } else { + const entries = await fs9.readdir(doneDir, { withFileTypes: true }); + doneRefs = new Set( + entries.filter((e) => e.isFile() && e.name.endsWith(".yaml")).map((e) => e.name.slice(0, -".yaml".length)) + ); + } + } catch (err) { + if (err !== null && typeof err === "object" && err.code === "ENOENT") { + doneRefs = /* @__PURE__ */ new Set(); + } else { + throw err; + } + } const usefulVerdictsBySession = /* @__PURE__ */ new Map(); const usefulVerdictsByStory = /* @__PURE__ */ new Map(); let usefulVerdictCount = 0; @@ -28623,6 +28641,9 @@ async function computeSkillEffectiveness(opts) { return true; }); } + if (!isUseful && inv.story_id !== void 0) { + isUseful = doneRefs.has(inv.story_id); + } } if (isUseful) { entry.useful++; @@ -28638,7 +28659,7 @@ async function computeSkillEffectiveness(opts) { skill_tier: counts.tier }; } - const isAttributed = usefulVerdictCount > 0 || anyNonExecutionInvoke; + const isAttributed = usefulVerdictCount > 0 || anyNonExecutionInvoke || doneRefs.size > 0; return { per_skill, window_size: window2, diff --git a/plugins/flow/mcp-server/dist/index.js b/plugins/flow/mcp-server/dist/index.js index 60ae97ba..9d4cad53 100644 --- a/plugins/flow/mcp-server/dist/index.js +++ b/plugins/flow/mcp-server/dist/index.js @@ -49487,7 +49487,7 @@ var SkillEffectivenessResultSchema = external_exports.object({ attribution: SkillEffectivenessAttribution }).strict(); async function computeSkillEffectiveness(opts) { - const { targetRepoRoot, window: rawWindow, readTelemetryDirImpl, readFileImpl } = opts; + const { targetRepoRoot, window: rawWindow, readTelemetryDirImpl, readFileImpl, readDoneRefsImpl } = opts; const window2 = rawWindow ?? DEFAULT_SKILL_EFFECTIVENESS_WINDOW; if (!Number.isFinite(window2) || !Number.isInteger(window2) || window2 <= 0) { throw new SkillEffectivenessWindowInvalidError({ @@ -49562,6 +49562,24 @@ async function computeSkillEffectiveness(opts) { return a2.session_id < b.session_id ? -1 : a2.session_id > b.session_id ? 1 : 0; }); const windowedInvokes = sortedInvokes.slice(0, window2); + const doneDir = path39.join(targetRepoRoot, ".flow", "state", "done"); + let doneRefs; + try { + if (readDoneRefsImpl) { + doneRefs = await readDoneRefsImpl(doneDir); + } else { + const entries = await fs32.readdir(doneDir, { withFileTypes: true }); + doneRefs = new Set( + entries.filter((e) => e.isFile() && e.name.endsWith(".yaml")).map((e) => e.name.slice(0, -".yaml".length)) + ); + } + } catch (err) { + if (err !== null && typeof err === "object" && err.code === "ENOENT") { + doneRefs = /* @__PURE__ */ new Set(); + } else { + throw err; + } + } const usefulVerdictsBySession = /* @__PURE__ */ new Map(); const usefulVerdictsByStory = /* @__PURE__ */ new Map(); let usefulVerdictCount = 0; @@ -49608,6 +49626,9 @@ async function computeSkillEffectiveness(opts) { return true; }); } + if (!isUseful && inv.story_id !== void 0) { + isUseful = doneRefs.has(inv.story_id); + } } if (isUseful) { entry.useful++; @@ -49623,7 +49644,7 @@ async function computeSkillEffectiveness(opts) { skill_tier: counts.tier }; } - const isAttributed = usefulVerdictCount > 0 || anyNonExecutionInvoke; + const isAttributed = usefulVerdictCount > 0 || anyNonExecutionInvoke || doneRefs.size > 0; return { per_skill, window_size: window2,