Fix skill loader GitHub fallback branch and add tests

The factory-plugins repo uses 'master' as its default branch, not 'main'.
The GitHub fallback was always 404ing in CI. Added unit tests for the
loader functions and an E2E test that verifies the skill downloads
from GitHub with no local cache (simulating a CI runner).

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>

Author: factory-nizar

src/utils/load-skill.ts

@@ -3,7 +3,7 @@ import { homedir } from "os";
 import { resolve, join } from "path";
 
 const PLUGINS_REPO = "Factory-AI/factory-plugins";
-const PLUGINS_BRANCH = "main";
+const PLUGINS_BRANCH = "master";
 
 const SHARED_BEGIN = "<!-- BEGIN_SHARED_METHODOLOGY -->";
 const SHARED_END = "<!-- END_SHARED_METHODOLOGY -->";

test/integration/load-skill-e2e.test.ts

@@ -0,0 +1,88 @@
+import {
+  afterEach,
+  beforeEach,
+  describe,
+  expect,
+  it,
+} from "bun:test";
+import { mkdtemp, rm } from "fs/promises";
+import { join } from "path";
+import os from "os";
+import {
+  loadSkill,
+  formatSkillSection,
+  extractSharedMethodology,
+} from "../../src/utils/load-skill";
+
+/**
+ * E2E test that simulates what the GitHub Action does:
+ * - No local plugin cache (fresh CI runner)
+ * - loadSkill("review") must succeed via GitHub fallback
+ * - formatSkillSection must produce usable prompt content
+ *
+ * This mirrors the code path in:
+ *   src/entrypoints/generate-review-prompt.ts
+ *   src/tag/commands/review.ts
+ *   src/tag/commands/review-validator.ts
+ */
+describe("skill loading E2E (GitHub Action simulation)", () => {
+  let fakeHome: string;
+  const originalHome = process.env.HOME;
+
+  beforeEach(async () => {
+    // Use a temp dir as HOME so there's no local plugin cache,
+    // forcing the GitHub fallback path (same as a CI runner)
+    fakeHome = await mkdtemp(join(os.tmpdir(), "skill-e2e-"));
+    process.env.HOME = fakeHome;
+  });
+
+  afterEach(async () => {
+    if (originalHome !== undefined) {
+      process.env.HOME = originalHome;
+    } else {
+      delete process.env.HOME;
+    }
+    await rm(fakeHome, { recursive: true, force: true });
+  });
+
+  it("loads the review skill from GitHub when no local cache exists", async () => {
+    const content = await loadSkill("review");
+
+    expect(content).toBeDefined();
+    expect(content.length).toBeGreaterThan(500);
+  });
+
+  it("extracts shared methodology with expected markers", async () => {
+    const content = await loadSkill("review");
+    const methodology = extractSharedMethodology(content);
+
+    // These sections must be present for the review prompts to work
+    expect(methodology).toContain("Reporting Gate");
+    expect(methodology).toContain("Bug Patterns");
+    expect(methodology).toContain("Confidence calibration");
+    expect(methodology).toContain("P0");
+    expect(methodology).toContain("P1");
+    expect(methodology).toContain("P2");
+  });
+
+  it("formatSkillSection produces non-empty prompt section", async () => {
+    const content = await loadSkill("review");
+    const section = formatSkillSection(content);
+
+    expect(section).toContain("<code_review_methodology>");
+    expect(section).toContain("</code_review_methodology>");
+    // The methodology must be substantial enough to replace the
+    // previously inlined guidance that was removed from the templates
+    expect(section.length).toBeGreaterThan(200);
+  });
+
+  it("methodology does NOT include the full skill (only shared section)", async () => {
+    const content = await loadSkill("review");
+    const methodology = extractSharedMethodology(content);
+
+    // The shared methodology should not include the YAML frontmatter
+    // or the Two-Pass Review Pipeline section (those are skill-only)
+    expect(methodology).not.toContain("name: review");
+    expect(methodology).not.toContain("version:");
+  });
+});

test/utils/load-skill.test.ts

@@ -0,0 +1,175 @@
+import {
+  afterEach,
+  beforeEach,
+  describe,
+  expect,
+  it,
+  spyOn,
+} from "bun:test";
+import * as fsPromises from "fs/promises";
+import {
+  extractSharedMethodology,
+  formatSkillSection,
+  loadSkill,
+} from "../../src/utils/load-skill";
+
+describe("extractSharedMethodology", () => {
+  it("extracts content between markers", () => {
+    const content = `
+# Skill Header
+
+Some intro text.
+
+<!-- BEGIN_SHARED_METHODOLOGY -->
+## Bug Patterns
+- Pattern 1
+- Pattern 2
+
+## Reporting Gate
+Only report real bugs.
+<!-- END_SHARED_METHODOLOGY -->
+
+## Other Section
+This should not be included.
+`;
+    const result = extractSharedMethodology(content);
+    expect(result).toContain("## Bug Patterns");
+    expect(result).toContain("## Reporting Gate");
+    expect(result).not.toContain("# Skill Header");
+    expect(result).not.toContain("## Other Section");
+  });
+
+  it("returns full content when markers are missing", () => {
+    const content = "No markers here, just plain content.";
+    const result = extractSharedMethodology(content);
+    expect(result).toBe(content);
+  });
+
+  it("returns full content when only BEGIN marker exists", () => {
+    const content = "<!-- BEGIN_SHARED_METHODOLOGY -->\nSome content";
+    const result = extractSharedMethodology(content);
+    expect(result).toBe(content);
+  });
+
+  it("returns full content when END comes before BEGIN", () => {
+    const content =
+      "<!-- END_SHARED_METHODOLOGY -->\nMiddle\n<!-- BEGIN_SHARED_METHODOLOGY -->";
+    const result = extractSharedMethodology(content);
+    expect(result).toBe(content);
+  });
+});
+
+describe("formatSkillSection", () => {
+  it("returns empty string for undefined input", () => {
+    expect(formatSkillSection(undefined)).toBe("");
+  });
+
+  it("returns empty string for empty string input", () => {
+    expect(formatSkillSection("")).toBe("");
+  });
+
+  it("wraps extracted methodology in XML tags", () => {
+    const content = `
+<!-- BEGIN_SHARED_METHODOLOGY -->
+## Reporting Gate
+Only report real bugs.
+<!-- END_SHARED_METHODOLOGY -->
+`;
+    const result = formatSkillSection(content);
+    expect(result).toContain("<code_review_methodology>");
+    expect(result).toContain("</code_review_methodology>");
+    expect(result).toContain("## Reporting Gate");
+  });
+});
+
+describe("loadSkill", () => {
+  let readdirSpy: ReturnType<typeof spyOn>;
+  let readFileSpy: ReturnType<typeof spyOn>;
+  let fetchSpy: ReturnType<typeof spyOn>;
+  const originalHome = process.env.HOME;
+
+  beforeEach(() => {
+    process.env.HOME = "/mock-home";
+    readdirSpy = spyOn(fsPromises, "readdir");
+    readFileSpy = spyOn(fsPromises, "readFile");
+    fetchSpy = spyOn(globalThis, "fetch");
+  });
+
+  afterEach(() => {
+    readdirSpy.mockRestore();
+    readFileSpy.mockRestore();
+    fetchSpy.mockRestore();
+    if (originalHome !== undefined) {
+      process.env.HOME = originalHome;
+    } else {
+      delete process.env.HOME;
+    }
+  });
+
+  it("loads skill from local cache when available", async () => {
+    readdirSpy.mockResolvedValue(["abc123"] as any);
+    readFileSpy.mockResolvedValue("cached skill content");
+
+    const result = await loadSkill("review");
+
+    expect(result).toBe("cached skill content");
+    expect(fetchSpy).not.toHaveBeenCalled();
+  });
+
+  it("falls back to GitHub when local cache is empty", async () => {
+    readdirSpy.mockRejectedValue(new Error("ENOENT"));
+    fetchSpy.mockResolvedValue({
+      ok: true,
+      text: () => Promise.resolve("github skill content"),
+    } as Response);
+
+    const result = await loadSkill("review");
+
+    expect(result).toBe("github skill content");
+    const fetchUrl = fetchSpy.mock.calls[0]![0] as string;
+    expect(fetchUrl).toContain("/master/");
+    expect(fetchUrl).not.toContain("/main/");
+  });
+
+  it("throws when both sources fail", async () => {
+    readdirSpy.mockRejectedValue(new Error("ENOENT"));
+    fetchSpy.mockResolvedValue({ ok: false, status: 404 } as Response);
+
+    await expect(loadSkill("review")).rejects.toThrow(
+      'Required skill "review" not found',
+    );
+  });
+
+  it("skips empty cache entries and tries next hash", async () => {
+    readdirSpy.mockResolvedValue(["hash1", "hash2"] as any);
+    readFileSpy
+      .mockRejectedValueOnce(new Error("ENOENT"))
+      .mockResolvedValueOnce("skill from hash2");
+
+    const result = await loadSkill("review");
+    expect(result).toBe("skill from hash2");
+  });
+});
+
+describe("loadSkill GitHub integration", () => {
+  let readdirSpy: ReturnType<typeof spyOn>;
+
+  beforeEach(() => {
+    readdirSpy = spyOn(fsPromises, "readdir").mockRejectedValue(
+      new Error("ENOENT"),
+    );
+  });
+
+  afterEach(() => {
+    readdirSpy.mockRestore();
+  });
+
+  it("fetches the review skill from the factory-plugins repo", async () => {
+    const content = await loadSkill("review");
+
+    expect(content.length).toBeGreaterThan(100);
+    expect(content).toContain("BEGIN_SHARED_METHODOLOGY");
+    expect(content).toContain("END_SHARED_METHODOLOGY");
+    expect(content).toContain("Reporting Gate");
+  });
+});