From d6551bfef950861cacda9ec5b87d8ce8915911c1 Mon Sep 17 00:00:00 2001 From: Nick Nisi Date: Thu, 23 Apr 2026 20:41:15 -0700 Subject: [PATCH 01/10] feat: surface WorkOS skill auto-install and track version autoInstallSkills now returns a summary describing which skills were installed to which agents. handleInstall prints a clack info line in TTY mode so users know their coding agent has up-to-date WorkOS guidance. Suppressed in JSON mode. Also writes a per-agent .workos-skill-version marker alongside installed skills so downstream checks can detect staleness. --- src/commands/install-skill.spec.ts | 57 +++++++++++++++++------ src/commands/install-skill.ts | 74 +++++++++++++++++++++++++----- src/commands/install.spec.ts | 40 +++++++++++++++- src/commands/install.ts | 8 +++- 4 files changed, 152 insertions(+), 27 deletions(-) diff --git a/src/commands/install-skill.spec.ts b/src/commands/install-skill.spec.ts index 9b9d11e2..4af36e2d 100644 --- a/src/commands/install-skill.spec.ts +++ b/src/commands/install-skill.spec.ts @@ -225,7 +225,7 @@ describe('install-skill', () => { vi.restoreAllMocks(); }); - it('installs all skills to all detected agents', async () => { + it('installs all skills to all detected agents and returns a summary', async () => { // Set up skills mkdirSync(join(skillsDir, 'skill-a')); writeFileSync(join(skillsDir, 'skill-a', 'SKILL.md'), '# Skill A'); @@ -236,51 +236,80 @@ describe('install-skill', () => { mkdirSync(join(homeDir, '.claude')); mkdirSync(join(homeDir, '.codex')); - await autoInstallSkills(); + const result = await autoInstallSkills(); expect(existsSync(join(homeDir, '.claude/skills/skill-a/SKILL.md'))).toBe(true); expect(existsSync(join(homeDir, '.claude/skills/skill-b/SKILL.md'))).toBe(true); expect(existsSync(join(homeDir, '.codex/skills/skill-a/SKILL.md'))).toBe(true); expect(existsSync(join(homeDir, '.codex/skills/skill-b/SKILL.md'))).toBe(true); + + expect(result).not.toBeNull(); + expect(result!.skills.sort()).toEqual(['skill-a', 'skill-b']); + expect(result!.agents.sort()).toEqual(['Claude Code', 'Codex']); + }); + + it('writes a version marker per agent when the bundled version is resolvable', async () => { + // The real @workos/skills package is present in node_modules, so + // getBundledSkillsVersion should succeed against the real skillsDir layout. + // Here we just verify that IF a version can be determined, it gets written. + const { SKILL_VERSION_MARKER_FILENAME } = await import('./install-skill.js'); + + mkdirSync(join(skillsDir, 'skill-a')); + writeFileSync(join(skillsDir, 'skill-a', 'SKILL.md'), '# Skill A'); + mkdirSync(join(homeDir, '.claude')); + + const result = await autoInstallSkills(); + + // Our test skillsDir is not a real npm package, so bundled version is null. + // That's fine — the marker is only written when version is resolvable, + // and result.version reflects what was written. + if (result?.version) { + const marker = join(homeDir, '.claude/skills', SKILL_VERSION_MARKER_FILENAME); + expect(existsSync(marker)).toBe(true); + expect(readFileSync(marker, 'utf8')).toBe(result.version); + } else { + // Marker should NOT be written when version is unknown. + const marker = join(homeDir, '.claude/skills', SKILL_VERSION_MARKER_FILENAME); + expect(existsSync(marker)).toBe(false); + } }); - it('no-ops silently when no agents are detected', async () => { + it('returns null when no agents are detected', async () => { mkdirSync(join(skillsDir, 'skill-a')); writeFileSync(join(skillsDir, 'skill-a', 'SKILL.md'), '# Skill A'); // No agent directories created — none detected - await expect(autoInstallSkills()).resolves.toBeUndefined(); + await expect(autoInstallSkills()).resolves.toBeNull(); }); - it('no-ops silently when no skills are discovered', async () => { + it('returns null when no skills are discovered', async () => { mkdirSync(join(homeDir, '.claude')); // No skills in skillsDir - await expect(autoInstallSkills()).resolves.toBeUndefined(); + await expect(autoInstallSkills()).resolves.toBeNull(); }); - it('swallows errors from discoverSkills', async () => { + it('swallows errors from discoverSkills and returns null', async () => { // Point to a nonexistent skills directory const { getSkillsDir } = await import('@workos/skills'); vi.mocked(getSkillsDir).mockReturnValue('/nonexistent/path'); - await expect(autoInstallSkills()).resolves.toBeUndefined(); + await expect(autoInstallSkills()).resolves.toBeNull(); }); - it('resolves silently when installSkill returns failure', async () => { + it('returns null when every installSkill call fails', async () => { // installSkill returns { success: false } on copy errors (doesn't throw). - // Verify autoInstallSkills completes without throwing even when installs fail. - // Simulate by creating a skill dir with SKILL.md for discovery, then making - // the target agent dir read-only so copyFile fails. + // When nothing succeeded, callers should get null so they don't advertise + // a bogus "installed" message. mkdirSync(join(skillsDir, 'test-skill')); writeFileSync(join(skillsDir, 'test-skill', 'SKILL.md'), '# Test'); mkdirSync(join(homeDir, '.claude')); - // Create a file where the skills directory should be, so mkdir fails + // Create a file where the skill subdir should be, so mkdir fails mkdirSync(join(homeDir, '.claude/skills')); writeFileSync(join(homeDir, '.claude/skills/test-skill'), 'not a directory'); - await expect(autoInstallSkills()).resolves.toBeUndefined(); + await expect(autoInstallSkills()).resolves.toBeNull(); }); it('does not produce any console output', async () => { diff --git a/src/commands/install-skill.ts b/src/commands/install-skill.ts index 749f71f8..eef011e8 100644 --- a/src/commands/install-skill.ts +++ b/src/commands/install-skill.ts @@ -1,10 +1,30 @@ import { homedir } from 'os'; -import { join } from 'path'; -import { existsSync } from 'fs'; -import { mkdir, copyFile, readdir } from 'fs/promises'; +import { dirname, join } from 'path'; +import { existsSync, readFileSync } from 'fs'; +import { mkdir, copyFile, readdir, writeFile } from 'fs/promises'; import chalk from 'chalk'; import { getSkillsDir as getSkillsPackageDir } from '@workos/skills'; +export const SKILL_VERSION_MARKER_FILENAME = '.workos-skill-version'; + +/** + * Read the bundled @workos/skills version by walking up from the skills + * directory to the package.json. The package's `exports` map doesn't expose + * package.json, so we resolve it by filesystem convention. + * Returns null if the version can't be determined — callers treat that as + * "no marker written" rather than failing the install. + */ +export function getBundledSkillsVersion(skillsDir: string = getSkillsPackageDir()): string | null { + try { + // skillsDir = /plugins/workos/skills + const packageRoot = dirname(dirname(dirname(skillsDir))); + const pkgJson = JSON.parse(readFileSync(join(packageRoot, 'package.json'), 'utf8')); + return typeof pkgJson.version === 'string' ? pkgJson.version : null; + } catch { + return null; + } +} + export interface AgentConfig { name: string; displayName: string; @@ -157,11 +177,20 @@ export async function runInstallSkill(options: InstallSkillOptions): Promise { +export async function autoInstallSkills(): Promise { try { const home = homedir(); const agents = createAgents(home); @@ -169,14 +198,37 @@ export async function autoInstallSkills(): Promise { const skills = await discoverSkills(skillsDir); const targetAgents = detectAgents(agents); - if (skills.length === 0 || targetAgents.length === 0) return; + if (skills.length === 0 || targetAgents.length === 0) return null; + + const version = getBundledSkillsVersion(skillsDir); + const succeededAgents: AgentConfig[] = []; - for (const skill of skills) { - for (const agent of targetAgents) { - await installSkill(skillsDir, skill, agent); + for (const agent of targetAgents) { + let agentSucceeded = false; + for (const skill of skills) { + const result = await installSkill(skillsDir, skill, agent); + if (result.success) agentSucceeded = true; + } + if (agentSucceeded) { + succeededAgents.push(agent); + if (version) { + try { + await writeFile(join(agent.globalSkillsDir, SKILL_VERSION_MARKER_FILENAME), version, 'utf8'); + } catch { + // Marker is best-effort; doctor treats missing marker as "unknown" + } + } } } + + if (succeededAgents.length === 0) return null; + + return { + skills, + agents: succeededAgents.map((a) => a.displayName), + version, + }; } catch { - // Intentionally swallowed — skill install is best-effort + return null; } } diff --git a/src/commands/install.spec.ts b/src/commands/install.spec.ts index b28e09aa..c46b9fc1 100644 --- a/src/commands/install.spec.ts +++ b/src/commands/install.spec.ts @@ -25,6 +25,8 @@ vi.mock('../utils/debug.js', () => ({ const { runInstaller } = await import('../run.js'); const { autoInstallSkills } = await import('./install-skill.js'); +const clack = (await import('../utils/clack.js')).default; +const { isJsonMode } = await import('../utils/output.js'); vi.spyOn(process, 'exit').mockImplementation((() => { throw new Error('process.exit called'); @@ -39,7 +41,7 @@ describe('handleInstall', () => { it('calls autoInstallSkills after successful install', async () => { vi.mocked(runInstaller).mockResolvedValue(undefined as any); - vi.mocked(autoInstallSkills).mockResolvedValue(undefined); + vi.mocked(autoInstallSkills).mockResolvedValue(null); await expect(handleInstall({ _: ['install'], $0: 'workos' } as any)).rejects.toThrow('process.exit called'); @@ -52,6 +54,42 @@ describe('handleInstall', () => { expect(autoInstallOrder).toBeGreaterThan(runInstallerOrder); }); + it('prints an info line when skills were installed in a TTY session', async () => { + vi.mocked(runInstaller).mockResolvedValue(undefined as any); + vi.mocked(autoInstallSkills).mockResolvedValue({ + skills: ['workos', 'workos-widgets'], + agents: ['Claude Code'], + }); + vi.mocked(isJsonMode).mockReturnValue(false); + + await expect(handleInstall({ _: ['install'], $0: 'workos' } as any)).rejects.toThrow('process.exit called'); + + expect(clack.log.info).toHaveBeenCalledWith(expect.stringContaining('Installed 2 WorkOS skills for Claude Code')); + }); + + it('does not print the info line when autoInstallSkills returns null', async () => { + vi.mocked(runInstaller).mockResolvedValue(undefined as any); + vi.mocked(autoInstallSkills).mockResolvedValue(null); + vi.mocked(isJsonMode).mockReturnValue(false); + + await expect(handleInstall({ _: ['install'], $0: 'workos' } as any)).rejects.toThrow('process.exit called'); + + expect(clack.log.info).not.toHaveBeenCalled(); + }); + + it('suppresses the info line in JSON mode', async () => { + vi.mocked(runInstaller).mockResolvedValue(undefined as any); + vi.mocked(autoInstallSkills).mockResolvedValue({ + skills: ['workos'], + agents: ['Claude Code'], + }); + vi.mocked(isJsonMode).mockReturnValue(true); + + await expect(handleInstall({ _: ['install'], $0: 'workos' } as any)).rejects.toThrow('process.exit called'); + + expect(clack.log.info).not.toHaveBeenCalled(); + }); + it('does not call autoInstallSkills when runInstaller throws', async () => { vi.mocked(runInstaller).mockRejectedValue(new Error('install failed')); diff --git a/src/commands/install.ts b/src/commands/install.ts index c023c995..d2b10dd8 100644 --- a/src/commands/install.ts +++ b/src/commands/install.ts @@ -29,7 +29,13 @@ export async function handleInstall(argv: ArgumentsCamelCase): Pr try { await runInstaller(options); - await autoInstallSkills(); + const skillResult = await autoInstallSkills(); + if (skillResult && !isJsonMode()) { + const skillWord = skillResult.skills.length === 1 ? 'skill' : 'skills'; + clack.log.info( + `Installed ${skillResult.skills.length} WorkOS ${skillWord} for ${skillResult.agents.join(', ')}. Your coding agent now has up-to-date WorkOS guidance.`, + ); + } process.exit(0); } catch (err) { const { getLogFilePath } = await import('../utils/debug.js'); From 3476e85c2984dbd2315d06957993a7f3a1288a4e Mon Sep 17 00:00:00 2001 From: Nick Nisi Date: Thu, 23 Apr 2026 20:41:24 -0700 Subject: [PATCH 02/10] feat(doctor): warn when WorkOS skills are outdated Compare each detected coding agent's .workos-skill-version marker against the bundled @workos/skills version. Emit a SKILLS_OUTDATED warning with a remediation pointing users at `workos skills install`. Returns null when no agent has WorkOS skills installed, so doctor stays quiet for users who never installed through the CLI. --- src/doctor/checks/skills.spec.ts | 92 ++++++++++++++++++++++++++++++++ src/doctor/checks/skills.ts | 48 +++++++++++++++++ src/doctor/index.ts | 5 ++ src/doctor/issues.ts | 16 ++++++ src/doctor/types.ts | 12 +++++ 5 files changed, 173 insertions(+) create mode 100644 src/doctor/checks/skills.spec.ts create mode 100644 src/doctor/checks/skills.ts diff --git a/src/doctor/checks/skills.spec.ts b/src/doctor/checks/skills.spec.ts new file mode 100644 index 00000000..8d5ae248 --- /dev/null +++ b/src/doctor/checks/skills.spec.ts @@ -0,0 +1,92 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { mkdtempSync, mkdirSync, writeFileSync, rmSync } from 'fs'; +import { join } from 'path'; +import { tmpdir } from 'os'; +import { SKILL_VERSION_MARKER_FILENAME } from '../../commands/install-skill.js'; + +// Mock getBundledSkillsVersion so we don't depend on the real bundled package. +vi.mock('../../commands/install-skill.js', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + getBundledSkillsVersion: vi.fn(), + }; +}); + +const { getBundledSkillsVersion } = await import('../../commands/install-skill.js'); +const { checkSkills } = await import('./skills.js'); + +describe('checkSkills', () => { + let testHome: string; + + beforeEach(() => { + testHome = mkdtempSync(join(tmpdir(), 'skills-check-')); + vi.mocked(getBundledSkillsVersion).mockReturnValue('0.3.0'); + }); + + afterEach(() => { + rmSync(testHome, { recursive: true, force: true }); + vi.restoreAllMocks(); + }); + + it('returns null when no agent skills directories exist', () => { + expect(checkSkills(testHome)).toBeNull(); + }); + + it('reports an agent with no marker as installedVersion=null and not stale', () => { + mkdirSync(join(testHome, '.claude/skills'), { recursive: true }); + + const result = checkSkills(testHome); + + expect(result).not.toBeNull(); + expect(result!.bundledVersion).toBe('0.3.0'); + expect(result!.agents).toEqual([{ agent: 'Claude Code', installedVersion: null, stale: false }]); + }); + + it('flags an agent as stale when the marker trails the bundled version', () => { + const skillsDir = join(testHome, '.claude/skills'); + mkdirSync(skillsDir, { recursive: true }); + writeFileSync(join(skillsDir, SKILL_VERSION_MARKER_FILENAME), '0.2.4'); + + const result = checkSkills(testHome); + + expect(result!.agents).toEqual([{ agent: 'Claude Code', installedVersion: '0.2.4', stale: true }]); + }); + + it('does not flag stale when marker matches bundled', () => { + const skillsDir = join(testHome, '.claude/skills'); + mkdirSync(skillsDir, { recursive: true }); + writeFileSync(join(skillsDir, SKILL_VERSION_MARKER_FILENAME), '0.3.0'); + + const result = checkSkills(testHome); + + expect(result!.agents[0].stale).toBe(false); + }); + + it('never flags stale when bundledVersion is null (unknown)', () => { + vi.mocked(getBundledSkillsVersion).mockReturnValue(null); + const skillsDir = join(testHome, '.claude/skills'); + mkdirSync(skillsDir, { recursive: true }); + writeFileSync(join(skillsDir, SKILL_VERSION_MARKER_FILENAME), '0.2.4'); + + const result = checkSkills(testHome); + + expect(result!.bundledVersion).toBeNull(); + expect(result!.agents[0].stale).toBe(false); + }); + + it('reports each detected agent separately', () => { + mkdirSync(join(testHome, '.claude/skills'), { recursive: true }); + writeFileSync(join(testHome, '.claude/skills', SKILL_VERSION_MARKER_FILENAME), '0.2.4'); + mkdirSync(join(testHome, '.codex/skills'), { recursive: true }); + writeFileSync(join(testHome, '.codex/skills', SKILL_VERSION_MARKER_FILENAME), '0.3.0'); + + const result = checkSkills(testHome); + + expect(result!.agents).toHaveLength(2); + const claude = result!.agents.find((a) => a.agent === 'Claude Code')!; + const codex = result!.agents.find((a) => a.agent === 'Codex')!; + expect(claude.stale).toBe(true); + expect(codex.stale).toBe(false); + }); +}); diff --git a/src/doctor/checks/skills.ts b/src/doctor/checks/skills.ts new file mode 100644 index 00000000..f8f9062e --- /dev/null +++ b/src/doctor/checks/skills.ts @@ -0,0 +1,48 @@ +import { homedir } from 'node:os'; +import { readFileSync, existsSync } from 'node:fs'; +import { join } from 'node:path'; +import { createAgents, getBundledSkillsVersion, SKILL_VERSION_MARKER_FILENAME } from '../../commands/install-skill.js'; +import type { SkillsInfo, SkillAgentStatus } from '../types.js'; + +/** + * Check the freshness of auto-installed WorkOS skills across detected coding + * agents. Compares each agent's version marker (written by autoInstallSkills) + * against the bundled @workos/skills version the CLI ships with. Returns null + * when no agents have a WorkOS skill installed at all — no noise for users who + * never installed through the CLI. + */ +export function checkSkills(home: string = homedir()): SkillsInfo | null { + const bundledVersion = getBundledSkillsVersion(); + const agents = createAgents(home); + + const statuses: SkillAgentStatus[] = []; + + for (const [, agent] of Object.entries(agents)) { + // Only report on agents that actually have our skills dir laid down. + // An agent directory existing (e.g. ~/.claude) doesn't imply we installed. + if (!existsSync(agent.globalSkillsDir)) continue; + + const markerPath = join(agent.globalSkillsDir, SKILL_VERSION_MARKER_FILENAME); + let installedVersion: string | null = null; + if (existsSync(markerPath)) { + try { + installedVersion = readFileSync(markerPath, 'utf8').trim() || null; + } catch { + installedVersion = null; + } + } + + statuses.push({ + agent: agent.displayName, + installedVersion, + stale: Boolean(bundledVersion && installedVersion && installedVersion !== bundledVersion), + }); + } + + if (statuses.length === 0) return null; + + return { + bundledVersion, + agents: statuses, + }; +} diff --git a/src/doctor/index.ts b/src/doctor/index.ts index 1b652d13..b0bcf9a5 100644 --- a/src/doctor/index.ts +++ b/src/doctor/index.ts @@ -7,6 +7,7 @@ import { checkConnectivity } from './checks/connectivity.js'; import { checkDashboardSettings, compareRedirectUris } from './checks/dashboard.js'; import { checkAuthPatterns } from './checks/auth-patterns.js'; import { checkAiAnalysis } from './checks/ai-analysis.js'; +import { checkSkills } from './checks/skills.js'; import { detectIssues } from './issues.js'; import { formatReport } from './output.js'; import { formatReportAsJson } from './json-output.js'; @@ -30,6 +31,8 @@ export async function runDoctor(options: DoctorOptions): Promise { checkLanguage(options.installDir), ]); + const skills = checkSkills() ?? undefined; + // Dashboard settings + auth patterns + AI analysis (parallel, all need sdk/framework results) // AI analysis also receives early issues as context to avoid duplication const earlyIssues = detectIssues({ @@ -42,6 +45,7 @@ export async function runDoctor(options: DoctorOptions): Promise { framework, environment, connectivity, + skills, }); const [dashboardResult, authPatterns, aiAnalysis] = await Promise.all([ @@ -86,6 +90,7 @@ export async function runDoctor(options: DoctorOptions): Promise { redirectUris, authPatterns, aiAnalysis, + skills, }; // Detect issues based on collected data diff --git a/src/doctor/issues.ts b/src/doctor/issues.ts index d1bf183a..d653251e 100644 --- a/src/doctor/issues.ts +++ b/src/doctor/issues.ts @@ -148,6 +148,22 @@ export function detectIssues(report: Omit): } } + // Skill freshness — warn when agent-installed skills trail the bundled version. + // Surfaced so users know their coding agent is using older WorkOS guidance. + if (report.skills?.bundledVersion) { + const stale = report.skills.agents.filter((a) => a.stale); + if (stale.length > 0) { + const agentList = stale.map((a) => `${a.agent} (${a.installedVersion ?? 'unknown'})`).join(', '); + issues.push({ + code: 'SKILLS_OUTDATED', + severity: 'warning', + message: `WorkOS skills outdated for ${agentList} — bundled: ${report.skills.bundledVersion}`, + remediation: 'Run: workos skills install', + details: { bundledVersion: report.skills.bundledVersion, stale: stale.map((a) => a.agent) }, + }); + } + } + return issues; } diff --git a/src/doctor/types.ts b/src/doctor/types.ts index 63e81897..f1f8e7c9 100644 --- a/src/doctor/types.ts +++ b/src/doctor/types.ts @@ -108,6 +108,17 @@ export interface AuthPatternInfo { findings: AuthPatternFinding[]; } +export interface SkillAgentStatus { + agent: string; + installedVersion: string | null; + stale: boolean; +} + +export interface SkillsInfo { + bundledVersion: string | null; + agents: SkillAgentStatus[]; +} + export interface DoctorReport { version: string; timestamp: string; @@ -127,6 +138,7 @@ export interface DoctorReport { credentialValidation?: CredentialValidation; authPatterns?: AuthPatternInfo; aiAnalysis?: AiAnalysis; + skills?: SkillsInfo; issues: Issue[]; summary: { errors: number; From ae7068545f29fb9e1392a0e4691da3609942fb17 Mon Sep 17 00:00:00 2001 From: Nick Nisi Date: Sun, 26 Apr 2026 14:25:09 -0500 Subject: [PATCH 03/10] feat(install): recursive prune-replace skill copy with rollback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 2 of cus-feedback-fixes. Closes the gap where `workos install` shipped only `SKILL.md` and not the `references/` tree the WorkOS skill router instructs the agent to Read. Without those reference files, the auto-installed skill was functionally empty. * Rewrite `installSkill` from a single `copyFile(SKILL.md)` to a recursive prune-replace of the whole `//` tree using `mkdtemp` + `cp { recursive: true }` + sibling `.workos.bak-` backup-rename. Rolls back to the original target if the temp→target rename fails mid-flight; tempDir is cleaned up on copy failure. Backup cleanup is best-effort post-success — the install does not fail if the backup remains, and `cleanupStaleOrphans` will reap it on the next run after 1h. * Add `cleanupStaleOrphans` with a 1-hour mtime cutoff (constant `ORPHAN_STALE_MS`). Concurrent runs that leave fresh `.tmp-*` / `.bak-*` siblings are never touched — the cutoff guarantees we only nuke orphans from runs that crashed long ago. * Extract `refreshWorkOSSkills(opts)` as a reusable primitive returning `RefreshResult` with `perAgentBefore` / `perAgentAfter` keyed by `agent.name`. Both `autoInstallSkills` (best-effort hook) and doctor `--fix` (Phase 3) will call this — no duplicate copy logic. `autoInstallSkills` becomes a thin back-compat wrapper that returns the existing `AutoInstallResult` shape (`install.ts` consumers stay unchanged). * Add `installSkillsAfterLogin` helper in `login.ts`, wired in after `provisionStagingEnvironment`. Wraps `autoInstallSkills` in its own try/catch so a skill install failure NEVER fails login itself. Skips logging in JSON mode, mirrors install.ts's success copy. Extracted as a separate exported function so it's unit-testable without standing up the device-auth polling loop. * Tests: - `installSkill` copies `references/` subdirectory - planted stale `references/workos-stale.md` is gone after re-install - simulated rename failure restores original target from backup - copy failure cleans up tempDir (no leftover `.workos.tmp-*`) - `cleanupStaleOrphans` removes >1h orphans of both prefixes, preserves <1h orphans of both prefixes - peer skill directories (different prefix) are never touched - `refreshWorkOSSkills` reports per-agent before/after marker state, respects `writeMarker: false`, filters by `skills` and `agents` - `installSkillsAfterLogin` calls `autoInstallSkills`, returns successfully when installer throws, JSON-mode-aware, singular vs plural copy NOT INCLUDED in this commit (intentional — see open items): - `package.json` bump from `@workos/skills@0.2.4` to `0.4.0`. Phase 1 (the skills-repo release providing the new content) is committed but not yet published to npm. The bump becomes a one-line follow-up once `0.4.0` is on the registry. --- src/commands/install-skill.spec.ts | 250 ++++++++++++++++++++++++++++- src/commands/install-skill.ts | 204 ++++++++++++++++++----- src/commands/login.spec.ts | 96 ++++++++++- src/commands/login.ts | 31 ++++ 4 files changed, 536 insertions(+), 45 deletions(-) diff --git a/src/commands/install-skill.spec.ts b/src/commands/install-skill.spec.ts index 4af36e2d..1b65bd43 100644 --- a/src/commands/install-skill.spec.ts +++ b/src/commands/install-skill.spec.ts @@ -1,5 +1,6 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; -import { existsSync, mkdirSync, writeFileSync, rmSync, readFileSync } from 'fs'; +import { existsSync, mkdirSync, writeFileSync, rmSync, readFileSync, readdirSync, chmodSync } from 'fs'; +import { utimes } from 'fs/promises'; import { join } from 'path'; import { mkdtempSync } from 'fs'; import { tmpdir } from 'os'; @@ -9,6 +10,7 @@ import { detectAgents, installSkill, autoInstallSkills, + refreshWorkOSSkills, type AgentConfig, } from './install-skill.js'; @@ -22,6 +24,19 @@ vi.mock('@workos/skills', async (importOriginal) => { return { ...actual, getSkillsDir: vi.fn(actual.getSkillsDir) }; }); +// Wrap fs/promises so individual fns (rename, cp) can be temporarily overridden +// via mockImplementationOnce. ESM exports are not directly spy-able, so we +// explicitly recreate the namespace with vi.fn passthroughs for the spies we +// need; everything else stays as the real impl. +vi.mock('fs/promises', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + rename: vi.fn(actual.rename), + cp: vi.fn(actual.cp), + }; +}); + describe('install-skill', () => { let testDir: string; let skillsDir: string; @@ -164,6 +179,10 @@ describe('install-skill', () => { mkdirSync(join(skillsDir, 'test-skill')); writeFileSync(join(skillsDir, 'test-skill', 'SKILL.md'), '---\nname: test-skill\n---\n# Test Skill'); + // Sub-tree the install must copy alongside SKILL.md. + mkdirSync(join(skillsDir, 'test-skill', 'references')); + writeFileSync(join(skillsDir, 'test-skill', 'references', 'topic.md'), '# Topic'); + targetAgent = { name: 'test-agent', displayName: 'Test Agent', @@ -185,6 +204,16 @@ describe('install-skill', () => { expect(content).toContain('# Test Skill'); }); + it('copies the entire skill directory tree, including references/', async () => { + const result = await installSkill(skillsDir, 'test-skill', targetAgent); + + expect(result.success).toBe(true); + + const referenceFile = join(homeDir, '.test-agent/skills/test-skill/references/topic.md'); + expect(existsSync(referenceFile)).toBe(true); + expect(readFileSync(referenceFile, 'utf-8')).toContain('# Topic'); + }); + it('creates nested directories as needed', async () => { const result = await installSkill(skillsDir, 'test-skill', targetAgent); @@ -211,6 +240,115 @@ describe('install-skill', () => { const content = readFileSync(join(homeDir, '.test-agent/skills/test-skill/SKILL.md'), 'utf-8'); expect(content).toContain('# Updated Skill'); }); + + it('prunes stale files in the target that are not in the source (replace, not overlay)', async () => { + // First install: target now matches source. + await installSkill(skillsDir, 'test-skill', targetAgent); + + // Plant a stale file the agent had from a prior skill version. + const staleFile = join(homeDir, '.test-agent/skills/test-skill/references/workos-stale.md'); + writeFileSync(staleFile, '# Stale Topic'); + expect(existsSync(staleFile)).toBe(true); + + // Re-install — the new tree should fully replace the old one. + const result = await installSkill(skillsDir, 'test-skill', targetAgent); + + expect(result.success).toBe(true); + expect(existsSync(staleFile)).toBe(false); + // Source files still present. + expect(existsSync(join(homeDir, '.test-agent/skills/test-skill/references/topic.md'))).toBe(true); + }); + + it('rolls back to the original target when the temp→target rename fails mid-install', async () => { + // Seed an existing target so the backup-rename branch has something to back up. + await installSkill(skillsDir, 'test-skill', targetAgent); + const targetFile = join(homeDir, '.test-agent/skills/test-skill/SKILL.md'); + const originalContent = readFileSync(targetFile, 'utf-8'); + + const fsPromises = await import('fs/promises'); + const realRename = (await vi.importActual('fs/promises')).rename; + + // Call sequence inside installSkill: 1=target→backup, 2=temp→target (force-throw), + // 3=backup→target (rollback). After the Once chain consumes calls 1 and 2, + // the default vi.fn(actual.rename) impl handles call 3 with the real fs op. + vi.mocked(fsPromises.rename) + .mockImplementationOnce(realRename) + .mockImplementationOnce(async () => { + throw new Error('simulated rename failure'); + }); + + const result = await installSkill(skillsDir, 'test-skill', targetAgent); + + expect(result.success).toBe(false); + expect(result.error).toContain('simulated rename failure'); + // Original target restored from backup. + expect(existsSync(targetFile)).toBe(true); + expect(readFileSync(targetFile, 'utf-8')).toBe(originalContent); + }); + + it('cleans up the temp dir when the copy itself fails', async () => { + const fsPromises = await import('fs/promises'); + vi.mocked(fsPromises.cp).mockRejectedValueOnce(new Error('copy boom')); + + const result = await installSkill(skillsDir, 'test-skill', targetAgent); + + expect(result.success).toBe(false); + expect(result.error).toContain('copy boom'); + + // No leftover .workos.tmp-test-skill-* in the parent. + const parent = join(homeDir, '.test-agent/skills'); + if (existsSync(parent)) { + const leftovers = readdirSync(parent).filter((e) => e.startsWith('.workos.tmp-')); + expect(leftovers).toEqual([]); + } + }); + + it('removes orphaned .workos.tmp-* / .workos.bak-* siblings older than 1h', async () => { + const parent = join(homeDir, '.test-agent/skills'); + mkdirSync(parent, { recursive: true }); + + // mtime cutoff applies independently to BOTH prefixes, both directions. + const oldTime = new Date(Date.now() - 2 * 60 * 60 * 1000); // 2h ago + + const oldTmp = join(parent, '.workos.tmp-test-skill-deadbeef'); + mkdirSync(oldTmp); + await utimes(oldTmp, oldTime, oldTime); + + const oldBak = join(parent, '.workos.bak-test-skill-feedface'); + mkdirSync(oldBak); + await utimes(oldBak, oldTime, oldTime); + + // Fresh siblings simulate a concurrent run — must NOT be deleted. + const freshTmp = join(parent, '.workos.tmp-test-skill-deadc0de'); + mkdirSync(freshTmp); + + const freshBak = join(parent, '.workos.bak-test-skill-cafef00d'); + mkdirSync(freshBak); + + const result = await installSkill(skillsDir, 'test-skill', targetAgent); + expect(result.success).toBe(true); + + expect(existsSync(oldTmp)).toBe(false); + expect(existsSync(oldBak)).toBe(false); + expect(existsSync(freshTmp)).toBe(true); + expect(existsSync(freshBak)).toBe(true); + }); + + it('does not remove unrelated files in the parent', async () => { + const parent = join(homeDir, '.test-agent/skills'); + mkdirSync(parent, { recursive: true }); + + // A peer skill from a different installer — must be preserved. + const peerSkill = join(parent, 'unrelated-skill'); + mkdirSync(peerSkill); + writeFileSync(join(peerSkill, 'SKILL.md'), '# Other'); + const oldTime = new Date(Date.now() - 2 * 60 * 60 * 1000); + await utimes(peerSkill, oldTime, oldTime); + + const result = await installSkill(skillsDir, 'test-skill', targetAgent); + expect(result.success).toBe(true); + expect(existsSync(join(peerSkill, 'SKILL.md'))).toBe(true); + }); }); describe('autoInstallSkills', () => { @@ -304,12 +442,16 @@ describe('install-skill', () => { mkdirSync(join(skillsDir, 'test-skill')); writeFileSync(join(skillsDir, 'test-skill', 'SKILL.md'), '# Test'); - mkdirSync(join(homeDir, '.claude')); - // Create a file where the skill subdir should be, so mkdir fails - mkdirSync(join(homeDir, '.claude/skills')); - writeFileSync(join(homeDir, '.claude/skills/test-skill'), 'not a directory'); + mkdirSync(join(homeDir, '.claude/skills'), { recursive: true }); + // Make the skills parent read-only so mkdtemp inside it fails with EACCES. + chmodSync(join(homeDir, '.claude/skills'), 0o555); - await expect(autoInstallSkills()).resolves.toBeNull(); + try { + await expect(autoInstallSkills()).resolves.toBeNull(); + } finally { + // Restore writable so afterEach cleanup can rm. + chmodSync(join(homeDir, '.claude/skills'), 0o755); + } }); it('does not produce any console output', async () => { @@ -329,4 +471,100 @@ describe('install-skill', () => { errorSpy.mockRestore(); }); }); + + describe('refreshWorkOSSkills', () => { + beforeEach(async () => { + const { homedir } = await import('os'); + const { getSkillsDir } = await import('@workos/skills'); + vi.mocked(homedir).mockReturnValue(homeDir); + vi.mocked(getSkillsDir).mockReturnValue(skillsDir); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it('reports per-agent before/after marker state keyed by agent.name', async () => { + const { SKILL_VERSION_MARKER_FILENAME } = await import('./install-skill.js'); + + mkdirSync(join(skillsDir, 'skill-a')); + writeFileSync(join(skillsDir, 'skill-a', 'SKILL.md'), '# Skill A'); + + // Plant a pre-existing marker for one agent so we have a non-null "before". + mkdirSync(join(homeDir, '.claude/skills'), { recursive: true }); + writeFileSync(join(homeDir, '.claude/skills', SKILL_VERSION_MARKER_FILENAME), '0.0.1', 'utf8'); + + // Codex has no prior marker. + mkdirSync(join(homeDir, '.codex')); + + const result = await refreshWorkOSSkills(); + + expect(result).not.toBeNull(); + expect(result!.perAgentBefore).toMatchObject({ + 'claude-code': '0.0.1', + codex: null, + }); + // After: in this fixture skillsDir isn't an npm package layout, so + // getBundledSkillsVersion returns null and no marker is rewritten. + // Therefore perAgentAfter must equal perAgentBefore for every agent. + expect(result!.perAgentAfter).toMatchObject({ + 'claude-code': '0.0.1', + codex: null, + }); + }); + + it('skips marker writing when writeMarker is false', async () => { + const { SKILL_VERSION_MARKER_FILENAME } = await import('./install-skill.js'); + + mkdirSync(join(skillsDir, 'skill-a')); + writeFileSync(join(skillsDir, 'skill-a', 'SKILL.md'), '# Skill A'); + mkdirSync(join(homeDir, '.claude')); + + const result = await refreshWorkOSSkills({ writeMarker: false }); + + expect(result).not.toBeNull(); + const marker = join(homeDir, '.claude/skills', SKILL_VERSION_MARKER_FILENAME); + expect(existsSync(marker)).toBe(false); + }); + + it('filters skills by the skills option', async () => { + mkdirSync(join(skillsDir, 'skill-a')); + writeFileSync(join(skillsDir, 'skill-a', 'SKILL.md'), '# A'); + mkdirSync(join(skillsDir, 'skill-b')); + writeFileSync(join(skillsDir, 'skill-b', 'SKILL.md'), '# B'); + mkdirSync(join(homeDir, '.claude')); + + const result = await refreshWorkOSSkills({ skills: ['skill-a'] }); + + expect(result).not.toBeNull(); + expect(result!.skills).toEqual(['skill-a']); + expect(existsSync(join(homeDir, '.claude/skills/skill-a/SKILL.md'))).toBe(true); + expect(existsSync(join(homeDir, '.claude/skills/skill-b'))).toBe(false); + }); + + it('uses the agents option instead of detecting from $HOME', async () => { + mkdirSync(join(skillsDir, 'skill-a')); + writeFileSync(join(skillsDir, 'skill-a', 'SKILL.md'), '# A'); + + // No agent dirs in homeDir — detectAgents would return []. + // Pass an explicit agent so the install proceeds anyway. + const explicitAgent: AgentConfig = { + name: 'manual', + displayName: 'Manual Agent', + globalSkillsDir: join(homeDir, 'manual/skills'), + detect: () => true, + }; + + const result = await refreshWorkOSSkills({ agents: [explicitAgent] }); + + expect(result).not.toBeNull(); + expect(result!.agents.map((a) => a.name)).toEqual(['manual']); + expect(existsSync(join(homeDir, 'manual/skills/skill-a/SKILL.md'))).toBe(true); + }); + + it('returns null when no agents and no skills', async () => { + // Empty skillsDir, no detected agents. + await expect(refreshWorkOSSkills()).resolves.toBeNull(); + }); + }); }); diff --git a/src/commands/install-skill.ts b/src/commands/install-skill.ts index eef011e8..2163944c 100644 --- a/src/commands/install-skill.ts +++ b/src/commands/install-skill.ts @@ -1,12 +1,17 @@ import { homedir } from 'os'; import { dirname, join } from 'path'; import { existsSync, readFileSync } from 'fs'; -import { mkdir, copyFile, readdir, writeFile } from 'fs/promises'; +import { mkdir, mkdtemp, cp, rename, rm, readdir, stat, writeFile } from 'fs/promises'; import chalk from 'chalk'; import { getSkillsDir as getSkillsPackageDir } from '@workos/skills'; export const SKILL_VERSION_MARKER_FILENAME = '.workos-skill-version'; +// Stale-orphan cutoff for `.workos.tmp-*` / `.workos.bak-*` siblings left behind +// by a crashed prior run. Anything younger may belong to a concurrent install +// and must NOT be removed. +const ORPHAN_STALE_MS = 60 * 60 * 1000; + /** * Read the bundled @workos/skills version by walking up from the skills * directory to the package.json. The package's `exports` map doesn't expose @@ -89,20 +94,56 @@ export function detectAgents(agents: Record, filter?: strin return detected; } +/** + * Recursively install a skill directory (SKILL.md + references/ + any other + * files) with prune-replace semantics. Uses a sibling temp dir + backup-rename + * pattern so the operation is effectively atomic per skill: the target either + * matches the source exactly, or (on rollback) is restored to its prior state. + * + * Returns `{ success, error }` rather than throwing — callers (autoInstallSkills, + * runInstallSkill) accumulate failures across the (skill × agent) matrix. + */ export async function installSkill( skillsDir: string, skillName: string, agent: AgentConfig, ): Promise<{ success: boolean; error?: string }> { - const sourceFile = join(skillsDir, skillName, 'SKILL.md'); + const sourceDir = join(skillsDir, skillName); const targetDir = join(agent.globalSkillsDir, skillName); - const targetFile = join(targetDir, 'SKILL.md'); + const parent = dirname(targetDir); + + await mkdir(parent, { recursive: true }); + // Best-effort cleanup of OLD orphans only — never current-run paths. + await cleanupStaleOrphans(parent, skillName).catch(() => {}); + + // mkdtemp gives us atomic creation + a random suffix that prevents + // collisions between concurrent installers. + const tempDir = await mkdtemp(join(parent, `.workos.tmp-${skillName}-`)); + const backupDir = tempDir.replace('.workos.tmp-', '.workos.bak-'); try { - await mkdir(targetDir, { recursive: true }); - await copyFile(sourceFile, targetFile); + await cp(sourceDir, tempDir, { recursive: true, errorOnExist: false }); + + const targetExisted = existsSync(targetDir); + if (targetExisted) { + await rename(targetDir, backupDir); + } + try { + await rename(tempDir, targetDir); + } catch (renameErr) { + if (targetExisted) { + await rename(backupDir, targetDir).catch(() => {}); + } + throw renameErr; + } + // Backup cleanup is best-effort: target is already in place, so failure + // here leaves a stale backup that the next run's cleanup handles after 1h. + if (targetExisted) { + await rm(backupDir, { recursive: true, force: true }).catch(() => {}); + } return { success: true }; } catch (error) { + await rm(tempDir, { recursive: true, force: true }).catch(() => {}); return { success: false, error: error instanceof Error ? error.message : 'Unknown error', @@ -110,6 +151,27 @@ export async function installSkill( } } +/** + * Remove `.workos.tmp-{skillName}-*` and `.workos.bak-{skillName}-*` siblings + * older than ORPHAN_STALE_MS. Fresh siblings (from a concurrent install) are + * preserved — destroying them would race the other run's final rename. + */ +async function cleanupStaleOrphans(parent: string, skillName: string): Promise { + if (!existsSync(parent)) return; + const entries = await readdir(parent).catch(() => []); + const cutoff = Date.now() - ORPHAN_STALE_MS; + for (const entry of entries) { + const isOrphan = + entry.startsWith(`.workos.tmp-${skillName}-`) || entry.startsWith(`.workos.bak-${skillName}-`); + if (!isOrphan) continue; + const path = join(parent, entry); + const st = await stat(path).catch(() => null); + if (st && st.mtimeMs < cutoff) { + await rm(path, { recursive: true, force: true }).catch(() => {}); + } + } +} + export async function runInstallSkill(options: InstallSkillOptions): Promise { const home = homedir(); const agents = createAgents(home); @@ -183,50 +245,116 @@ export interface AutoInstallResult { version: string | null; } +export interface RefreshOptions { + /** Pre-detected agents. Default: detect from $HOME. */ + agents?: AgentConfig[]; + /** Skill names to install. Default: all bundled skills. */ + skills?: string[]; + /** Whether to write the version marker after a successful per-agent install. Default: true. */ + writeMarker?: boolean; +} + +export interface RefreshResult { + /** Agents where at least one skill installed successfully. */ + agents: AgentConfig[]; + /** Skills that were attempted (the resolved set after filtering). */ + skills: string[]; + /** Bundled skills package version, or null if it couldn't be resolved. */ + version: string | null; + /** Marker version per agent.name BEFORE refresh (null = no marker / unreadable). */ + perAgentBefore: Record; + /** Marker version per agent.name AFTER refresh. */ + perAgentAfter: Record; +} + +function readSkillVersionMarker(agent: AgentConfig): string | null { + const path = join(agent.globalSkillsDir, SKILL_VERSION_MARKER_FILENAME); + if (!existsSync(path)) return null; + try { + return readFileSync(path, 'utf8').trim() || null; + } catch { + return null; + } +} + /** - * Install all bundled skills to all detected coding agents. - * Returns a summary when anything was installed, or null when nothing applied. - * Performs minimal IO: writes a version marker file alongside installed - * skills so `workos doctor` can detect staleness later. Errors are swallowed - * so skill install never disrupts the calling flow. + * Reusable primitive: discover bundled skills, install each one to each agent, + * write per-agent version markers, and report before/after marker state. + * + * Both `autoInstallSkills` (best-effort hook called from install/login) and + * `doctor --fix` (Phase 3) call this — there is no duplicate copy logic. + * + * Returns null when nothing applied (no agents detected, no skills found, or + * every install attempt failed). */ -export async function autoInstallSkills(): Promise { - try { - const home = homedir(); - const agents = createAgents(home); - const skillsDir = getSkillsDir(); - const skills = await discoverSkills(skillsDir); - const targetAgents = detectAgents(agents); +export async function refreshWorkOSSkills(opts: RefreshOptions = {}): Promise { + const home = homedir(); + const skillsDir = getSkillsDir(); + const detected = opts.agents ?? detectAgents(createAgents(home)); + const allSkills = await discoverSkills(skillsDir).catch(() => []); + const skills = opts.skills ? allSkills.filter((s) => opts.skills!.includes(s)) : allSkills; + const writeMarker = opts.writeMarker ?? true; - if (skills.length === 0 || targetAgents.length === 0) return null; + if (skills.length === 0 || detected.length === 0) return null; - const version = getBundledSkillsVersion(skillsDir); - const succeededAgents: AgentConfig[] = []; + const version = getBundledSkillsVersion(skillsDir); + const perAgentBefore: Record = {}; + const perAgentAfter: Record = {}; + const succeededAgents: AgentConfig[] = []; - for (const agent of targetAgents) { - let agentSucceeded = false; - for (const skill of skills) { - const result = await installSkill(skillsDir, skill, agent); - if (result.success) agentSucceeded = true; - } - if (agentSucceeded) { - succeededAgents.push(agent); - if (version) { - try { - await writeFile(join(agent.globalSkillsDir, SKILL_VERSION_MARKER_FILENAME), version, 'utf8'); - } catch { - // Marker is best-effort; doctor treats missing marker as "unknown" - } + for (const agent of detected) { + perAgentBefore[agent.name] = readSkillVersionMarker(agent); + + let agentSucceeded = false; + for (const skill of skills) { + const result = await installSkill(skillsDir, skill, agent); + if (result.success) agentSucceeded = true; + } + + if (agentSucceeded) { + succeededAgents.push(agent); + if (writeMarker && version) { + try { + await writeFile(join(agent.globalSkillsDir, SKILL_VERSION_MARKER_FILENAME), version, 'utf8'); + } catch { + // Marker is best-effort; doctor treats missing marker as "unknown". } } } - if (succeededAgents.length === 0) return null; + perAgentAfter[agent.name] = readSkillVersionMarker(agent); + } + if (succeededAgents.length === 0) return null; + + return { + agents: succeededAgents, + skills, + version, + perAgentBefore, + perAgentAfter, + }; +} + +/** + * Install all bundled skills to all detected coding agents. + * Returns a summary when anything was installed, or null when nothing applied. + * Performs minimal IO: writes a version marker file alongside installed + * skills so `workos doctor` can detect staleness later. Errors are swallowed + * so skill install never disrupts the calling flow. + * + * Thin back-compat wrapper around `refreshWorkOSSkills` — the install/auth-login + * call sites use this; doctor `--fix` (Phase 3) calls `refreshWorkOSSkills` + * directly to surface the per-agent before/after marker state. + */ +export async function autoInstallSkills(): Promise { + try { + const result = await refreshWorkOSSkills(); + if (!result) return null; return { - skills, - agents: succeededAgents.map((a) => a.displayName), - version, + skills: result.skills, + agents: result.agents.map((a) => a.displayName), + version: result.version, }; } catch { return null; diff --git a/src/commands/login.spec.ts b/src/commands/login.spec.ts index bb15df3e..eaf1c705 100644 --- a/src/commands/login.spec.ts +++ b/src/commands/login.spec.ts @@ -33,6 +33,16 @@ vi.mock('../lib/staging-api.js', () => ({ fetchStagingCredentials: (...args: unknown[]) => mockFetchStagingCredentials(...args), })); +// Mock skill install + JSON mode — installSkillsAfterLogin tests drive both. +vi.mock('./install-skill.js', () => ({ + autoInstallSkills: vi.fn(), +})); + +vi.mock('../utils/output.js', () => ({ + isJsonMode: vi.fn(() => false), + exitWithError: vi.fn(), +})); + let testDir: string; vi.mock('node:os', async (importOriginal) => { @@ -48,7 +58,10 @@ vi.mock('node:os', async (importOriginal) => { }); const { getConfig, setInsecureConfigStorage, clearConfig } = await import('../lib/config-store.js'); -const { provisionStagingEnvironment } = await import('./login.js'); +const { provisionStagingEnvironment, installSkillsAfterLogin } = await import('./login.js'); +const { autoInstallSkills } = await import('./install-skill.js'); +const { isJsonMode } = await import('../utils/output.js'); +const clackMod = await import('../utils/clack.js'); describe('login', () => { beforeEach(() => { @@ -189,4 +202,85 @@ describe('login', () => { expect(result).toBe(false); }); }); + + describe('installSkillsAfterLogin', () => { + it('invokes autoInstallSkills', async () => { + vi.mocked(autoInstallSkills).mockResolvedValueOnce(null); + + await installSkillsAfterLogin(); + + expect(autoInstallSkills).toHaveBeenCalledOnce(); + }); + + it('returns without throwing when autoInstallSkills rejects', async () => { + vi.mocked(autoInstallSkills).mockRejectedValueOnce(new Error('install boom')); + + // The whole point of the helper: login must keep its success even when + // skill install fails. Asserting no rejection IS the test. + await expect(installSkillsAfterLogin()).resolves.toBeUndefined(); + }); + + it('logs a one-line success message in human mode', async () => { + vi.mocked(autoInstallSkills).mockResolvedValueOnce({ + skills: ['workos', 'workos-widgets'], + agents: ['Claude Code', 'Codex'], + version: '0.4.0', + }); + + const infoSpy = vi.mocked(clackMod.default.log.info); + infoSpy.mockClear(); + + await installSkillsAfterLogin(); + + expect(infoSpy).toHaveBeenCalledOnce(); + const message = infoSpy.mock.calls[0]?.[0] as string; + expect(message).toContain('2 WorkOS skills'); + expect(message).toContain('Claude Code'); + expect(message).toContain('Codex'); + }); + + it('uses singular "skill" when exactly one skill installed', async () => { + vi.mocked(autoInstallSkills).mockResolvedValueOnce({ + skills: ['workos'], + agents: ['Claude Code'], + version: '0.4.0', + }); + + const infoSpy = vi.mocked(clackMod.default.log.info); + infoSpy.mockClear(); + + await installSkillsAfterLogin(); + + const message = infoSpy.mock.calls[0]?.[0] as string; + expect(message).toContain('1 WorkOS skill '); + expect(message).not.toContain('1 WorkOS skills'); + }); + + it('skips logging in JSON mode', async () => { + vi.mocked(isJsonMode).mockReturnValueOnce(true); + vi.mocked(autoInstallSkills).mockResolvedValueOnce({ + skills: ['workos'], + agents: ['Claude Code'], + version: '0.4.0', + }); + + const infoSpy = vi.mocked(clackMod.default.log.info); + infoSpy.mockClear(); + + await installSkillsAfterLogin(); + + expect(infoSpy).not.toHaveBeenCalled(); + }); + + it('skips logging when autoInstallSkills returns null', async () => { + vi.mocked(autoInstallSkills).mockResolvedValueOnce(null); + + const infoSpy = vi.mocked(clackMod.default.log.info); + infoSpy.mockClear(); + + await installSkillsAfterLogin(); + + expect(infoSpy).not.toHaveBeenCalled(); + }); + }); }); diff --git a/src/commands/login.ts b/src/commands/login.ts index ca033d61..9edf54e3 100644 --- a/src/commands/login.ts +++ b/src/commands/login.ts @@ -9,6 +9,8 @@ import { fetchStagingCredentials } from '../lib/staging-api.js'; import { getConfig, saveConfig } from '../lib/config-store.js'; import type { CliConfig } from '../lib/config-store.js'; import { formatWorkOSCommand } from '../utils/command-invocation.js'; +import { autoInstallSkills } from './install-skill.js'; +import { isJsonMode } from '../utils/output.js'; /** * Parse JWT payload @@ -70,6 +72,31 @@ function sleep(ms: number): Promise { return new Promise((resolve) => setTimeout(resolve, ms)); } +/** + * Best-effort skill install after a successful auth-login. + * + * Mirrors the install.ts hook copy, but wraps `autoInstallSkills` in its own + * try/catch so a skill install failure (or hang inside install) NEVER fails + * the login itself. Login already succeeded by the time this runs — the user + * having a working session is the contract that must hold. + * + * Extracted from runLogin so it can be unit-tested without standing up the + * device-auth polling loop. + */ +export async function installSkillsAfterLogin(): Promise { + try { + const result = await autoInstallSkills(); + if (result && !isJsonMode()) { + const skillWord = result.skills.length === 1 ? 'skill' : 'skills'; + clack.log.info( + `Installed ${result.skills.length} WorkOS ${skillWord} for ${result.agents.join(', ')}.`, + ); + } + } catch { + // Skill install must never fail login. + } +} + /** * Auto-provision a staging environment after login. * @@ -225,6 +252,10 @@ export async function runLogin(): Promise { } else { clack.log.info(chalk.dim('Run `workos env add` to configure an environment manually')); } + + // Best-effort skill install. Wrapped helper guarantees login never + // fails on skill errors. + await installSkillsAfterLogin(); return; } From e49149ebaaaa2151630419a88c2c8660dc047a1c Mon Sep 17 00:00:00 2001 From: Nick Nisi Date: Sun, 26 Apr 2026 14:52:40 -0500 Subject: [PATCH 04/10] feat(doctor): add --fix to refresh stale WorkOS skills MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 3 of cus-feedback-fixes. `workos doctor` (no flag) keeps its warn-only behavior — `--fix` is the explicit opt-in for mutating state from a diagnostic command. When the user runs `workos doctor --fix` and at least one agent reports a stale or missing version marker, refresh is scoped to a hardcoded allowlist of bundled WorkOS skills (`workos`, `workos-widgets`) and a per-agent before/after summary is rendered. * `FIXABLE_SKILLS` is hardcoded `['workos', 'workos-widgets']` — NOT derived from `discoverSkills()`. A future bundled skill needs an explicit opt-in here before doctor `--fix` will write to its target directory. This is the contract's promise that `--fix` only ever touches `workos/` and `workos-widgets/`. * `maybeRefreshSkills(options, skills)` extracted as a testable helper. Calls Phase 2's `refreshWorkOSSkills` with the allowlist via the `skills` filter, then re-runs `checkSkills()` so issue detection sees the post-refresh marker state. Without the re-run, `detectIssues` would still report `SKILLS_OUTDATED` immediately after fixing it. * Renderer in `output.ts` adds a "Skills" section that prints one line per agent: `✓ Updated WorkOS skills for {DisplayName}: {before} → {after}` (treating `null` as `(none)`). JSON mode picks up `skillsRefresh` automatically via `JSON.stringify(report)`. * Wired through `src/bin.ts` (yargs `--fix` boolean), `src/commands/ doctor.ts` (DoctorArgs pass-through), and `src/doctor/types.ts` (`fix?: boolean` on DoctorOptions; `SkillsRefreshResult` interface with before/after keyed by agent.name; `skillsRefresh?` on DoctorReport). * Tests in `src/doctor/checks/skills-fix.spec.ts`: - `--fix=false` does NOT call refresh even with stale skills - `--fix=true` is no-op when no skills info / when nothing is stale - allowlist passed to refreshWorkOSSkills exactly matches FIXABLE_SKILLS - success path returns skillsRefresh + post-refresh re-read of skills - refresh-returns-null preserves original skills (no skillsRefresh) - integration test against real refreshWorkOSSkills: planted third-party-skill and hypothetical workos-future-skill files at the agent target are byte-identical after `--fix` runs Open Item from spec deferred: whether `--fix` should also fix non-skill issues in the future. Out of scope here. --- src/bin.ts | 5 + src/commands/doctor.ts | 2 + src/doctor/checks/skills-fix.spec.ts | 197 +++++++++++++++++++++++++++ src/doctor/index.ts | 61 ++++++++- src/doctor/output.ts | 18 +++ src/doctor/types.ts | 18 +++ 6 files changed, 298 insertions(+), 3 deletions(-) create mode 100644 src/doctor/checks/skills-fix.spec.ts diff --git a/src/bin.ts b/src/bin.ts index 169811be..bdbf96bd 100644 --- a/src/bin.ts +++ b/src/bin.ts @@ -343,6 +343,11 @@ yargs(rawArgs) default: false, description: 'Copy report to clipboard', }, + fix: { + type: 'boolean', + default: false, + description: 'Auto-update stale WorkOS skills (writes to /skills/workos/ and workos-widgets/ only)', + }, }), async (argv) => { const { handleDoctor } = await import('./commands/doctor.js'); diff --git a/src/commands/doctor.ts b/src/commands/doctor.ts index 425aea85..419deee8 100644 --- a/src/commands/doctor.ts +++ b/src/commands/doctor.ts @@ -9,6 +9,7 @@ interface DoctorArgs { installDir?: string; json?: boolean; copy?: boolean; + fix?: boolean; } export async function handleDoctor(argv: ArgumentsCamelCase): Promise { @@ -19,6 +20,7 @@ export async function handleDoctor(argv: ArgumentsCamelCase): Promis skipAi: argv.skipAi ?? false, json: argv.json ?? false, copy: argv.copy ?? false, + fix: argv.fix ?? false, }; try { diff --git a/src/doctor/checks/skills-fix.spec.ts b/src/doctor/checks/skills-fix.spec.ts new file mode 100644 index 00000000..4af19f06 --- /dev/null +++ b/src/doctor/checks/skills-fix.spec.ts @@ -0,0 +1,197 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'fs'; +import { tmpdir } from 'os'; +import { join } from 'path'; +import type { SkillsInfo } from '../types.js'; + +// Mock the two collaborators maybeRefreshSkills depends on. +vi.mock('../../commands/install-skill.js', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + refreshWorkOSSkills: vi.fn(), + }; +}); + +vi.mock('./skills.js', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + checkSkills: vi.fn(), + }; +}); + +const { refreshWorkOSSkills } = await import('../../commands/install-skill.js'); +const { checkSkills } = await import('./skills.js'); +const { maybeRefreshSkills, FIXABLE_SKILLS } = await import('../index.js'); + +const STALE: SkillsInfo = { + bundledVersion: '0.4.0', + agents: [ + { agent: 'Claude Code', installedVersion: '0.2.4', stale: true }, + { agent: 'Codex', installedVersion: null, stale: false }, + ], +}; + +const CURRENT: SkillsInfo = { + bundledVersion: '0.4.0', + agents: [ + { agent: 'Claude Code', installedVersion: '0.4.0', stale: false }, + { agent: 'Codex', installedVersion: '0.4.0', stale: false }, + ], +}; + +describe('maybeRefreshSkills', () => { + beforeEach(() => { + vi.mocked(refreshWorkOSSkills).mockReset(); + vi.mocked(checkSkills).mockReset(); + }); + + it('does NOT call refresh when fix=false even with stale skills', async () => { + const result = await maybeRefreshSkills({ fix: false }, STALE); + + expect(refreshWorkOSSkills).not.toHaveBeenCalled(); + expect(checkSkills).not.toHaveBeenCalled(); + expect(result.skillsRefresh).toBeUndefined(); + expect(result.skills).toBe(STALE); + }); + + it('does NOT call refresh when skills info is undefined', async () => { + const result = await maybeRefreshSkills({ fix: true }, undefined); + + expect(refreshWorkOSSkills).not.toHaveBeenCalled(); + expect(result.skillsRefresh).toBeUndefined(); + expect(result.skills).toBeUndefined(); + }); + + it('does NOT call refresh when no agent is stale or missing-marker', async () => { + const result = await maybeRefreshSkills({ fix: true }, CURRENT); + + expect(refreshWorkOSSkills).not.toHaveBeenCalled(); + expect(result.skillsRefresh).toBeUndefined(); + expect(result.skills).toBe(CURRENT); + }); + + it('passes ONLY the FIXABLE_SKILLS allowlist into refreshWorkOSSkills', async () => { + vi.mocked(refreshWorkOSSkills).mockResolvedValueOnce(null); + + await maybeRefreshSkills({ fix: true }, STALE); + + expect(refreshWorkOSSkills).toHaveBeenCalledOnce(); + expect(refreshWorkOSSkills).toHaveBeenCalledWith({ + skills: [...FIXABLE_SKILLS], + }); + expect(FIXABLE_SKILLS).toEqual(['workos', 'workos-widgets']); + }); + + it('returns skillsRefresh and post-refresh skills when refresh succeeds', async () => { + vi.mocked(refreshWorkOSSkills).mockResolvedValueOnce({ + // refreshWorkOSSkills's return shape — only the fields maybeRefreshSkills reads matter here. + agents: [], + skills: ['workos', 'workos-widgets'], + version: '0.4.0', + perAgentBefore: { 'claude-code': '0.2.4', codex: null }, + perAgentAfter: { 'claude-code': '0.4.0', codex: '0.4.0' }, + }); + vi.mocked(checkSkills).mockReturnValueOnce({ + bundledVersion: '0.4.0', + agents: [ + { agent: 'Claude Code', installedVersion: '0.4.0', stale: false }, + { agent: 'Codex', installedVersion: '0.4.0', stale: false }, + ], + }); + + const result = await maybeRefreshSkills({ fix: true }, STALE); + + expect(result.skillsRefresh).toEqual({ + before: { 'claude-code': '0.2.4', codex: null }, + after: { 'claude-code': '0.4.0', codex: '0.4.0' }, + skillsInstalled: ['workos', 'workos-widgets'], + }); + // Re-read happens, so post-refresh state replaces the original. + expect(result.skills?.agents.every((a) => !a.stale)).toBe(true); + expect(checkSkills).toHaveBeenCalledOnce(); + }); + + it('returns the original skills (and no skillsRefresh) when refresh produces null', async () => { + // refreshWorkOSSkills returns null when no agents detected or all installs failed. + vi.mocked(refreshWorkOSSkills).mockResolvedValueOnce(null); + + const result = await maybeRefreshSkills({ fix: true }, STALE); + + expect(result.skillsRefresh).toBeUndefined(); + // Original skills preserved — we don't re-read on null refresh. + expect(result.skills).toBe(STALE); + expect(checkSkills).not.toHaveBeenCalled(); + }); +}); + +describe('--fix sibling protection (integration via real refreshWorkOSSkills)', () => { + let testDir: string; + let homeDir: string; + + beforeEach(async () => { + // The `vi.mock` calls above replace these for the unit-test suite; + // restore them here so this suite uses the real implementations. + vi.doUnmock('../../commands/install-skill.js'); + vi.doUnmock('./skills.js'); + vi.resetModules(); + + testDir = mkdtempSync(join(tmpdir(), 'doctor-fix-sibling-')); + homeDir = join(testDir, 'home'); + mkdirSync(homeDir); + }); + + afterEach(() => { + rmSync(testDir, { recursive: true, force: true }); + vi.resetAllMocks(); + vi.resetModules(); + }); + + it('only writes to skills on the FIXABLE_SKILLS allowlist; planted siblings are untouched', async () => { + // Re-import after unmocking so we get the real impls. The real + // refreshWorkOSSkills resolves its skill source via @workos/skills's + // getSkillsDir(), so we cannot mock the source side here — the meaningful + // assertion is target-side: pre-existing sibling skill dirs at the agent's + // target path must NOT be touched, regardless of what's in the bundled + // source. The opts.skills allowlist is what scopes refresh. + const { refreshWorkOSSkills: realRefresh, createAgents } = await import('../../commands/install-skill.js'); + const { FIXABLE_SKILLS: realFixable } = await import('../index.js'); + + const claudeSkillsDir = join(homeDir, '.claude/skills'); + mkdirSync(claudeSkillsDir, { recursive: true }); + + // Third-party skill — must survive the refresh. + mkdirSync(join(claudeSkillsDir, 'some-third-party-skill')); + writeFileSync(join(claudeSkillsDir, 'some-third-party-skill', 'SKILL.md'), '# Third Party (do not touch)'); + + // Hypothetical future bundled skill at the target but NOT on allowlist — + // also must survive (allowlist defends both source-side and target-side). + mkdirSync(join(claudeSkillsDir, 'workos-future-skill')); + writeFileSync(join(claudeSkillsDir, 'workos-future-skill', 'SKILL.md'), '# Future (do not touch)'); + + const explicitAgent = createAgents(homeDir)['claude-code']; + const result = await realRefresh({ + agents: [explicitAgent], + skills: [...realFixable], + writeMarker: false, + }); + + expect(result).not.toBeNull(); + // The result.skills set is the intersection of allowlist and what's actually + // bundled. workos and workos-widgets are both bundled today. + expect(result!.skills.sort()).toEqual(['workos', 'workos-widgets']); + + // Allowlist names WERE installed. + expect(existsSync(join(claudeSkillsDir, 'workos/SKILL.md'))).toBe(true); + expect(existsSync(join(claudeSkillsDir, 'workos-widgets/SKILL.md'))).toBe(true); + + // Siblings (whether unrelated or hypothetical-future) are byte-identical. + expect(readFileSync(join(claudeSkillsDir, 'some-third-party-skill/SKILL.md'), 'utf-8')).toBe( + '# Third Party (do not touch)', + ); + expect(readFileSync(join(claudeSkillsDir, 'workos-future-skill/SKILL.md'), 'utf-8')).toBe( + '# Future (do not touch)', + ); + }); +}); diff --git a/src/doctor/index.ts b/src/doctor/index.ts index b0bcf9a5..641047d2 100644 --- a/src/doctor/index.ts +++ b/src/doctor/index.ts @@ -8,15 +8,62 @@ import { checkDashboardSettings, compareRedirectUris } from './checks/dashboard. import { checkAuthPatterns } from './checks/auth-patterns.js'; import { checkAiAnalysis } from './checks/ai-analysis.js'; import { checkSkills } from './checks/skills.js'; +import { refreshWorkOSSkills } from '../commands/install-skill.js'; import { detectIssues } from './issues.js'; import { formatReport } from './output.js'; import { formatReportAsJson } from './json-output.js'; import { copyToClipboard } from './clipboard.js'; import Chalk from 'chalk'; -import type { DoctorOptions, DoctorReport } from './types.js'; +import type { DoctorOptions, DoctorReport, SkillsRefreshResult } from './types.js'; const DOCTOR_VERSION = '1.0.0'; +/** + * Skills `--fix` is allowed to refresh. Hardcoded — NOT derived from + * discoverSkills — so future bundled skills require an explicit opt-in here + * before doctor will write to their target directory. This is the contract's + * promise that `--fix` only ever touches `workos/` and `workos-widgets/`. + */ +export const FIXABLE_SKILLS = ['workos', 'workos-widgets'] as const; + +/** + * Refresh stale WorkOS skills if `--fix` is set and at least one agent is + * stale or has no marker. Always re-reads `checkSkills()` after a successful + * refresh so detectIssues sees the post-refresh state and we don't ship a + * doctor report that simultaneously claims "fixed" and "still stale". + * + * Extracted from runDoctor for unit testability — runDoctor itself depends on + * eight upstream checks that are expensive to mock. + */ +export async function maybeRefreshSkills( + options: Pick, + skills: DoctorReport['skills'], +): Promise<{ + skillsRefresh?: SkillsRefreshResult; + skills: DoctorReport['skills']; +}> { + if (!options.fix || !skills) return { skills }; + + const stalePresent = skills.agents.some((a) => a.stale || a.installedVersion === null); + if (!stalePresent) return { skills }; + + const refresh = await refreshWorkOSSkills({ + // Explicit allowlist — NOT discoverSkills — so the contract's + // workos/+workos-widgets-only constraint can't drift. + skills: [...FIXABLE_SKILLS], + }); + if (!refresh) return { skills }; + + return { + skillsRefresh: { + before: refresh.perAgentBefore, + after: refresh.perAgentAfter, + skillsInstalled: refresh.skills, + }, + skills: checkSkills() ?? undefined, + }; +} + export async function runDoctor(options: DoctorOptions): Promise { // Environment check first - loads project's .env/.env.local files // Must run before connectivity so the resolved base URL is available @@ -31,7 +78,7 @@ export async function runDoctor(options: DoctorOptions): Promise { checkLanguage(options.installDir), ]); - const skills = checkSkills() ?? undefined; + let skills = checkSkills() ?? undefined; // Dashboard settings + auth patterns + AI analysis (parallel, all need sdk/framework results) // AI analysis also receives early issues as context to avoid duplication @@ -70,6 +117,13 @@ export async function runDoctor(options: DoctorOptions): Promise { ? compareRedirectUris(expectedRedirectUri, dashboardResult.settings.redirectUris, redirectUriSource) : undefined; + // `--fix`: refresh stale WorkOS skills BEFORE issue detection so the report + // reflects the post-refresh state (no lingering SKILLS_OUTDATED warning + // after the refresh has already fixed it). + const refreshOutcome = await maybeRefreshSkills(options, skills); + const skillsRefresh = refreshOutcome.skillsRefresh; + skills = refreshOutcome.skills; + // Build partial report const partialReport = { version: DOCTOR_VERSION, @@ -91,9 +145,10 @@ export async function runDoctor(options: DoctorOptions): Promise { authPatterns, aiAnalysis, skills, + skillsRefresh, }; - // Detect issues based on collected data + // Detect issues based on (post-refresh) data. const issues = detectIssues(partialReport); // Calculate summary diff --git a/src/doctor/output.ts b/src/doctor/output.ts index 8ccacbb9..ff9ddac9 100644 --- a/src/doctor/output.ts +++ b/src/doctor/output.ts @@ -1,4 +1,6 @@ import Chalk from 'chalk'; +import { homedir } from 'os'; +import { createAgents } from '../commands/install-skill.js'; import type { DoctorReport, Issue } from './types.js'; import { renderSummaryBox, type SummaryBoxItem } from '../utils/summary-box.js'; import type { LockExpression } from '../utils/lock-art.js'; @@ -150,6 +152,22 @@ export function formatReport(report: DoctorReport, options?: FormatOptions): voi } } + // Skills refresh (--fix path) + if (report.skillsRefresh) { + const agents = createAgents(homedir()); + const displayName = (slug: string): string => agents[slug]?.displayName ?? slug; + const formatVersion = (v: string | null): string => v ?? '(none)'; + + console.log(''); + console.log('Skills'); + const slugs = Object.keys(report.skillsRefresh.before); + for (const slug of slugs) { + const before = formatVersion(report.skillsRefresh.before[slug] ?? null); + const after = formatVersion(report.skillsRefresh.after[slug] ?? null); + console.log(` ${Chalk.green('✓')} Updated WorkOS skills for ${displayName(slug)}: ${before} → ${after}`); + } + } + // Verbose mode additions if (options?.verbose) { console.log(''); diff --git a/src/doctor/types.ts b/src/doctor/types.ts index f1f8e7c9..d87c0efd 100644 --- a/src/doctor/types.ts +++ b/src/doctor/types.ts @@ -119,6 +119,20 @@ export interface SkillsInfo { agents: SkillAgentStatus[]; } +/** + * Result of `workos doctor --fix` refreshing WorkOS skills. Captured per agent + * so the human-mode renderer can show a before/after line and the JSON consumer + * can reason about which agents changed. + */ +export interface SkillsRefreshResult { + /** Marker version per agent.name BEFORE refresh (null = no marker). */ + before: Record; + /** Marker version per agent.name AFTER refresh. */ + after: Record; + /** Skills the refresh was scoped to (the FIXABLE_SKILLS allowlist). */ + skillsInstalled: string[]; +} + export interface DoctorReport { version: string; timestamp: string; @@ -139,6 +153,8 @@ export interface DoctorReport { authPatterns?: AuthPatternInfo; aiAnalysis?: AiAnalysis; skills?: SkillsInfo; + /** Present only when `--fix` actually performed a refresh. */ + skillsRefresh?: SkillsRefreshResult; issues: Issue[]; summary: { errors: number; @@ -171,4 +187,6 @@ export interface DoctorOptions { skipAi?: boolean; json?: boolean; copy?: boolean; + /** When true, refresh stale WorkOS skills (constrained to workos/ + workos-widgets/). */ + fix?: boolean; } From f0e4f9b08f0470896591e55439eb603394d33239 Mon Sep 17 00:00:00 2001 From: Nick Nisi Date: Sun, 26 Apr 2026 15:48:10 -0500 Subject: [PATCH 05/10] chore(deps): bump @workos/skills from 0.2.4 to 0.4.0 Phase 1 of cus-feedback-fixes is now live on npm. Pulling the new content in so the recursive prune-replace install machinery added in Phase 2 actually delivers the new guardrails: * references/workos-cli-upgrade.md (CLI upgrade-path topic) * SKILL.md routing rule for outdated-CLI symptoms * references/workos-management.md "Detecting CLI upgrades" subsection Exact pin matching the prior 0.2.4 convention. --- package.json | 2 +- pnpm-lock.yaml | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/package.json b/package.json index b60dbde3..582f2ee2 100644 --- a/package.json +++ b/package.json @@ -51,7 +51,7 @@ "@clack/prompts": "1.0.1", "@napi-rs/keyring": "^1.2.0", "@workos-inc/node": "^8.7.0", - "@workos/skills": "0.2.4", + "@workos/skills": "0.4.0", "chalk": "^5.6.2", "diff": "^8.0.3", "fast-glob": "^3.3.3", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 9f6d9516..38de34b7 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -30,8 +30,8 @@ importers: specifier: ^8.7.0 version: 8.7.0 '@workos/skills': - specifier: 0.2.4 - version: 0.2.4 + specifier: 0.4.0 + version: 0.4.0 chalk: specifier: ^5.6.2 version: 5.6.2 @@ -1071,8 +1071,8 @@ packages: resolution: {integrity: sha512-43HfXSR2Ez7M4ixpebuYVZzZf3gauh5jvv9lYnePg/x0XZMN2hjpEV3FD1LQX1vfMbqQ5gON3DN+/gH2rITm3A==} engines: {node: '>=20.15.0'} - '@workos/skills@0.2.4': - resolution: {integrity: sha512-Ku/8aD6zRwA7M4jREp/l1pUHOGCmDMBudlJAUfpkfZqFdDatxWJy+CQmYiIlMKt3IP4/GURe1rA72HpAcEkcaw==} + '@workos/skills@0.4.0': + resolution: {integrity: sha512-pH562ybfmXid9GcKsmer8bH/mNezhqIvXnwNN0T3UnR+m/ZHfBmiygI27GAxIf8Kl9ppNbOrTSGAVgzw1bG3Gg==} ansi-escapes@7.3.0: resolution: {integrity: sha512-BvU8nYgGQBxcmMuEeUEmNTvrMVjJNSH7RgW24vXexN4Ven6qCvy4TntnvlnwnMLTVlcRQQdbRY8NKnaIoeWDNg==} @@ -2515,7 +2515,7 @@ snapshots: iron-webcrypto: 2.0.0 jose: 6.1.3 - '@workos/skills@0.2.4': + '@workos/skills@0.4.0': dependencies: yaml: 2.8.3 From 147a8e8a69b33f6a406a7ae074ff0a32736c2810 Mon Sep 17 00:00:00 2001 From: Nick Nisi Date: Sun, 26 Apr 2026 15:52:07 -0500 Subject: [PATCH 06/10] chore: formatting --- src/commands/install-skill.ts | 3 +-- src/commands/login.ts | 4 +--- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/src/commands/install-skill.ts b/src/commands/install-skill.ts index 2163944c..0b338ae2 100644 --- a/src/commands/install-skill.ts +++ b/src/commands/install-skill.ts @@ -161,8 +161,7 @@ async function cleanupStaleOrphans(parent: string, skillName: string): Promise []); const cutoff = Date.now() - ORPHAN_STALE_MS; for (const entry of entries) { - const isOrphan = - entry.startsWith(`.workos.tmp-${skillName}-`) || entry.startsWith(`.workos.bak-${skillName}-`); + const isOrphan = entry.startsWith(`.workos.tmp-${skillName}-`) || entry.startsWith(`.workos.bak-${skillName}-`); if (!isOrphan) continue; const path = join(parent, entry); const st = await stat(path).catch(() => null); diff --git a/src/commands/login.ts b/src/commands/login.ts index 9edf54e3..31a33338 100644 --- a/src/commands/login.ts +++ b/src/commands/login.ts @@ -88,9 +88,7 @@ export async function installSkillsAfterLogin(): Promise { const result = await autoInstallSkills(); if (result && !isJsonMode()) { const skillWord = result.skills.length === 1 ? 'skill' : 'skills'; - clack.log.info( - `Installed ${result.skills.length} WorkOS ${skillWord} for ${result.agents.join(', ')}.`, - ); + clack.log.info(`Installed ${result.skills.length} WorkOS ${skillWord} for ${result.agents.join(', ')}.`); } } catch { // Skill install must never fail login. From 0a7b96350202d3a2ebdf994c56a8f0f135c4cef6 Mon Sep 17 00:00:00 2001 From: Nick Nisi Date: Sun, 26 Apr 2026 16:12:08 -0500 Subject: [PATCH 07/10] fix: address PR review feedback (CodeRabbit / Greptile / Codex) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review surface findings on PR #130. Each fix below addresses a specific reviewer comment. * `refreshWorkOSSkills` no longer over-reports installed skill count. Tracks the union of skills that succeeded for at least one agent and returns that as `RefreshResult.skills` instead of the full filtered attempt list. Fixes the "Installed N skills" line inflating when some skills failed to copy. (Greptile P1, CodeRabbit Major) * `runInstallSkill` (the explicit `workos skills install` command) now writes per-agent `.workos-skill-version` markers after success, so `workos doctor` doesn't immediately re-flag the freshly-installed skills as stale. Closes the loop where the SKILLS_OUTDATED remediation pointed at an install path that didn't update markers. (Codex P2, CodeRabbit Major) * `installSkill` setup operations (mkdir, mkdtemp, cleanupStaleOrphans) now run inside the try block. Filesystem errors before the copy (EACCES, ENOTDIR, etc.) surface as `{ success: false }` instead of rejecting — both runInstallSkill and refreshWorkOSSkills accumulate per-(skill, agent) failures, and a single bad agent dir would otherwise abort the whole batch (and now: abort the entire `doctor --fix` run). (Codex P2) * `checkSkills` only reports agents that actually have a WorkOS skill installed (marker file OR `workos/` subdir present), instead of any agent with a `skills/` directory. Without this, a Claude Code user who had unrelated skills but never installed WorkOS would have `installedVersion === null` reported, which `--fix` would then interpret as "missing marker, refresh" and write `workos/` + `workos-widgets/` to that agent unprompted. (Codex P2) * Staleness check uses semver ordering instead of string inequality. String comparison would flag `installed > bundled` (downgrade scenario: user installed via newer CLI then downgraded) as stale and the SKILLS_OUTDATED remediation would silently downgrade their agent's skills. Falls back to string inequality when either version is non-semver. (Greptile P2) * `installSkillsAfterLogin` wraps `autoInstallSkills` in a 30s Promise.race timeout. The hook can no longer block login completion on a hung filesystem call. (CodeRabbit Major; was an open item in the spec) * `maybeRefreshSkills` runs BEFORE `earlyIssues` and AI analysis in runDoctor, so AI prompt context (and downstream issue detection) see the post-refresh state. Without this move, a successful `--fix` could clear `report.issues` while `report.aiAnalysis` still references the just-fixed SKILLS_OUTDATED warning. (CodeRabbit Major, outside-diff finding) * `--fix` flag now appears in `workos doctor --help --json` via `src/utils/help-json.ts`. The CLI intercepts `--help --json` from this static registry; without the entry, automation/agent consumers (the same audience this whole feature targets) wouldn't discover `--fix` from machine-readable help. (Codex P3) * `install.spec.ts` mocks for `autoInstallSkills` now include the required `version` field, matching the AutoInstallResult type. (Greptile P2) * `install-skill.spec.ts` "writes a version marker per agent when the bundled version is resolvable" now plants a deterministic `/package.json` + `/plugins/workos/skills/` layout so `getBundledSkillsVersion` returns a known value (`9.9.9`) and the marker-write success path is genuinely exercised. (CodeRabbit Minor) Pushed back / dismissed: * CodeRabbit Critical "0.4.0 doesn't exist": false positive. The registry data the bot's web search saw was stale; `npm view @workos/skills@0.4.0 version` returns `0.4.0` and `pnpm install` resolves cleanly. * CodeRabbit Major "use async fs in getBundledSkillsVersion": the file's pre-existing convention is sync APIs for one-shot path probes (`existsSync`, `readFileSync` in checkSkills, install-skill). Converting one function would cascade through `checkSkills` → `runDoctor` for marginal benefit. Keeping consistent. --- src/commands/install-skill.spec.ts | 38 ++++++++++-------- src/commands/install-skill.ts | 64 +++++++++++++++++++++--------- src/commands/install.spec.ts | 2 + src/commands/login.ts | 11 +++-- src/doctor/checks/skills.spec.ts | 16 +++++++- src/doctor/checks/skills.ts | 34 +++++++++++++--- src/doctor/index.ts | 15 +++---- src/utils/help-json.ts | 8 ++++ 8 files changed, 134 insertions(+), 54 deletions(-) diff --git a/src/commands/install-skill.spec.ts b/src/commands/install-skill.spec.ts index 1b65bd43..98539ba9 100644 --- a/src/commands/install-skill.spec.ts +++ b/src/commands/install-skill.spec.ts @@ -387,29 +387,33 @@ describe('install-skill', () => { }); it('writes a version marker per agent when the bundled version is resolvable', async () => { - // The real @workos/skills package is present in node_modules, so - // getBundledSkillsVersion should succeed against the real skillsDir layout. - // Here we just verify that IF a version can be determined, it gets written. + // Plant a deterministic package layout so getBundledSkillsVersion finds + // a real version. The function walks up 3 dirnames from skillsDir to + // locate the package.json, so we mirror that layout here: + // /package.json ← version source + // /plugins/workos/skills ← skillsDir const { SKILL_VERSION_MARKER_FILENAME } = await import('./install-skill.js'); + const { getSkillsDir } = await import('@workos/skills'); + + const packageRoot = join(testDir, 'pkg'); + mkdirSync(packageRoot); + writeFileSync(join(packageRoot, 'package.json'), JSON.stringify({ name: '@workos/skills', version: '9.9.9' })); + const deepSkillsDir = join(packageRoot, 'plugins/workos/skills'); + mkdirSync(deepSkillsDir, { recursive: true }); + mkdirSync(join(deepSkillsDir, 'skill-a')); + writeFileSync(join(deepSkillsDir, 'skill-a', 'SKILL.md'), '# Skill A'); + vi.mocked(getSkillsDir).mockReturnValue(deepSkillsDir); - mkdirSync(join(skillsDir, 'skill-a')); - writeFileSync(join(skillsDir, 'skill-a', 'SKILL.md'), '# Skill A'); mkdirSync(join(homeDir, '.claude')); const result = await autoInstallSkills(); - // Our test skillsDir is not a real npm package, so bundled version is null. - // That's fine — the marker is only written when version is resolvable, - // and result.version reflects what was written. - if (result?.version) { - const marker = join(homeDir, '.claude/skills', SKILL_VERSION_MARKER_FILENAME); - expect(existsSync(marker)).toBe(true); - expect(readFileSync(marker, 'utf8')).toBe(result.version); - } else { - // Marker should NOT be written when version is unknown. - const marker = join(homeDir, '.claude/skills', SKILL_VERSION_MARKER_FILENAME); - expect(existsSync(marker)).toBe(false); - } + expect(result).not.toBeNull(); + expect(result!.version).toBe('9.9.9'); + + const marker = join(homeDir, '.claude/skills', SKILL_VERSION_MARKER_FILENAME); + expect(existsSync(marker)).toBe(true); + expect(readFileSync(marker, 'utf8')).toBe('9.9.9'); }); it('returns null when no agents are detected', async () => { diff --git a/src/commands/install-skill.ts b/src/commands/install-skill.ts index 0b338ae2..f98ecead 100644 --- a/src/commands/install-skill.ts +++ b/src/commands/install-skill.ts @@ -112,16 +112,21 @@ export async function installSkill( const targetDir = join(agent.globalSkillsDir, skillName); const parent = dirname(targetDir); - await mkdir(parent, { recursive: true }); - // Best-effort cleanup of OLD orphans only — never current-run paths. - await cleanupStaleOrphans(parent, skillName).catch(() => {}); + // Setup (mkdir parent, mkdtemp) is inside the try so EACCES / ENOTDIR / etc. + // surface as `{ success: false }` rather than rejecting — runInstallSkill and + // refreshWorkOSSkills accumulate failures across the (skill × agent) matrix + // and would otherwise abort the whole batch on a single bad agent dir. + let tempDir: string | undefined; + try { + await mkdir(parent, { recursive: true }); + // Best-effort cleanup of OLD orphans only — never current-run paths. + await cleanupStaleOrphans(parent, skillName).catch(() => {}); - // mkdtemp gives us atomic creation + a random suffix that prevents - // collisions between concurrent installers. - const tempDir = await mkdtemp(join(parent, `.workos.tmp-${skillName}-`)); - const backupDir = tempDir.replace('.workos.tmp-', '.workos.bak-'); + // mkdtemp gives us atomic creation + a random suffix that prevents + // collisions between concurrent installers. + tempDir = await mkdtemp(join(parent, `.workos.tmp-${skillName}-`)); + const backupDir = tempDir.replace('.workos.tmp-', '.workos.bak-'); - try { await cp(sourceDir, tempDir, { recursive: true, errorOnExist: false }); const targetExisted = existsSync(targetDir); @@ -143,7 +148,9 @@ export async function installSkill( } return { success: true }; } catch (error) { - await rm(tempDir, { recursive: true, force: true }).catch(() => {}); + if (tempDir) { + await rm(tempDir, { recursive: true, force: true }).catch(() => {}); + } return { success: false, error: error instanceof Error ? error.message : 'Unknown error', @@ -201,7 +208,7 @@ export async function runInstallSkill(options: InstallSkillOptions): Promise = []; @@ -209,11 +216,7 @@ export async function runInstallSkill(options: InstallSkillOptions): Promise 0) { console.log(chalk.green(`✓ Installed ${successful.length} skill(s):\n`)); for (const r of successful) { - console.log(` ${chalk.cyan(r.skill)} → ${chalk.dim(r.agent)}`); + console.log(` ${chalk.cyan(r.skill)} → ${chalk.dim(r.agent.displayName)}`); + } + } + + // Write per-agent version markers for any agent that had at least one + // successful install, so `workos doctor` doesn't immediately flag the + // freshly-installed skills as stale or missing. + const version = getBundledSkillsVersion(skillsDir); + if (version) { + const succeededAgents = new Set(); + for (const r of successful) succeededAgents.add(r.agent); + for (const agent of succeededAgents) { + try { + await writeFile(join(agent.globalSkillsDir, SKILL_VERSION_MARKER_FILENAME), version, 'utf8'); + } catch { + // Marker is best-effort; doctor treats missing marker as "unknown". + } } } if (failed.length > 0) { console.log(chalk.red(`\n✗ Failed to install ${failed.length}:\n`)); for (const r of failed) { - console.log(` ${r.skill} → ${r.agent}: ${chalk.dim(r.error)}`); + console.log(` ${r.skill} → ${r.agent.displayName}: ${chalk.dim(r.error)}`); } process.exit(1); } @@ -300,6 +319,10 @@ export async function refreshWorkOSSkills(opts: RefreshOptions = {}): Promise = {}; const perAgentAfter: Record = {}; const succeededAgents: AgentConfig[] = []; + // Union of skills that succeeded for at least one agent. Returning the full + // attempted list would inflate "Installed N skills" copy when some skills + // failed to copy; only count what actually landed somewhere. + const installedSkills = new Set(); for (const agent of detected) { perAgentBefore[agent.name] = readSkillVersionMarker(agent); @@ -307,7 +330,10 @@ export async function refreshWorkOSSkills(opts: RefreshOptions = {}): Promise installedSkills.has(s)), version, perAgentBefore, perAgentAfter, diff --git a/src/commands/install.spec.ts b/src/commands/install.spec.ts index c46b9fc1..2e3031ff 100644 --- a/src/commands/install.spec.ts +++ b/src/commands/install.spec.ts @@ -59,6 +59,7 @@ describe('handleInstall', () => { vi.mocked(autoInstallSkills).mockResolvedValue({ skills: ['workos', 'workos-widgets'], agents: ['Claude Code'], + version: '0.4.0', }); vi.mocked(isJsonMode).mockReturnValue(false); @@ -82,6 +83,7 @@ describe('handleInstall', () => { vi.mocked(autoInstallSkills).mockResolvedValue({ skills: ['workos'], agents: ['Claude Code'], + version: '0.4.0', }); vi.mocked(isJsonMode).mockReturnValue(true); diff --git a/src/commands/login.ts b/src/commands/login.ts index 31a33338..98d334f2 100644 --- a/src/commands/login.ts +++ b/src/commands/login.ts @@ -76,16 +76,19 @@ function sleep(ms: number): Promise { * Best-effort skill install after a successful auth-login. * * Mirrors the install.ts hook copy, but wraps `autoInstallSkills` in its own - * try/catch so a skill install failure (or hang inside install) NEVER fails - * the login itself. Login already succeeded by the time this runs — the user - * having a working session is the contract that must hold. + * try/catch AND a 30s timeout so a skill install hang (e.g. blocked filesystem + * call) never blocks login completion. Login already succeeded by the time + * this runs — the user having a working session is the contract that must hold. * * Extracted from runLogin so it can be unit-tested without standing up the * device-auth polling loop. */ +export const SKILL_INSTALL_TIMEOUT_MS = 30 * 1000; + export async function installSkillsAfterLogin(): Promise { try { - const result = await autoInstallSkills(); + const timeout = new Promise((resolve) => setTimeout(() => resolve(null), SKILL_INSTALL_TIMEOUT_MS)); + const result = await Promise.race([autoInstallSkills(), timeout]); if (result && !isJsonMode()) { const skillWord = result.skills.length === 1 ? 'skill' : 'skills'; clack.log.info(`Installed ${result.skills.length} WorkOS ${skillWord} for ${result.agents.join(', ')}.`); diff --git a/src/doctor/checks/skills.spec.ts b/src/doctor/checks/skills.spec.ts index 8d5ae248..dec19875 100644 --- a/src/doctor/checks/skills.spec.ts +++ b/src/doctor/checks/skills.spec.ts @@ -33,8 +33,20 @@ describe('checkSkills', () => { expect(checkSkills(testHome)).toBeNull(); }); - it('reports an agent with no marker as installedVersion=null and not stale', () => { - mkdirSync(join(testHome, '.claude/skills'), { recursive: true }); + it('returns null when an agent skills dir exists but has no WorkOS skills (no marker, no workos/)', () => { + // The agent has its skills/ dir for unrelated user-installed skills. We + // must NOT report it as having WorkOS skills — `doctor --fix` would + // otherwise write workos/ + workos-widgets/ onto an agent that never + // opted in. Marker OR workos/ subdir is the signal. + mkdirSync(join(testHome, '.claude/skills/some-other-skill'), { recursive: true }); + writeFileSync(join(testHome, '.claude/skills/some-other-skill/SKILL.md'), '# Other'); + + expect(checkSkills(testHome)).toBeNull(); + }); + + it('reports an agent with workos/ subdir but no marker as installedVersion=null and not stale', () => { + // Pre-Phase-2 install (only SKILL.md was copied) — a real possible state. + mkdirSync(join(testHome, '.claude/skills/workos'), { recursive: true }); const result = checkSkills(testHome); diff --git a/src/doctor/checks/skills.ts b/src/doctor/checks/skills.ts index f8f9062e..82cdf007 100644 --- a/src/doctor/checks/skills.ts +++ b/src/doctor/checks/skills.ts @@ -1,9 +1,29 @@ import { homedir } from 'node:os'; import { readFileSync, existsSync } from 'node:fs'; import { join } from 'node:path'; +import semver from 'semver'; import { createAgents, getBundledSkillsVersion, SKILL_VERSION_MARKER_FILENAME } from '../../commands/install-skill.js'; import type { SkillsInfo, SkillAgentStatus } from '../types.js'; +/** + * Stale = installed version is strictly older than the bundled version. + * String inequality would also fire when installed > bundled (user installed + * via a newer CLI then downgraded), and the SKILLS_OUTDATED remediation would + * silently downgrade their agent's skills. Use semver ordering so we only + * recommend an update when the bundled set is actually ahead. + * + * Falls back to string inequality when either version can't be parsed as + * semver — better to flag a possibly-stale skill than to ignore drift entirely. + */ +function isStale(installed: string, bundled: string): boolean { + const installedValid = semver.valid(installed); + const bundledValid = semver.valid(bundled); + if (installedValid && bundledValid) { + return semver.lt(installedValid, bundledValid); + } + return installed !== bundled; +} + /** * Check the freshness of auto-installed WorkOS skills across detected coding * agents. Compares each agent's version marker (written by autoInstallSkills) @@ -18,11 +38,15 @@ export function checkSkills(home: string = homedir()): SkillsInfo | null { const statuses: SkillAgentStatus[] = []; for (const [, agent] of Object.entries(agents)) { - // Only report on agents that actually have our skills dir laid down. - // An agent directory existing (e.g. ~/.claude) doesn't imply we installed. - if (!existsSync(agent.globalSkillsDir)) continue; - + // Only report on agents that actually have a WorkOS skill installed. + // An agent's `skills/` dir existing (e.g. for unrelated user-installed + // skills) doesn't mean WE installed — and `doctor --fix` would otherwise + // happily write `workos/` and `workos-widgets/` onto an agent that never + // opted in. The marker OR an existing `workos/` skill subdir is the signal. const markerPath = join(agent.globalSkillsDir, SKILL_VERSION_MARKER_FILENAME); + const workosSkillDir = join(agent.globalSkillsDir, 'workos'); + if (!existsSync(markerPath) && !existsSync(workosSkillDir)) continue; + let installedVersion: string | null = null; if (existsSync(markerPath)) { try { @@ -35,7 +59,7 @@ export function checkSkills(home: string = homedir()): SkillsInfo | null { statuses.push({ agent: agent.displayName, installedVersion, - stale: Boolean(bundledVersion && installedVersion && installedVersion !== bundledVersion), + stale: Boolean(bundledVersion && installedVersion && isStale(installedVersion, bundledVersion)), }); } diff --git a/src/doctor/index.ts b/src/doctor/index.ts index 641047d2..6ea8f320 100644 --- a/src/doctor/index.ts +++ b/src/doctor/index.ts @@ -80,6 +80,14 @@ export async function runDoctor(options: DoctorOptions): Promise { let skills = checkSkills() ?? undefined; + // `--fix`: refresh stale WorkOS skills BEFORE earlyIssues + AI analysis so + // every downstream consumer (issue detection, AI prompt context) sees the + // post-refresh skill state and doesn't reference a SKILLS_OUTDATED warning + // that was just resolved. + const refreshOutcome = await maybeRefreshSkills(options, skills); + const skillsRefresh = refreshOutcome.skillsRefresh; + skills = refreshOutcome.skills; + // Dashboard settings + auth patterns + AI analysis (parallel, all need sdk/framework results) // AI analysis also receives early issues as context to avoid duplication const earlyIssues = detectIssues({ @@ -117,13 +125,6 @@ export async function runDoctor(options: DoctorOptions): Promise { ? compareRedirectUris(expectedRedirectUri, dashboardResult.settings.redirectUris, redirectUriSource) : undefined; - // `--fix`: refresh stale WorkOS skills BEFORE issue detection so the report - // reflects the post-refresh state (no lingering SKILLS_OUTDATED warning - // after the refresh has already fixed it). - const refreshOutcome = await maybeRefreshSkills(options, skills); - const skillsRefresh = refreshOutcome.skillsRefresh; - skills = refreshOutcome.skills; - // Build partial report const partialReport = { version: DOCTOR_VERSION, diff --git a/src/utils/help-json.ts b/src/utils/help-json.ts index 660d03e4..c8ef2796 100644 --- a/src/utils/help-json.ts +++ b/src/utils/help-json.ts @@ -225,6 +225,14 @@ const commands: CommandSchema[] = [ default: false, hidden: false, }, + { + name: 'fix', + type: 'boolean', + description: 'Auto-update stale WorkOS skills (writes to /skills/workos/ and workos-widgets/ only)', + required: false, + default: false, + hidden: false, + }, ], }, { From 7dc8deb75c21cfbb0820d6b2bf1c55dad9eabd9b Mon Sep 17 00:00:00 2001 From: Nick Nisi Date: Sun, 26 Apr 2026 16:45:18 -0500 Subject: [PATCH 08/10] fix: address remaining CodeRabbit findings on PR #130 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings I missed in the previous review-fix pass plus the broader sync-fs cascade I had pushed back on. Reconsidered after re-reading CLAUDE.md's 'Avoid Node-specific sync APIs' guideline as a project rule rather than a suggestion. * `installSkillsAfterLogin`'s setTimeout handle is now stored, both `unref()`'d on creation and `clearTimeout()`'d in a finally block. Without this, when `autoInstallSkills` won the race the pending timer kept the Node event loop alive for up to 30s and the CLI appeared to hang. (CodeRabbit, login.ts:91) * `checkSkills` now treats `/skills/workos-widgets/` as a pre-marker install signal in addition to `workos/`. An agent that only had `workos-widgets/` from an older explicit install was invisible to doctor under the previous narrowing. (CodeRabbit, skills.ts:48) * Sync fs APIs replaced with async equivalents in install-skill.ts and doctor/checks/skills.ts: - `getBundledSkillsVersion` → async (await readFile) - `readSkillVersionMarker` → async (await readFile + access-based pathExists) - `checkSkills` → async (cascades to runDoctor, maybeRefreshSkills) - `installSkill`'s `existsSync(targetDir)` → `pathExists` - `cleanupStaleOrphans`'s `existsSync(parent)` → `pathExists` - `discoverSkills` filter uses `Promise.all(pathExists(...))` - `existsSync` import retained only for `agent.detect()` callbacks (sync-by-design boolean predicates inside detection table) (CodeRabbit, install-skill.ts:28+132 and skills.ts:5) Plus an opportunistic cleanup that came out of the cascade: * Extracted `writeAgentSkillMarker` helper so `runInstallSkill` and `refreshWorkOSSkills` share the same best-effort marker semantics (single source of truth, no behavioral drift). Partially addresses CodeRabbit's "route runInstallSkill through refreshWorkOSSkills" — the marker-write logic is the actual functional duplication and is now shared; the install loop itself stays separate because runInstallSkill needs granular per-(skill, agent) failure rendering that the primitive doesn't expose. Tests: - `skills.spec.ts` updated for async checkSkills + new test for workos-widgets-only install + new test for downgrade scenario (installed > bundled must NOT be flagged stale, semver fix) - `skills-fix.spec.ts` switched to mockResolvedValueOnce for the now async checkSkills mock - All 1614 tests pass --- src/commands/install-skill.ts | 69 ++++++++++++++++++---------- src/commands/login.ts | 10 +++- src/doctor/checks/skills-fix.spec.ts | 2 +- src/doctor/checks/skills.spec.ts | 57 ++++++++++++++++------- src/doctor/checks/skills.ts | 31 ++++++++++--- src/doctor/index.ts | 4 +- 6 files changed, 120 insertions(+), 53 deletions(-) diff --git a/src/commands/install-skill.ts b/src/commands/install-skill.ts index f98ecead..3996deae 100644 --- a/src/commands/install-skill.ts +++ b/src/commands/install-skill.ts @@ -1,7 +1,7 @@ import { homedir } from 'os'; import { dirname, join } from 'path'; -import { existsSync, readFileSync } from 'fs'; -import { mkdir, mkdtemp, cp, rename, rm, readdir, stat, writeFile } from 'fs/promises'; +import { existsSync } from 'fs'; +import { mkdir, mkdtemp, cp, rename, rm, readdir, readFile, stat, access, writeFile } from 'fs/promises'; import chalk from 'chalk'; import { getSkillsDir as getSkillsPackageDir } from '@workos/skills'; @@ -12,6 +12,16 @@ export const SKILL_VERSION_MARKER_FILENAME = '.workos-skill-version'; // and must NOT be removed. const ORPHAN_STALE_MS = 60 * 60 * 1000; +/** Async equivalent of `existsSync` — `access` rejects with ENOENT when missing. */ +async function pathExists(p: string): Promise { + try { + await access(p); + return true; + } catch { + return false; + } +} + /** * Read the bundled @workos/skills version by walking up from the skills * directory to the package.json. The package's `exports` map doesn't expose @@ -19,11 +29,13 @@ const ORPHAN_STALE_MS = 60 * 60 * 1000; * Returns null if the version can't be determined — callers treat that as * "no marker written" rather than failing the install. */ -export function getBundledSkillsVersion(skillsDir: string = getSkillsPackageDir()): string | null { +export async function getBundledSkillsVersion( + skillsDir: string = getSkillsPackageDir(), +): Promise { try { // skillsDir = /plugins/workos/skills const packageRoot = dirname(dirname(dirname(skillsDir))); - const pkgJson = JSON.parse(readFileSync(join(packageRoot, 'package.json'), 'utf8')); + const pkgJson = JSON.parse(await readFile(join(packageRoot, 'package.json'), 'utf8')); return typeof pkgJson.version === 'string' ? pkgJson.version : null; } catch { return null; @@ -78,7 +90,9 @@ export function getSkillsDir(): string { export async function discoverSkills(skillsDir: string): Promise { const entries = await readdir(skillsDir, { withFileTypes: true }); - return entries.filter((e) => e.isDirectory() && existsSync(join(skillsDir, e.name, 'SKILL.md'))).map((e) => e.name); + const dirs = entries.filter((e) => e.isDirectory()); + const checks = await Promise.all(dirs.map((e) => pathExists(join(skillsDir, e.name, 'SKILL.md')))); + return dirs.filter((_, i) => checks[i]).map((e) => e.name); } export function detectAgents(agents: Record, filter?: string[]): AgentConfig[] { @@ -129,7 +143,7 @@ export async function installSkill( await cp(sourceDir, tempDir, { recursive: true, errorOnExist: false }); - const targetExisted = existsSync(targetDir); + const targetExisted = await pathExists(targetDir); if (targetExisted) { await rename(targetDir, backupDir); } @@ -164,7 +178,7 @@ export async function installSkill( * preserved — destroying them would race the other run's final rename. */ async function cleanupStaleOrphans(parent: string, skillName: string): Promise { - if (!existsSync(parent)) return; + if (!(await pathExists(parent))) return; const entries = await readdir(parent).catch(() => []); const cutoff = Date.now() - ORPHAN_STALE_MS; for (const entry of entries) { @@ -232,17 +246,14 @@ export async function runInstallSkill(options: InstallSkillOptions): Promise(); for (const r of successful) succeededAgents.add(r.agent); for (const agent of succeededAgents) { - try { - await writeFile(join(agent.globalSkillsDir, SKILL_VERSION_MARKER_FILENAME), version, 'utf8'); - } catch { - // Marker is best-effort; doctor treats missing marker as "unknown". - } + await writeAgentSkillMarker(agent, version); } } @@ -285,16 +296,28 @@ export interface RefreshResult { perAgentAfter: Record; } -function readSkillVersionMarker(agent: AgentConfig): string | null { +async function readSkillVersionMarker(agent: AgentConfig): Promise { const path = join(agent.globalSkillsDir, SKILL_VERSION_MARKER_FILENAME); - if (!existsSync(path)) return null; try { - return readFileSync(path, 'utf8').trim() || null; + return (await readFile(path, 'utf8')).trim() || null; } catch { return null; } } +/** + * Best-effort marker write — any failure is swallowed (filesystem permission + * errors shouldn't fail the install; doctor treats missing markers as "unknown"). + * Single source of truth for the .workos-skill-version write semantics. + */ +async function writeAgentSkillMarker(agent: AgentConfig, version: string): Promise { + try { + await writeFile(join(agent.globalSkillsDir, SKILL_VERSION_MARKER_FILENAME), version, 'utf8'); + } catch { + // Marker is best-effort; doctor treats missing marker as "unknown". + } +} + /** * Reusable primitive: discover bundled skills, install each one to each agent, * write per-agent version markers, and report before/after marker state. @@ -315,7 +338,7 @@ export async function refreshWorkOSSkills(opts: RefreshOptions = {}): Promise = {}; const perAgentAfter: Record = {}; const succeededAgents: AgentConfig[] = []; @@ -325,7 +348,7 @@ export async function refreshWorkOSSkills(opts: RefreshOptions = {}): Promise(); for (const agent of detected) { - perAgentBefore[agent.name] = readSkillVersionMarker(agent); + perAgentBefore[agent.name] = await readSkillVersionMarker(agent); let agentSucceeded = false; for (const skill of skills) { @@ -339,15 +362,11 @@ export async function refreshWorkOSSkills(opts: RefreshOptions = {}): Promise { export const SKILL_INSTALL_TIMEOUT_MS = 30 * 1000; export async function installSkillsAfterLogin(): Promise { + let timeoutHandle: ReturnType | undefined; try { - const timeout = new Promise((resolve) => setTimeout(() => resolve(null), SKILL_INSTALL_TIMEOUT_MS)); + const timeout = new Promise((resolve) => { + timeoutHandle = setTimeout(() => resolve(null), SKILL_INSTALL_TIMEOUT_MS); + // Don't keep the event loop alive on this timer — process should exit + // immediately if everything else has resolved. + timeoutHandle.unref?.(); + }); const result = await Promise.race([autoInstallSkills(), timeout]); if (result && !isJsonMode()) { const skillWord = result.skills.length === 1 ? 'skill' : 'skills'; @@ -95,6 +101,8 @@ export async function installSkillsAfterLogin(): Promise { } } catch { // Skill install must never fail login. + } finally { + if (timeoutHandle) clearTimeout(timeoutHandle); } } diff --git a/src/doctor/checks/skills-fix.spec.ts b/src/doctor/checks/skills-fix.spec.ts index 4af19f06..edfb503d 100644 --- a/src/doctor/checks/skills-fix.spec.ts +++ b/src/doctor/checks/skills-fix.spec.ts @@ -93,7 +93,7 @@ describe('maybeRefreshSkills', () => { perAgentBefore: { 'claude-code': '0.2.4', codex: null }, perAgentAfter: { 'claude-code': '0.4.0', codex: '0.4.0' }, }); - vi.mocked(checkSkills).mockReturnValueOnce({ + vi.mocked(checkSkills).mockResolvedValueOnce({ bundledVersion: '0.4.0', agents: [ { agent: 'Claude Code', installedVersion: '0.4.0', stale: false }, diff --git a/src/doctor/checks/skills.spec.ts b/src/doctor/checks/skills.spec.ts index dec19875..9aacc1b3 100644 --- a/src/doctor/checks/skills.spec.ts +++ b/src/doctor/checks/skills.spec.ts @@ -21,7 +21,7 @@ describe('checkSkills', () => { beforeEach(() => { testHome = mkdtempSync(join(tmpdir(), 'skills-check-')); - vi.mocked(getBundledSkillsVersion).mockReturnValue('0.3.0'); + vi.mocked(getBundledSkillsVersion).mockResolvedValue('0.3.0'); }); afterEach(() => { @@ -29,71 +29,94 @@ describe('checkSkills', () => { vi.restoreAllMocks(); }); - it('returns null when no agent skills directories exist', () => { - expect(checkSkills(testHome)).toBeNull(); + it('returns null when no agent skills directories exist', async () => { + expect(await checkSkills(testHome)).toBeNull(); }); - it('returns null when an agent skills dir exists but has no WorkOS skills (no marker, no workos/)', () => { + it('returns null when an agent skills dir exists but has no WorkOS skills (no marker, no workos/, no workos-widgets/)', async () => { // The agent has its skills/ dir for unrelated user-installed skills. We // must NOT report it as having WorkOS skills — `doctor --fix` would // otherwise write workos/ + workos-widgets/ onto an agent that never - // opted in. Marker OR workos/ subdir is the signal. + // opted in. Marker OR workos/ OR workos-widgets/ subdir is the signal. mkdirSync(join(testHome, '.claude/skills/some-other-skill'), { recursive: true }); writeFileSync(join(testHome, '.claude/skills/some-other-skill/SKILL.md'), '# Other'); - expect(checkSkills(testHome)).toBeNull(); + expect(await checkSkills(testHome)).toBeNull(); }); - it('reports an agent with workos/ subdir but no marker as installedVersion=null and not stale', () => { + it('reports an agent with workos/ subdir but no marker as installedVersion=null and not stale', async () => { // Pre-Phase-2 install (only SKILL.md was copied) — a real possible state. mkdirSync(join(testHome, '.claude/skills/workos'), { recursive: true }); - const result = checkSkills(testHome); + const result = await checkSkills(testHome); expect(result).not.toBeNull(); expect(result!.bundledVersion).toBe('0.3.0'); expect(result!.agents).toEqual([{ agent: 'Claude Code', installedVersion: null, stale: false }]); }); - it('flags an agent as stale when the marker trails the bundled version', () => { + it('reports an agent that only has workos-widgets/ (older explicit install)', async () => { + // workos-widgets is also on the FIXABLE_SKILLS allowlist; an agent that + // installed only it via an older CLI version should still be visible. + mkdirSync(join(testHome, '.claude/skills/workos-widgets'), { recursive: true }); + + const result = await checkSkills(testHome); + + expect(result).not.toBeNull(); + expect(result!.agents).toEqual([{ agent: 'Claude Code', installedVersion: null, stale: false }]); + }); + + it('flags an agent as stale when the marker trails the bundled version', async () => { const skillsDir = join(testHome, '.claude/skills'); mkdirSync(skillsDir, { recursive: true }); writeFileSync(join(skillsDir, SKILL_VERSION_MARKER_FILENAME), '0.2.4'); - const result = checkSkills(testHome); + const result = await checkSkills(testHome); expect(result!.agents).toEqual([{ agent: 'Claude Code', installedVersion: '0.2.4', stale: true }]); }); - it('does not flag stale when marker matches bundled', () => { + it('does not flag stale when marker matches bundled', async () => { const skillsDir = join(testHome, '.claude/skills'); mkdirSync(skillsDir, { recursive: true }); writeFileSync(join(skillsDir, SKILL_VERSION_MARKER_FILENAME), '0.3.0'); - const result = checkSkills(testHome); + const result = await checkSkills(testHome); expect(result!.agents[0].stale).toBe(false); }); - it('never flags stale when bundledVersion is null (unknown)', () => { - vi.mocked(getBundledSkillsVersion).mockReturnValue(null); + it('never flags stale when bundledVersion is null (unknown)', async () => { + vi.mocked(getBundledSkillsVersion).mockResolvedValue(null); const skillsDir = join(testHome, '.claude/skills'); mkdirSync(skillsDir, { recursive: true }); writeFileSync(join(skillsDir, SKILL_VERSION_MARKER_FILENAME), '0.2.4'); - const result = checkSkills(testHome); + const result = await checkSkills(testHome); expect(result!.bundledVersion).toBeNull(); expect(result!.agents[0].stale).toBe(false); }); - it('reports each detected agent separately', () => { + it('does not flag stale when installed > bundled (downgrade scenario)', async () => { + // String inequality would incorrectly fire here. Semver ordering must not. + const skillsDir = join(testHome, '.claude/skills'); + mkdirSync(skillsDir, { recursive: true }); + writeFileSync(join(skillsDir, SKILL_VERSION_MARKER_FILENAME), '0.5.0'); + + const result = await checkSkills(testHome); + + expect(result!.agents[0].installedVersion).toBe('0.5.0'); + expect(result!.agents[0].stale).toBe(false); + }); + + it('reports each detected agent separately', async () => { mkdirSync(join(testHome, '.claude/skills'), { recursive: true }); writeFileSync(join(testHome, '.claude/skills', SKILL_VERSION_MARKER_FILENAME), '0.2.4'); mkdirSync(join(testHome, '.codex/skills'), { recursive: true }); writeFileSync(join(testHome, '.codex/skills', SKILL_VERSION_MARKER_FILENAME), '0.3.0'); - const result = checkSkills(testHome); + const result = await checkSkills(testHome); expect(result!.agents).toHaveLength(2); const claude = result!.agents.find((a) => a.agent === 'Claude Code')!; diff --git a/src/doctor/checks/skills.ts b/src/doctor/checks/skills.ts index 82cdf007..eb539889 100644 --- a/src/doctor/checks/skills.ts +++ b/src/doctor/checks/skills.ts @@ -1,10 +1,19 @@ import { homedir } from 'node:os'; -import { readFileSync, existsSync } from 'node:fs'; +import { access, readFile } from 'node:fs/promises'; import { join } from 'node:path'; import semver from 'semver'; import { createAgents, getBundledSkillsVersion, SKILL_VERSION_MARKER_FILENAME } from '../../commands/install-skill.js'; import type { SkillsInfo, SkillAgentStatus } from '../types.js'; +async function pathExists(p: string): Promise { + try { + await access(p); + return true; + } catch { + return false; + } +} + /** * Stale = installed version is strictly older than the bundled version. * String inequality would also fire when installed > bundled (user installed @@ -31,8 +40,8 @@ function isStale(installed: string, bundled: string): boolean { * when no agents have a WorkOS skill installed at all — no noise for users who * never installed through the CLI. */ -export function checkSkills(home: string = homedir()): SkillsInfo | null { - const bundledVersion = getBundledSkillsVersion(); +export async function checkSkills(home: string = homedir()): Promise { + const bundledVersion = await getBundledSkillsVersion(); const agents = createAgents(home); const statuses: SkillAgentStatus[] = []; @@ -42,15 +51,23 @@ export function checkSkills(home: string = homedir()): SkillsInfo | null { // An agent's `skills/` dir existing (e.g. for unrelated user-installed // skills) doesn't mean WE installed — and `doctor --fix` would otherwise // happily write `workos/` and `workos-widgets/` onto an agent that never - // opted in. The marker OR an existing `workos/` skill subdir is the signal. + // opted in. The marker OR a workos/ / workos-widgets/ subdir is the signal + // (either is enough — older explicit installs of just `workos-widgets` + // shouldn't be invisible to doctor). const markerPath = join(agent.globalSkillsDir, SKILL_VERSION_MARKER_FILENAME); const workosSkillDir = join(agent.globalSkillsDir, 'workos'); - if (!existsSync(markerPath) && !existsSync(workosSkillDir)) continue; + const widgetsSkillDir = join(agent.globalSkillsDir, 'workos-widgets'); + const [hasMarker, hasWorkos, hasWidgets] = await Promise.all([ + pathExists(markerPath), + pathExists(workosSkillDir), + pathExists(widgetsSkillDir), + ]); + if (!hasMarker && !hasWorkos && !hasWidgets) continue; let installedVersion: string | null = null; - if (existsSync(markerPath)) { + if (hasMarker) { try { - installedVersion = readFileSync(markerPath, 'utf8').trim() || null; + installedVersion = (await readFile(markerPath, 'utf8')).trim() || null; } catch { installedVersion = null; } diff --git a/src/doctor/index.ts b/src/doctor/index.ts index 6ea8f320..7c3cacb5 100644 --- a/src/doctor/index.ts +++ b/src/doctor/index.ts @@ -60,7 +60,7 @@ export async function maybeRefreshSkills( after: refresh.perAgentAfter, skillsInstalled: refresh.skills, }, - skills: checkSkills() ?? undefined, + skills: (await checkSkills()) ?? undefined, }; } @@ -78,7 +78,7 @@ export async function runDoctor(options: DoctorOptions): Promise { checkLanguage(options.installDir), ]); - let skills = checkSkills() ?? undefined; + let skills = (await checkSkills()) ?? undefined; // `--fix`: refresh stale WorkOS skills BEFORE earlyIssues + AI analysis so // every downstream consumer (issue detection, AI prompt context) sees the From f4a9fcd22d489e08c94b31201bbc8a03de400cf8 Mon Sep 17 00:00:00 2001 From: Nick Nisi Date: Sun, 26 Apr 2026 16:57:30 -0500 Subject: [PATCH 09/10] chore: oxfmt format --- src/commands/install-skill.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/commands/install-skill.ts b/src/commands/install-skill.ts index 3996deae..6cecc35b 100644 --- a/src/commands/install-skill.ts +++ b/src/commands/install-skill.ts @@ -29,9 +29,7 @@ async function pathExists(p: string): Promise { * Returns null if the version can't be determined — callers treat that as * "no marker written" rather than failing the install. */ -export async function getBundledSkillsVersion( - skillsDir: string = getSkillsPackageDir(), -): Promise { +export async function getBundledSkillsVersion(skillsDir: string = getSkillsPackageDir()): Promise { try { // skillsDir = /plugins/workos/skills const packageRoot = dirname(dirname(dirname(skillsDir))); From cc6ffe793ab7b6250a9a989695917a9252127d05 Mon Sep 17 00:00:00 2001 From: Nick Nisi Date: Sun, 26 Apr 2026 17:35:41 -0500 Subject: [PATCH 10/10] docs(readme): mention auth-login skill hook and doctor --fix --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index ebc5b9f4..c762d013 100644 --- a/README.md +++ b/README.md @@ -59,7 +59,7 @@ Commands: doctor Diagnose WorkOS integration issues skills Manage WorkOS skills for coding agents (install, uninstall, list) -Skills are automatically installed to detected coding agents when you run `workos install`. Use `workos skills list` to check status. +Skills auto-install to detected coding agents on `workos install` and `workos auth login`. Use `workos skills list` to check status, `workos doctor` to detect stale skills, or `workos doctor --fix` to refresh them in place (constrained to `workos/` and `workos-widgets/`). Resource Management: organization (org) Manage organizations