diff --git a/.opencode/plugins/ecc-hooks.ts b/.opencode/plugins/ecc-hooks.ts index e350adad6..86eaa7cdf 100644 --- a/.opencode/plugins/ecc-hooks.ts +++ b/.opencode/plugins/ecc-hooks.ts @@ -16,9 +16,26 @@ import type { PluginInput } from "@opencode-ai/plugin" import * as fs from "fs" import * as path from "path" +import { fileURLToPath, pathToFileURL } from "url" import changedFilesTool from "../tools/changed-files.ts" import dependencyAnalyzerTool from "../tools/dependency-analyzer.ts" +/** + * Directory of this module. + * + * The plugin is ESM: tsc compiles it to .opencode/dist/plugins/*.js and OpenCode + * loads the .ts directly, so `__dirname` is not defined there. Reading it used to + * throw inside a try/catch and silently degrade the reported ECC version to the + * default. `import.meta.url` is the ESM source of truth; the `__dirname` check is + * kept first only so a CJS consumer of this module still resolves correctly. + */ +function resolvePluginDir(): string { + if (typeof __dirname === "string" && __dirname) return __dirname + return path.dirname(fileURLToPath(import.meta.url)) +} + +const PLUGIN_DIR = resolvePluginDir() + /** * Type definitions for better type safety */ @@ -51,17 +68,84 @@ interface TodoEvent { } /** - * Read ECC version from package.json - * Falls back to a default if package.json cannot be read + * Artifacts that identify an ECC root, one per capability this plugin needs. + * + * These are required runtime files, never optional content: `skills/` belongs to + * the optional `workflow-quality` module, so probing for a skill rejected + * supported modular installs (e.g. `platform-configs` + `hooks-runtime` without + * `workflow-quality`) that still ship both the plugin and the tracker — silently + * disabling skill telemetry and the shell root export for those users. + * + * Each probe is the exact artifact the caller is about to use, so detection is + * layout detection rather than an authenticity control: a directory only + * qualifies because it contains what we are going to read. */ -function getECCVersion(): string { - try { - const packageJsonPath = path.resolve(__dirname, "../../package.json") - const packageJson = JSON.parse(fs.readFileSync(packageJsonPath, "utf-8")) - return packageJson.version || "2.0.0" - } catch { - return "2.0.0" +const ECC_ROOT_PROBES = { + // scripts/lib/utils.js is the shared library every scripts/ module requires. + root: [path.join("scripts", "lib", "utils.js")], + // What the skill-run telemetry hook imports at runtime. + skillTracker: [path.join("scripts", "hooks", "skill-run-tracker.js")], + // What the resolver embedded in commands requires once they read + // CLAUDE_PLUGIN_ROOT, and the version lookup for shell.env. + resolver: [path.join("scripts", "lib", "resolve-ecc-root.js")], +} as const + +/** + * Walk upwards from `startDir` looking for an ancestor that contains every + * relative path in `probes`. + * + * The plugin ships in three layouts — `/plugins` (installed), `/dist/plugins` + * (compiled) and `/.opencode/plugins` (repo checkout) — so the number of + * levels back to the root is not fixed. Probing beats a hardcoded `..`: a wrong + * root would make every command resolve against a directory that has no scripts. + * + * The walk is bounded to 5 levels so it cannot wander into unrelated ancestors. + * + * Returns null when no ancestor qualifies, so callers can leave the capability + * disabled instead of advertising a root that does not hold the artifact. + */ +function findEccRootDir(startDir: string, probes: readonly string[]): string | null { + let current = startDir + for (let depth = 0; depth < 5; depth += 1) { + if (probes.every((probe) => fs.existsSync(path.join(current, probe)))) { + return current + } + const parent = path.dirname(current) + if (parent === current) break + current = parent } + return null +} + +/** + * Read the ECC version from package.json. + * + * Candidates are tried in order — the resolved ECC root first, then the two + * plausible parent depths of the plugin — because the layout differs between an + * installed root (`/plugins`), the compiled bundle (`/dist/plugins`) + * and a repo checkout (`/.opencode/plugins`). Falls back to a default if + * no package.json can be read. + */ +function getECCVersion(eccRootDir?: string | null): string { + const candidates = [ + eccRootDir, + path.resolve(PLUGIN_DIR, ".."), + path.resolve(PLUGIN_DIR, "..", ".."), + ] + + for (const candidate of candidates) { + if (!candidate) continue + try { + const packageJson = JSON.parse(fs.readFileSync(path.join(candidate, "package.json"), "utf-8")) + if (typeof packageJson.version === "string" && packageJson.version) { + return packageJson.version + } + } catch { + // Try the next candidate. + } + } + + return "2.0.0" } type ECCHooksPluginFn = (input: PluginInput) => Promise> @@ -135,6 +219,60 @@ export const ECCHooksPlugin: ECCHooksPluginFn = async ({ .catch(() => {}) } + // Claude Code routes post:skill:track through + // scripts/hooks/posttooluse-dispatcher.js, which OpenCode never invokes. + // Without a caller here, nothing ever writes skill-runs.jsonl and + // `skills-health.js --dashboard` has no data to report on any skill (#2463). + // + // The tracker module is reused rather than reimplemented so its identifier + // validation, length bounds and "never persist prompt text" guarantees stay + // the ones that were written and audited for the Claude Code path. + // + // Loaded lazily, for the same reason as the store above: this file is + // OpenCode's startup entry point, and a static import of a missing module + // crashes the plugin -- and with it the whole session -- before any hook can + // run (#2530). A runtime file URL also keeps tsc from resolving a relative + // specifier that exists in no compiled layout. + type SkillRunTrackerModule = { run?: (payload: unknown) => void } + let recordSkillRun: ((payload: unknown) => void) | undefined + // Detection is scoped to the tracker itself, so a modular install that omits + // optional content still gets telemetry. + const trackerRoot = findEccRootDir(PLUGIN_DIR, ECC_ROOT_PROBES.skillTracker) + // The root advertised to commands is a different question: those commands + // require the resolver, so a root without it would break them again. + const eccRoot = + findEccRootDir(PLUGIN_DIR, ECC_ROOT_PROBES.resolver) ?? findEccRootDir(PLUGIN_DIR, ECC_ROOT_PROBES.root) + try { + const trackerPath = trackerRoot + ? path.join(trackerRoot, "scripts", "hooks", "skill-run-tracker.js") + : null + if (!trackerPath || !fs.existsSync(trackerPath)) { + throw new Error("skill-run-tracker.js not found") + } + // CJS module loaded from ESM: run() is on the namespace in Node/Bun, but a + // transpiled default export must stay supported. + const loaded = (await import(pathToFileURL(trackerPath).href)) as SkillRunTrackerModule & { + default?: SkillRunTrackerModule + } + const entry = typeof loaded.run === "function" ? loaded : loaded.default + if (!entry || typeof entry.run !== "function") { + throw new Error("skill-run-tracker.js exposes no run()") + } + recordSkillRun = entry.run + } catch { + // Same deferred-log rationale as the store above: guarantee that a + // telemetry failure can never escape this catch and abort startup. + Promise.resolve() + .then(() => + log( + "warn", + "[ECC] skill-run telemetry disabled: could not load scripts/hooks/skill-run-tracker.js. " + + "Run `ecc repair --target opencode` to restore the missing files. Other ECC hooks are unaffected." + ) + ) + .catch(() => {}) + } + const normalizeProfile = (value: string | undefined): HookProfile => { if (value === "minimal" || value === "strict") return value return "standard" @@ -264,6 +402,25 @@ export const ECCHooksPlugin: ECCHooksPluginFn = async ({ ) { log("info", "[ECC] PR created - check GitHub Actions status") } + + // Skill telemetry (#2463). OpenCode's tool is named `skill`, so the + // harness-side name is matched here (case-insensitively) and the payload + // is handed over in the shape the tracker already understands. + if ( + hookEnabled("post:skill:track", ["standard", "strict"]) && + typeof input.tool === "string" && + input.tool.trim().toLowerCase() === "skill" + ) { + try { + recordSkillRun?.({ + tool_name: "Skill", + tool_input: input.args ?? {}, + tool_response: output, + }) + } catch { + // Telemetry is best-effort; never let it affect the tool result. + } + } }, /** @@ -483,7 +640,7 @@ export const ECCHooksPlugin: ECCHooksPluginFn = async ({ */ "shell.env": async (_input: { cwd: string }, output: { env: Record }) => { const env: Record = { - ECC_VERSION: getECCVersion(), + ECC_VERSION: getECCVersion(eccRoot), ECC_PLUGIN: "true", ECC_HOOK_PROFILE: currentProfile, ECC_DISABLED_HOOKS: process.env.ECC_DISABLED_HOOKS || "", @@ -523,6 +680,25 @@ export const ECCHooksPlugin: ECCHooksPluginFn = async ({ env.PRIMARY_LANGUAGE = detected[0] } + // resolve-ecc-root.js only probes ~/.claude and Claude Code's plugin + // cache, so under OpenCode it fell back to ~/.claude and every command + // carrying the inlined resolver failed to find its scripts. That resolver + // reads CLAUDE_PLUGIN_ROOT first, so exporting the root here fixes them + // through the mechanism ECC already provides -- rather than editing the + // command files that embed the locator. + // + // Only set when a root actually exists, and never replace a root someone + // else already chose: neither the process environment (set by the user) + // nor `output.env` (set by an earlier plugin). The final merge below + // overwrites output.env, so checking it only in `env` would clobber it. + const rootAlreadySet = Boolean( + (process.env.CLAUDE_PLUGIN_ROOT || "").trim() || + (output.env?.CLAUDE_PLUGIN_ROOT || "").trim() + ) + if (eccRoot && !rootAlreadySet) { + env.CLAUDE_PLUGIN_ROOT = eccRoot + } + // OpenCode reads the supplied output object and ignores callback return values. output.env = { ...output.env, ...env } }, diff --git a/scripts/hooks/skill-run-tracker.js b/scripts/hooks/skill-run-tracker.js index eef54b859..a306245d5 100644 --- a/scripts/hooks/skill-run-tracker.js +++ b/scripts/hooks/skill-run-tracker.js @@ -16,8 +16,9 @@ * skill id is an identifier, not free text, so anything that does not look like * one is dropped rather than written through. * - * Best-effort: never blocks tool execution. Runs under the PostToolUse - * dispatcher, which owns stdin, pass-through, and exit codes. + * Best-effort: never blocks tool execution. Under Claude Code it runs via the + * PostToolUse dispatcher, which owns stdin, pass-through, and exit codes; under + * OpenCode the ecc-hooks plugin calls run() directly with an equivalent payload. * * Cross-platform (Windows, macOS, Linux); CommonJS. */ @@ -102,6 +103,28 @@ function deriveOutcome(payload) { return 'success'; } +const SKILL_TOOL_NAME = 'skill'; + +/** + * Report whether a hook payload describes a skill tool invocation. + * + * Claude Code names the tool `Skill`; OpenCode and other harnesses name it + * `skill`. Comparing the raw string meant a case difference silently disabled + * the whole write side again — the hook ran, dropped every event, and the + * dashboard kept reporting unmeasured skills with no hint why. + * + * @param {{tool_name?: unknown} | null} payload + * @returns {boolean} + */ +function isSkillToolCall(payload) { + if (!payload || typeof payload !== 'object') { + return false; + } + + const toolName = payload.tool_name; + return typeof toolName === 'string' && toolName.trim().toLowerCase() === SKILL_TOOL_NAME; +} + function buildRecord(payload) { const skillId = extractSkillId(payload.tool_input); if (!skillId) { @@ -130,12 +153,22 @@ function buildRecord(payload) { }; } +/** + * Record a skill invocation from a hook payload. + * + * Accepts the raw JSON string Claude Code's dispatcher reads from stdin, or an + * already-parsed payload for in-process callers such as the OpenCode plugin. + * Non-skill tools and unparsable input are ignored. + * + * @param {string | object | null | undefined} rawInput + * @returns {void} + */ function run(rawInput) { try { const payload = typeof rawInput === 'string' ? (rawInput.trim() ? JSON.parse(rawInput) : {}) : rawInput; - if (payload && typeof payload === 'object' && payload.tool_name === 'Skill') { + if (isSkillToolCall(payload)) { const record = buildRecord(payload); if (record) { recordSkillExecution(record); @@ -146,4 +179,4 @@ function run(rawInput) { } } -module.exports = { buildRecord, deriveOutcome, extractSkillId, run }; +module.exports = { buildRecord, deriveOutcome, extractSkillId, isSkillToolCall, run }; diff --git a/scripts/lib/skill-evolution/dashboard.js b/scripts/lib/skill-evolution/dashboard.js index 7049707bd..edd98ce74 100644 --- a/scripts/lib/skill-evolution/dashboard.js +++ b/scripts/lib/skill-evolution/dashboard.js @@ -328,6 +328,18 @@ function renderVersionTimelinePanel(skillsById, options = {}) { }; } +/** + * Render the full skill-health dashboard: header, coverage warning and panels. + * + * Coverage is stated explicitly because the 30-day window can empty out while + * runs are still retained, so "unmeasured" must not be reported as "never + * recorded" (#2463). + * + * @param {object} [options] + * @param {string} [options.now] ISO timestamp used as the evaluation instant + * @param {string} [options.panel] render only this panel + * @returns {{text: string, data: object}} + */ function renderDashboard(options = {}) { const now = options.now || new Date().toISOString(); const nowMs = Date.parse(now); @@ -359,12 +371,31 @@ function renderDashboard(options = {}) { const header = [ 'ECC Skill Health Dashboard', `Generated: ${now}`, - `Skills: ${summary.total_skills} total, ${summary.healthy_skills} healthy, ${summary.declining_skills} declining`, + `Skills: ${summary.total_skills} total, ${summary.measured_skills} measured, ${summary.healthy_skills} healthy, ${summary.declining_skills} declining, ${summary.unmeasured_skills} unmeasured`, '', ]; textParts.push(header.join('\n')); + if (summary.unmeasured_skills > 0) { + // The window can empty out while runs are still retained, so "unmeasured" + // must not be reported as "never recorded" (#2463). Three distinct states, + // because "no recent runs" alone does not say which one applies. + const recentRuns = health.filterRecordsWithinDays(records, nowMs, 30).length; + const coverageNote = records.length === 0 + ? 'Nothing has ever been recorded, so the panels below are not a signal about skill health.' + : recentRuns === 0 + ? `All ${records.length} retained run${records.length === 1 ? '' : 's'} are older than the 30-day window: coverage aged out rather than never existing. The panels below still show that history.` + : `Coverage is partial: ${recentRuns} recent run${recentRuns === 1 ? '' : 's'} inside the window, ${records.length - recentRuns} older one${records.length - recentRuns === 1 ? '' : 's'} retained outside it. Only the measured skills can be judged.`; + + textParts.push([ + `WARNING: ${summary.unmeasured_skills} of ${summary.total_skills} skills have no recorded run in the last 30 days.`, + 'This dashboard can say nothing about them: no telemetry is an absence of evidence,', + 'not a healthy result. Their success rate reads "n/a", which means unknown.', + coverageNote, + ].join('\n')); + } + if (selectedPanel) { const result = panelRenderers[selectedPanel](); panels[selectedPanel] = result.data; diff --git a/scripts/lib/skill-evolution/health.js b/scripts/lib/skill-evolution/health.js index 309f16094..c57e23354 100644 --- a/scripts/lib/skill-evolution/health.js +++ b/scripts/lib/skill-evolution/health.js @@ -26,15 +26,36 @@ function formatRate(value) { return `${Math.round(value * 100)}%`; } +/** + * Summarise a health report into counting buckets. + * + * A skill with no runs in the window is not healthy, it is unmeasured: the + * trend helpers return 'stable' when both rates are null, so deriving health + * from "not declining" turned a missing telemetry writer into a green + * dashboard across every skill (#2463). A declining skill always has runs, so + * the buckets partition the total. + * + * @param {{skills: Array<{declining?: boolean, run_count_30d?: number}>}} report + * @returns {{total_skills: number, measured_skills: number, healthy_skills: number, declining_skills: number, unmeasured_skills: number}} + */ function summarizeHealthReport(report) { const totalSkills = report.skills.length; + // A skill with no runs in the window is not healthy, it is unmeasured. + // Folding it into "healthy" turned a missing telemetry writer into a green + // dashboard across every skill (#2463) — false confidence that never triggers + // an alert. A declining skill always has runs, so the buckets partition the + // total. + const unmeasuredSkills = report.skills.filter(skill => !skill.run_count_30d).length; const decliningSkills = report.skills.filter(skill => skill.declining).length; - const healthySkills = totalSkills - decliningSkills; + const measuredSkills = totalSkills - unmeasuredSkills; + const healthySkills = measuredSkills - decliningSkills; return { total_skills: totalSkills, + measured_skills: measuredSkills, healthy_skills: healthySkills, declining_skills: decliningSkills, + unmeasured_skills: unmeasuredSkills, }; } @@ -230,7 +251,7 @@ function formatHealthReport(report, options = {}) { const lines = [ 'ECC skill health', `Generated: ${report.generated_at}`, - `Skills: ${summary.total_skills} total, ${summary.healthy_skills} healthy, ${summary.declining_skills} declining`, + `Skills: ${summary.total_skills} total, ${summary.measured_skills} measured, ${summary.healthy_skills} healthy, ${summary.declining_skills} declining, ${summary.unmeasured_skills} unmeasured`, '', 'skill version 7d 30d trend pending last run', '--------------------------------------------------------------------------', diff --git a/tests/hooks/skill-run-tracker.test.js b/tests/hooks/skill-run-tracker.test.js index d3e63e247..f836ab247 100644 --- a/tests/hooks/skill-run-tracker.test.js +++ b/tests/hooks/skill-run-tracker.test.js @@ -15,7 +15,7 @@ const fs = require('fs'); const os = require('os'); const path = require('path'); -const { buildRecord, deriveOutcome, extractSkillId, run } = require('../../scripts/hooks/skill-run-tracker'); +const { buildRecord, deriveOutcome, extractSkillId, isSkillToolCall, run } = require('../../scripts/hooks/skill-run-tracker'); const { readHooksConfig } = require('../../scripts/lib/hooks-config'); const { MAX_RUN_RECORDS, @@ -152,6 +152,57 @@ test('run ignores non-Skill tools and malformed input without throwing', () => { assert.doesNotThrow(() => run('')); }); +// ── tool-name tolerance ─────────────────────────────────────────────────────── +// OpenCode names the tool `skill`. A case-sensitive matcher meant the hook ran, +// dropped every event, and the dashboard kept reporting "unmeasured" with no +// indication that a writer existed at all. + +test('isSkillToolCall accepts both harness spellings and rejects everything else', () => { + assert.strictEqual(isSkillToolCall({ tool_name: 'Skill' }), true); + assert.strictEqual(isSkillToolCall({ tool_name: 'skill' }), true); + assert.strictEqual(isSkillToolCall({ tool_name: ' SKILL ' }), true); + assert.strictEqual(isSkillToolCall({ tool_name: 'Bash' }), false); + assert.strictEqual(isSkillToolCall({ tool_name: '' }), false); + assert.strictEqual(isSkillToolCall({ tool_name: 42 }), false); + assert.strictEqual(isSkillToolCall({}), false); + assert.strictEqual(isSkillToolCall(null), false); +}); + +test('run records a run for a lowercase skill tool name', () => { + withTempHome(homeDir => { + const previousHome = process.env.HOME; + const previousUserProfile = process.env.USERPROFILE; + process.env.HOME = homeDir; + process.env.USERPROFILE = homeDir; + try { + run(payload({ tool_name: 'skill' })); + + const records = readSkillExecutionRecords({ homeDir }); + assert.strictEqual(records.length, 1); + assert.strictEqual(records[0].skill_id, 'code-review'); + } finally { + if (previousHome === undefined) delete process.env.HOME; else process.env.HOME = previousHome; + if (previousUserProfile === undefined) delete process.env.USERPROFILE; else process.env.USERPROFILE = previousUserProfile; + } + }); +}); + +test('run still records nothing for a non-skill tool', () => { + withTempHome(homeDir => { + const previousHome = process.env.HOME; + const previousUserProfile = process.env.USERPROFILE; + process.env.HOME = homeDir; + process.env.USERPROFILE = homeDir; + try { + run(payload({ tool_name: 'bash' })); + assert.strictEqual(readSkillExecutionRecords({ homeDir }).length, 0); + } finally { + if (previousHome === undefined) delete process.env.HOME; else process.env.HOME = previousHome; + if (previousUserProfile === undefined) delete process.env.USERPROFILE; else process.env.USERPROFILE = previousUserProfile; + } + }); +}); + // ── JSONL sink bounds ──────────────────────────────────────────────────────── test('recordSkillExecution writes the sink owner-only', function () { diff --git a/tests/lib/skill-dashboard.test.js b/tests/lib/skill-dashboard.test.js index f90052e85..d1f29d0df 100644 --- a/tests/lib/skill-dashboard.test.js +++ b/tests/lib/skill-dashboard.test.js @@ -358,6 +358,179 @@ function runTests() { }, /Unknown panel/); })) passed++; else failed++; + // A panel that reports every never-used skill as healthy is worse than an + // empty one: it looks like evidence and never raises an alert (#2463). + if (test('renderDashboard warns that unmeasured skills say nothing about health', () => { + const result = dashboard.renderDashboard({ + skillsRoot, + learnedRoot, + importedRoot, + homeDir, + runsFilePath: path.join(homeDir, '.claude', 'state', 'no-such-runs.jsonl'), + now, + warnThreshold: 0.1, + }); + + const summary = result.data.summary; + assert.ok(summary.unmeasured_skills > 0, 'expected unmeasured skills'); + assert.ok( + result.text.includes('WARNING'), + 'expected an explicit warning when skills have no telemetry' + ); + assert.match(result.text, /unmeasured/); + assert.strictEqual( + summary.healthy_skills, + 0, + 'skills without runs must not be counted as healthy' + ); + })) passed++; else failed++; + + // Isolated roots so the full-coverage case is guaranteed rather than + // conditional: every discovered skill needs a run inside the 30-day window. + if (test('renderDashboard omits the warning once every skill is measured', () => { + const coveredRoot = createTempDir('ecc-dashboard-covered-'); + const coveredLearned = createTempDir('ecc-dashboard-covered-'); + const coveredImported = createTempDir('ecc-dashboard-covered-'); + const coveredRuns = path.join(homeDir, '.claude', 'state', 'covered-skill-runs.jsonl'); + try { + for (const name of ['covered-a', 'covered-b']) { + createSkill(coveredRoot, name, `# ${name}\n`); + } + appendJsonl(coveredRuns, ['covered-a', 'covered-b'].map(skill_id => ({ + skill_id, + skill_version: 'v1', + task_description: `Skill invocation: ${skill_id}`, + outcome: 'success', + recorded_at: now, + }))); + + const result = dashboard.renderDashboard({ + skillsRoot: coveredRoot, + learnedRoot: coveredLearned, + importedRoot: coveredImported, + homeDir, + runsFilePath: coveredRuns, + now, + warnThreshold: 0.1, + }); + + assert.strictEqual(result.data.summary.unmeasured_skills, 0); + assert.strictEqual(result.data.summary.measured_skills, 2); + assert.ok( + !result.text.includes('WARNING'), + 'no warning expected when every skill has a run in the window' + ); + assert.strictEqual( + result.data.summary.total_skills, + result.data.summary.measured_skills + result.data.summary.unmeasured_skills, + 'measured and unmeasured must partition the total' + ); + } finally { + cleanupTempDir(coveredRoot); + cleanupTempDir(coveredLearned); + cleanupTempDir(coveredImported); + } + })) passed++; else failed++; + + // The 30-day window can empty out while runs are still retained. Saying + // "nothing has ever been recorded" then is factually wrong. + if (test('the unmeasured warning distinguishes aged-out coverage from no history', () => { + const agedRuns = path.join(homeDir, '.claude', 'state', 'aged-skill-runs.jsonl'); + const agedOut = new Date(Date.parse(now) - (60 * 24 * 60 * 60 * 1000)).toISOString(); + appendJsonl(agedRuns, [{ + skill_id: 'alpha', + skill_version: 'v1', + task_description: 'Skill invocation: alpha', + outcome: 'failure', + recorded_at: agedOut, + }]); + + const result = dashboard.renderDashboard({ + skillsRoot, + learnedRoot, + importedRoot, + homeDir, + runsFilePath: agedRuns, + now, + warnThreshold: 0.1, + }); + + assert.ok(result.data.summary.unmeasured_skills > 0, 'expected unmeasured skills'); + assert.ok(result.text.includes('WARNING'), 'expected the warning'); + assert.ok( + result.text.includes('aged out'), + 'retained history outside the window must be described as aged out' + ); + assert.ok( + !result.text.includes('Nothing has ever been recorded'), + 'must not claim there is no history when runs are retained' + ); + })) passed++; else failed++; + + // Mixed coverage is the common case: some skills used recently, others not. +// It must not be described as aged out, nor as having no history at all. + if (test('the unmeasured warning describes partial coverage as partial', () => { + const mixedRuns = path.join(homeDir, '.claude', 'state', 'mixed-skill-runs.jsonl'); + appendJsonl(mixedRuns, [ + { + skill_id: 'alpha', + skill_version: 'v1', + task_description: 'Skill invocation: alpha', + outcome: 'success', + recorded_at: now, + }, + { + skill_id: 'beta', + skill_version: 'v1', + task_description: 'Skill invocation: beta', + outcome: 'success', + recorded_at: new Date(Date.parse(now) - (60 * 24 * 60 * 60 * 1000)).toISOString(), + }, + ]); + + const result = dashboard.renderDashboard({ + skillsRoot, + learnedRoot, + importedRoot, + homeDir, + runsFilePath: mixedRuns, + now, + warnThreshold: 0.1, + }); + + assert.ok(result.text.includes('WARNING'), 'expected the warning'); + assert.ok( + result.text.includes('Coverage is partial'), + 'mixed recent and aged runs must be reported as partial coverage' + ); + assert.ok( + !result.text.includes('aged out'), + 'coverage is not aged out while recent runs exist' + ); + assert.ok( + !result.text.includes('Nothing has ever been recorded'), + 'runs are retained, so history is not missing' + ); + })) passed++; else failed++; + + // With no runs at all the stronger statement is the accurate one. + if (test('the unmeasured warning says nothing was ever recorded when there is no history', () => { + const result = dashboard.renderDashboard({ + skillsRoot, + learnedRoot, + importedRoot, + homeDir, + runsFilePath: path.join(homeDir, '.claude', 'state', 'definitely-absent.jsonl'), + now, + warnThreshold: 0.1, + }); + + assert.ok( + result.text.includes('Nothing has ever been recorded'), + 'an empty record set must be reported as missing telemetry' + ); + })) passed++; else failed++; + console.log('\nCLI integration:'); if (test('CLI --dashboard --json returns valid JSON with all panels', () => { diff --git a/tests/lib/skill-evolution.test.js b/tests/lib/skill-evolution.test.js index bfff437bf..f2dd4e6ec 100644 --- a/tests/lib/skill-evolution.test.js +++ b/tests/lib/skill-evolution.test.js @@ -559,19 +559,67 @@ function runTests() { const summary = health.summarizeHealthReport(report); assert.deepStrictEqual(summary, { total_skills: 6, - healthy_skills: 5, + measured_skills: 2, + healthy_skills: 1, declining_skills: 1, + unmeasured_skills: 4, }); const human = health.formatHealthReport(report, { json: false }); assert.match(human, /alpha/); assert.match(human, /worsening/); - assert.match( - human, - new RegExp(`Skills: ${summary.total_skills} total, ${summary.healthy_skills} healthy, ${summary.declining_skills} declining`) + // Built from values rather than interpolated into a RegExp: the numbers are + // internally generated, but a literal comparison needs no pattern engine. + assert.ok( + human.includes( + `Skills: ${summary.total_skills} total, ${summary.measured_skills} measured, ` + + `${summary.healthy_skills} healthy, ${summary.declining_skills} declining, ` + + `${summary.unmeasured_skills} unmeasured` + ), + 'summary line must report measured and unmeasured counts' ); })) passed++; else failed++; + if (test('does not report unmeasured skills as healthy (#2463)', () => { + // A runs file that does not exist: skills are still discovered, so this + // is exactly the "discovered but never executed" state. + const emptyReport = health.collectSkillHealth({ + repoRoot, + homeDir, + runsFilePath: path.join(homeDir, '.claude', 'state', 'no-such-runs.jsonl'), + now, + warnThreshold: 0.1, + }); + + const summary = health.summarizeHealthReport(emptyReport); + assert.ok(summary.total_skills > 0, 'expected discovered skills'); + assert.strictEqual(summary.total_skills, summary.unmeasured_skills); + assert.strictEqual(summary.measured_skills, 0); + assert.strictEqual(summary.declining_skills, 0); + assert.strictEqual( + summary.healthy_skills, + 0, + 'a skill with no runs is unmeasured, never healthy' + ); + + // Healthy + declining must account for every measured skill, so a partial + // coverage state cannot over-report. + const partial = health.summarizeHealthReport({ + skills: [ + { skill_id: 'a', run_count_30d: 4, declining: false }, + { skill_id: 'b', run_count_30d: 2, declining: true }, + { skill_id: 'c', run_count_30d: 0, declining: false }, + ], + }); + assert.deepStrictEqual(partial, { + total_skills: 3, + measured_skills: 2, + healthy_skills: 1, + declining_skills: 1, + unmeasured_skills: 1, + }); + })) passed++; else failed++; + if (test('treats an unsnapshotted SKILL.md as v1 and orders last_run by actual time', () => { const gammaSkillDir = createSkill(skillsRoot, 'gamma', '# Gamma v1\n'); const offsetRunsFile = path.join(homeDir, '.claude', 'state', 'offset-skill-runs.jsonl'); diff --git a/tests/opencode-plugin-hooks.test.js b/tests/opencode-plugin-hooks.test.js index a6c3ed2ae..e327f6ebf 100644 --- a/tests/opencode-plugin-hooks.test.js +++ b/tests/opencode-plugin-hooks.test.js @@ -79,6 +79,90 @@ async function withTempProject(files, fn) { } } +/** + * Build a fake ECC installation root and copy the compiled plugin into it. + * + * Reproduces a *modular* install: the runtime scripts the plugin needs are + * present, but `skills/` is absent because that directory belongs to the optional + * `workflow-quality` module. `options.withSkills` adds it back so the same helper + * can prove the difference is what matters. + * + * @param {{withSkills?: boolean}} [options] + * @returns {string} path to the fake installation root + */ +function createModularInstallRoot(options = {}) { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "ecc-opencode-install-")) + const distSource = path.join(__dirname, "..", ".opencode", "dist") + const repoRoot = path.join(__dirname, "..") + + for (const dir of ["plugins", "tools"]) { + fs.cpSync(path.join(distSource, dir), path.join(root, dir), { recursive: true }) + } + + // The runtime files an install without `workflow-quality` still ships. Copied + // whole rather than file-by-file: scripts/lib has transitive requires (utils.js + // alone pulls in agent-data-home), and a partial copy would fail the import for + // reasons unrelated to what this test is about. + for (const dir of ["hooks", "lib"]) { + fs.cpSync(path.join(repoRoot, "scripts", dir), path.join(root, "scripts", dir), { + recursive: true, + }) + } + + fs.writeFileSync( + path.join(root, "package.json"), + JSON.stringify({ name: "ecc-universal", version: "0.0.0-test" }) + ) + + // The plugin's tools import @opencode-ai/plugin, which resolves through + // node_modules — a real installation has it, an isolated temp dir would not. + fs.symlinkSync(path.join(repoRoot, "node_modules"), path.join(root, "node_modules"), "junction") + + if (options.withSkills) { + fs.mkdirSync(path.join(root, "skills", "continuous-learning-v2"), { recursive: true }) + fs.writeFileSync(path.join(root, "skills", "continuous-learning-v2", "SKILL.md"), "# sentinel\n") + } + + return root +} + +/** Load the compiled plugin from a fake install root instead of the repo dist. */ +async function loadPluginFrom(root) { + return import(pathToFileURL(path.join(root, "plugins", "ecc-hooks.js")).href) +} + +/** + * Restore environment variables saved with Object.fromEntries of name -> value. + * + * @param {Record} saved + * @returns {void} + */ +function restoreEnv(saved) { + for (const [key, value] of Object.entries(saved)) { + if (value === undefined) delete process.env[key] + else process.env[key] = value + } +} + +/** + * Read the skill-runs sink the tracker wrote under `homeDir`. + * + * The tracker resolves the sink from the *current* home, so the telemetry tests + * move HOME/USERPROFILE to a temp dir and read from there. + * + * @param {string} homeDir + * @returns {Array>} + */ +function readRecordedRuns(homeDir) { + const runsFilePath = path.join(homeDir, ".claude", "state", "skill-runs.jsonl") + if (!fs.existsSync(runsFilePath)) return [] + return fs + .readFileSync(runsFilePath, "utf8") + .split("\n") + .filter(Boolean) + .map((line) => JSON.parse(line)) +} + async function main() { console.log("\n=== Testing OpenCode plugin hooks ===\n") @@ -381,6 +465,232 @@ async function main() { } ), ], + [ + "skill telemetry records a run through the shared tracker (#2463)", + async () => withTempProject([], async (projectDir) => { + const homeDir = fs.mkdtempSync(path.join(os.tmpdir(), "ecc-opencode-skill-home-")) + const previousHome = process.env.HOME + const previousUserProfile = process.env.USERPROFILE + process.env.HOME = homeDir + process.env.USERPROFILE = homeDir + try { + const client = createClient() + const $ = createFailingShell() + const hooks = await ECCHooksPlugin({ client, $, directory: projectDir }) + + assert.ok( + !client.logs.some( + (entry) => + entry.level === "warn" && entry.message.includes("skill-run telemetry disabled") + ), + "Did not expect telemetry to be disabled when scripts/hooks/skill-run-tracker.js is present" + ) + + // OpenCode names the tool `skill`; the id arrives on `name`. + await hooks["tool.execute.after"]({ tool: "skill", args: { name: "code-review" } }, {}) + + const records = readRecordedRuns(homeDir) + + assert.strictEqual(records.length, 1, "Expected exactly one recorded skill run") + assert.strictEqual(records[0].skill_id, "code-review") + assert.strictEqual(records[0].outcome, "success") + } finally { + restoreEnv({ HOME: previousHome, USERPROFILE: previousUserProfile }) + fs.rmSync(homeDir, { recursive: true, force: true }) + } + }), + ], + [ + "skill telemetry records nothing for other tools", + async () => withTempProject([], async (projectDir) => { + const homeDir = fs.mkdtempSync(path.join(os.tmpdir(), "ecc-opencode-skill-home-")) + const previousHome = process.env.HOME + const previousUserProfile = process.env.USERPROFILE + process.env.HOME = homeDir + process.env.USERPROFILE = homeDir + try { + const client = createClient() + const $ = createFailingShell() + const hooks = await ECCHooksPlugin({ client, $, directory: projectDir }) + + await hooks["tool.execute.after"]({ tool: "edit", args: { path: "src/a.ts" } }, {}) + await hooks["tool.execute.after"]({ tool: "bash", args: { command: "ls" } }, {}) + + assert.strictEqual(readRecordedRuns(homeDir).length, 0) + } finally { + restoreEnv({ HOME: previousHome, USERPROFILE: previousUserProfile }) + fs.rmSync(homeDir, { recursive: true, force: true }) + } + }), + ], + [ + "skill telemetry respects the minimal hook profile", + async () => withTempProject([], async (projectDir) => { + const homeDir = fs.mkdtempSync(path.join(os.tmpdir(), "ecc-opencode-skill-home-")) + const previousHome = process.env.HOME + const previousUserProfile = process.env.USERPROFILE + const previousProfile = process.env.ECC_HOOK_PROFILE + process.env.HOME = homeDir + process.env.USERPROFILE = homeDir + process.env.ECC_HOOK_PROFILE = "minimal" + try { + const client = createClient() + const $ = createFailingShell() + const hooks = await ECCHooksPlugin({ client, $, directory: projectDir }) + + await hooks["tool.execute.after"]({ tool: "skill", args: { name: "code-review" } }, {}) + + assert.strictEqual(readRecordedRuns(homeDir).length, 0) + } finally { + restoreEnv({ + HOME: previousHome, + USERPROFILE: previousUserProfile, + ECC_HOOK_PROFILE: previousProfile, + }) + fs.rmSync(homeDir, { recursive: true, force: true }) + } + }), + ], + [ + "shell.env exports CLAUDE_PLUGIN_ROOT so commands can resolve the ECC root", + async () => withTempProject([], async (projectDir) => { + const previousRoot = process.env.CLAUDE_PLUGIN_ROOT + delete process.env.CLAUDE_PLUGIN_ROOT + try { + const client = createClient() + const $ = createFailingShell() + const hooks = await ECCHooksPlugin({ client, $, directory: projectDir }) + + const output = { env: {} } + await hooks["shell.env"]({ cwd: projectDir }, output) + + assert.deepStrictEqual($.calls, [], `Unexpected shell probes: ${$.calls.join(", ")}`) + assert.ok(output.env.CLAUDE_PLUGIN_ROOT, "Expected CLAUDE_PLUGIN_ROOT to be exported") + + // The resolver consumes this variable verbatim, so it must be a + // directory that actually carries the ECC script tree. + const root = output.env.CLAUDE_PLUGIN_ROOT + assert.ok( + fs.existsSync(path.join(root, "scripts", "lib", "resolve-ecc-root.js")), + `Expected ${root} to contain scripts/lib/resolve-ecc-root.js` + ) + } finally { + restoreEnv({ CLAUDE_PLUGIN_ROOT: previousRoot }) + } + }), + ], + [ + "shell.env does not override a CLAUDE_PLUGIN_ROOT the user set", + async () => withTempProject([], async (projectDir) => { + const explicitRoot = path.join(projectDir, "explicit-root") + const previousRoot = process.env.CLAUDE_PLUGIN_ROOT + process.env.CLAUDE_PLUGIN_ROOT = explicitRoot + try { + const client = createClient() + const $ = createFailingShell() + const hooks = await ECCHooksPlugin({ client, $, directory: projectDir }) + + const output = { env: {} } + await hooks["shell.env"]({ cwd: projectDir }, output) + + assert.strictEqual( + output.env.CLAUDE_PLUGIN_ROOT, + undefined, + "the plugin must not inject its own root over one the user set" + ) + assert.strictEqual( + process.env.CLAUDE_PLUGIN_ROOT, + explicitRoot, + "the user's value must survive untouched" + ) + } finally { + restoreEnv({ CLAUDE_PLUGIN_ROOT: previousRoot }) + } + }), + ], + [ + "shell.env does not clobber a root another plugin already put in output.env", + async () => withTempProject([], async (projectDir) => { + const previousRoot = process.env.CLAUDE_PLUGIN_ROOT + delete process.env.CLAUDE_PLUGIN_ROOT + try { + const client = createClient() + const $ = createFailingShell() + const hooks = await ECCHooksPlugin({ client, $, directory: projectDir }) + + // An earlier plugin chose a root. The final merge below spreads our env + // over output.env, so a guard that only checked process.env would + // replace this value. + const output = { env: { CLAUDE_PLUGIN_ROOT: path.join(projectDir, "other-plugin-root") } } + await hooks["shell.env"]({ cwd: projectDir }, output) + + assert.strictEqual( + output.env.CLAUDE_PLUGIN_ROOT, + path.join(projectDir, "other-plugin-root"), + "an existing output.env root must be preserved" + ) + } finally { + restoreEnv({ CLAUDE_PLUGIN_ROOT: previousRoot }) + } + }), + ], + [ + "telemetry and root export work in a modular install without optional skills", + async () => { + // Regression: root detection probed skills/continuous-learning-v2, which + // belongs to the optional workflow-quality module. An install with + // platform-configs + hooks-runtime but without workflow-quality still + // ships the plugin and the tracker, and it lost both fixes. + const root = createModularInstallRoot() + const projectDir = fs.mkdtempSync(path.join(os.tmpdir(), "ecc-opencode-modular-")) + const homeDir = fs.mkdtempSync(path.join(os.tmpdir(), "ecc-opencode-skill-home-")) + const previousHome = process.env.HOME + const previousUserProfile = process.env.USERPROFILE + const previousRoot = process.env.CLAUDE_PLUGIN_ROOT + process.env.HOME = homeDir + process.env.USERPROFILE = homeDir + delete process.env.CLAUDE_PLUGIN_ROOT + try { + assert.ok( + !fs.existsSync(path.join(root, "skills")), + "the fixture must not contain skills/, or it would not test the modular case" + ) + + const { ECCHooksPlugin: modularPlugin } = await loadPluginFrom(root) + const client = createClient() + const hooks = await modularPlugin({ + client, + $: createFailingShell(), + directory: projectDir, + }) + + assert.ok( + !client.logs.some( + (entry) => + entry.level === "warn" && entry.message.includes("skill-run telemetry disabled") + ), + "telemetry must stay enabled when the optional skills module is absent" + ) + + await hooks["tool.execute.after"]({ tool: "skill", args: { name: "code-review" } }, {}) + assert.strictEqual(readRecordedRuns(homeDir).length, 1, "expected the run to be recorded") + + const output = { env: {} } + await hooks["shell.env"]({ cwd: projectDir }, output) + assert.strictEqual(output.env.CLAUDE_PLUGIN_ROOT, root) + assert.strictEqual(output.env.ECC_VERSION, "0.0.0-test") + } finally { + restoreEnv({ + HOME: previousHome, + USERPROFILE: previousUserProfile, + CLAUDE_PLUGIN_ROOT: previousRoot, + }) + fs.rmSync(root, { recursive: true, force: true }) + fs.rmSync(projectDir, { recursive: true, force: true }) + fs.rmSync(homeDir, { recursive: true, force: true }) + } + }, + ], ] let passed = 0