diff --git a/packages/workspace-server/src/services/posthog-plugin/posthog-plugin.test.ts b/packages/workspace-server/src/services/posthog-plugin/posthog-plugin.test.ts index ea718b59d0..577a360328 100644 --- a/packages/workspace-server/src/services/posthog-plugin/posthog-plugin.test.ts +++ b/packages/workspace-server/src/services/posthog-plugin/posthog-plugin.test.ts @@ -433,8 +433,9 @@ describe("PosthogPluginService", () => { await expect(service.updateSkills()).resolves.toBeUndefined(); }); - it("handles missing skills dir in archive", async () => { - // Extraction creates no skills directory + it("reports a clear, actionable error when nothing downloads and no cache exists", async () => { + // Extraction creates no skills directory and there is no pre-existing + // skills cache to fall back on — the genuine failure case. mockExtractZip.mockImplementation( async (_zipPath: string, extractDir: string) => { vol.mkdirSync(`${extractDir}/random-dir`, { recursive: true }); @@ -447,6 +448,42 @@ describe("PosthogPluginService", () => { await service.updateSkills(); expect(handler).not.toHaveBeenCalled(); + // The reported error must be the clear, user-useful message, not the + // opaque "No skills found from any source". + expect(mockAnalytics.captureException).toHaveBeenCalledTimes(1); + const reportedError = mockAnalytics.captureException.mock + .calls[0][0] as Error; + expect(reportedError.message).not.toContain("No skills found"); + expect(reportedError.message).toContain("Couldn't download skills"); + expect(reportedError.message).toContain("retry automatically"); + }); + + it("keeps existing skills and stays silent when a download cycle is empty", async () => { + // Simulate a previously-downloaded skills cache from an earlier run. + vol.mkdirSync(`${RUNTIME_SKILLS_DIR}/cached-skill`, { recursive: true }); + vol.writeFileSync( + `${RUNTIME_SKILLS_DIR}/cached-skill/SKILL.md`, + "# Cached", + ); + + // Both downloads fail this cycle (e.g. transient network failure). + mockFetch.mockRejectedValue(new Error("Network error")); + + const handler = vi.fn(); + service.on("skillsUpdated", handler); + await service.updateSkills(); + + // Existing cache is preserved, no false-alarm exception, no update event, + // and the staging dir is cleaned up. + expect( + vol.readFileSync( + `${RUNTIME_SKILLS_DIR}/cached-skill/SKILL.md`, + "utf-8", + ), + ).toBe("# Cached"); + expect(mockAnalytics.captureException).not.toHaveBeenCalled(); + expect(handler).not.toHaveBeenCalled(); + expect(vol.existsSync(`${RUNTIME_SKILLS_DIR}.new`)).toBe(false); }); it("cleans up temp dir even on error", async () => { diff --git a/packages/workspace-server/src/services/posthog-plugin/posthog-plugin.ts b/packages/workspace-server/src/services/posthog-plugin/posthog-plugin.ts index 9aa3291cbc..31efc751e5 100644 --- a/packages/workspace-server/src/services/posthog-plugin/posthog-plugin.ts +++ b/packages/workspace-server/src/services/posthog-plugin/posthog-plugin.ts @@ -183,7 +183,11 @@ export class PosthogPluginService extends TypedEventEmitter }); if (result.success) { - this.emit("skillsUpdated", true); + // Only signal listeners when the cache actually changed; an empty + // download cycle succeeds as a no-op (result.data.updated === false). + if (result.data.updated) { + this.emit("skillsUpdated", true); + } } else { this.log.warn("Skills update failed", { error: result.error, diff --git a/packages/workspace-server/src/services/posthog-plugin/update-skills-saga.ts b/packages/workspace-server/src/services/posthog-plugin/update-skills-saga.ts index c3daed4556..9177aa4487 100644 --- a/packages/workspace-server/src/services/posthog-plugin/update-skills-saga.ts +++ b/packages/workspace-server/src/services/posthog-plugin/update-skills-saga.ts @@ -108,13 +108,54 @@ export class UpdateSkillsSaga extends Saga< } }); - // Step 3: validate skills (fatal if empty → triggers rollback of step 1) - await this.readOnlyStep("validate-skills", async () => { - const entries = await readdir(newSkillsDir); - if (entries.length === 0) { - throw new Error("No skills found from any source"); + // Step 3: validate skills. An empty staging dir means both downloads + // produced nothing this cycle (e.g. a transient network failure — the + // download steps above are intentionally non-fatal). The existing skills + // cache and the bundled skills remain in place, so this is a no-op cycle, + // not a failure: skip the swap and try again on the next interval rather + // than throwing, which would surface a misleading "no skills" exception. + const stagedSkillCount = await this.readOnlyStep( + "validate-skills", + async () => { + const entries = await readdir(newSkillsDir); + return entries.length; + }, + ); + + if (stagedSkillCount === 0) { + // Both downloads produced nothing this cycle. Clean up the empty + // staging dir, then decide whether this is a recoverable no-op or a + // genuine failure based on whether we have skills to fall back on. + await rm(newSkillsDir, { recursive: true, force: true }); + + const hasCachedSkills = + existsSync(input.runtimeSkillsDir) && + (await readdir(input.runtimeSkillsDir)).length > 0; + + if (hasCachedSkills) { + // A transient blip (e.g. network failure — the download steps above + // are non-fatal) shouldn't surface as an error: the previously + // downloaded skills are still in place. Skip the swap and retry next + // interval. + this.log.warn( + "No skills downloaded this cycle; keeping existing skills cache", + ); + return { updated: false }; } - }); + + // Nothing downloaded and no cache to fall back on. Surface a clear, + // actionable message rather than the opaque "No skills found from any + // source" — it tells the user what happened, that they're not stuck + // (bundled skills still work), and that it will recover on its own. + throw new Error( + "Couldn't download skills from PostHog (the skills and context-mill " + + "sources both returned nothing, likely a temporary network or " + + "server issue) and no previously downloaded skills were cached. " + + "The skills bundled with this version of PostHog Code are still " + + "available, and PostHog Code will retry automatically on the next " + + "update.", + ); + } // Step 4: atomic swap const oldSkillsDir = `${input.runtimeSkillsDir}.old`;