diff --git a/apps/server/src/personas/loader.ts b/apps/server/src/personas/loader.ts index ccbf89c8..f23f56bb 100644 --- a/apps/server/src/personas/loader.ts +++ b/apps/server/src/personas/loader.ts @@ -89,6 +89,33 @@ export async function loadPersonas( return personas; } +export function mergePersonasWithWorktreePrecedence< + T extends { slug: string }, +>(input: { worktreePersonas: T[]; repoPersonas: T[] }): T[] { + const worktreeSlugs = new Set(input.worktreePersonas.map((p) => p.slug)); + return [ + ...input.worktreePersonas, + ...input.repoPersonas.filter((p) => !worktreeSlugs.has(p.slug)), + ]; +} + +export async function loadPersonasFromRoots(input: { + worktreeRoot?: string | null; + repoRoot?: string | null; +}): Promise { + const worktreeRoot = input.worktreeRoot ?? null; + const repoRoot = input.repoRoot ?? null; + + const worktreePersonas = worktreeRoot ? await loadPersonas(worktreeRoot) : []; + const repoPersonas = + repoRoot && repoRoot !== worktreeRoot ? await loadPersonas(repoRoot) : []; + + return mergePersonasWithWorktreePrecedence({ + worktreePersonas, + repoPersonas, + }); +} + export async function loadPersonaBySlug( repoRoot: string, slug: string diff --git a/apps/server/src/routes/persona-reviews.ts b/apps/server/src/routes/persona-reviews.ts index ab307c82..12f169a5 100644 --- a/apps/server/src/routes/persona-reviews.ts +++ b/apps/server/src/routes/persona-reviews.ts @@ -5,7 +5,7 @@ import { CLI_AGENT_TYPES, getEnabledAgentTypes, } from "../agent-type-settings.js"; -import { loadPersonas } from "../personas/loader.js"; +import { loadPersonasFromRoots } from "../personas/loader.js"; import { resolveRepoRoot, resolveWorktreeRoot, @@ -39,6 +39,24 @@ type PersonaReviewRouteDeps = { const PERSONA_SLUG_PATTERN = /^[a-zA-Z0-9_-]+$/; +async function resolveOptionalWorktreeRoot( + cwd: string +): Promise { + try { + return await resolveWorktreeRoot(cwd); + } catch { + return null; + } +} + +async function resolveOptionalRepoRoot(cwd: string): Promise { + try { + return await resolveRepoRoot(cwd); + } catch { + return null; + } +} + export async function registerPersonaReviewRoutes( app: FastifyInstance, deps: PersonaReviewRouteDeps @@ -51,10 +69,9 @@ export async function registerPersonaReviewRoutes( .send({ error: "cwd query parameter is required." }); } try { - let personas = await loadPersonas(await resolveWorktreeRoot(query.cwd)); - if (personas.length === 0) { - personas = await loadPersonas(await resolveRepoRoot(query.cwd)); - } + const worktreeRoot = await resolveOptionalWorktreeRoot(query.cwd); + const repoRoot = await resolveOptionalRepoRoot(query.cwd); + const personas = await loadPersonasFromRoots({ worktreeRoot, repoRoot }); return { personas }; } catch { return { personas: [] }; diff --git a/apps/server/src/shared/mcp/persona-interaction-tools.ts b/apps/server/src/shared/mcp/persona-interaction-tools.ts index 01c83ca6..04728331 100644 --- a/apps/server/src/shared/mcp/persona-interaction-tools.ts +++ b/apps/server/src/shared/mcp/persona-interaction-tools.ts @@ -1,6 +1,7 @@ import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import * as z from "zod/v4"; +import { mergePersonasWithWorktreePrecedence } from "../../personas/loader.js"; import type { McpRequestContext } from "./server.js"; import { toToolError } from "./tool-error.js"; @@ -24,6 +25,27 @@ export type PersonaInteractionCallbacks = { submitResolution?: McpRequestContext["submitResolution"]; }; +type PersonaSummary = { slug: string; name: string; description: string }; + +export async function resolvePersonaList( + listPersonas: (root: string) => Promise, + worktreeRoot?: string | null, + repoRoot?: string | null +): Promise { + const worktreePersonas = worktreeRoot + ? await listPersonas(worktreeRoot).catch(() => []) + : []; + const repoPersonas = + repoRoot && repoRoot !== worktreeRoot + ? await listPersonas(repoRoot).catch(() => []) + : []; + + return mergePersonasWithWorktreePrecedence({ + worktreePersonas, + repoPersonas, + }); +} + export function registerPersonaInteractionTools( server: McpServer, allowed: Set, @@ -33,8 +55,8 @@ export function registerPersonaInteractionTools( // ── list_personas ──────────────────────────────────────────────── if (allowed.has("list_personas") && callbacks.listPersonas) { - const personaRoot = callbacks.worktreeRoot ?? callbacks.repoRoot; const listPersonas = callbacks.listPersonas; + const { worktreeRoot, repoRoot } = callbacks; server.registerTool( "list_personas", @@ -44,16 +66,12 @@ export function registerPersonaInteractionTools( inputSchema: {}, }, async () => { - if (!personaRoot) { - return { - content: [ - { type: "text", text: JSON.stringify({ personas: [] }, null, 2) }, - ], - structuredContent: { personas: [] }, - }; - } try { - const personas = await listPersonas(personaRoot); + const personas = await resolvePersonaList( + listPersonas, + worktreeRoot, + repoRoot + ); return { content: [ { type: "text", text: JSON.stringify({ personas }, null, 2) }, diff --git a/apps/server/test/persona-list-merge.test.ts b/apps/server/test/persona-list-merge.test.ts new file mode 100644 index 00000000..d5a51931 --- /dev/null +++ b/apps/server/test/persona-list-merge.test.ts @@ -0,0 +1,92 @@ +import { describe, expect, it, vi } from "vitest"; + +import { resolvePersonaList } from "../src/shared/mcp/persona-interaction-tools.js"; + +const persona = (slug: string, name = slug) => ({ + slug, + name, + description: "", +}); + +describe("resolvePersonaList", () => { + it("includes personas from both the worktree and repo root", async () => { + const listPersonas = vi.fn(async (root: string) => + root === "/wt" ? [persona("local")] : [persona("repo")] + ); + + const result = await resolvePersonaList(listPersonas, "/wt", "/repo"); + + expect(result.map((p) => p.slug)).toEqual(["local", "repo"]); + expect(listPersonas).toHaveBeenCalledWith("/wt"); + expect(listPersonas).toHaveBeenCalledWith("/repo"); + }); + + it("uses worktree personas as the override for duplicate slugs", async () => { + const listPersonas = vi.fn(async (root: string) => + root === "/wt" + ? [persona("review", "Worktree Review")] + : [persona("review", "Repo Review"), persona("release")] + ); + + const result = await resolvePersonaList(listPersonas, "/wt", "/repo"); + + expect(result).toEqual([ + persona("review", "Worktree Review"), + persona("release"), + ]); + }); + + it("uses the repo root directly when there is no worktree root", async () => { + const listPersonas = vi.fn(async (root: string) => + root === "/repo" ? [persona("repo")] : [] + ); + + const result = await resolvePersonaList(listPersonas, null, "/repo"); + + expect(result).toEqual([persona("repo")]); + expect(listPersonas).toHaveBeenCalledTimes(1); + expect(listPersonas).toHaveBeenCalledWith("/repo"); + }); + + it("does not re-query when the worktree root and repo root are identical", async () => { + const listPersonas = vi.fn(async () => [persona("repo")]); + + const result = await resolvePersonaList(listPersonas, "/repo", "/repo"); + + expect(result).toEqual([persona("repo")]); + expect(listPersonas).toHaveBeenCalledTimes(1); + }); + + it("keeps worktree personas when the repo root listing fails", async () => { + const listPersonas = vi.fn(async (root: string) => { + if (root === "/repo") throw new Error("repo unavailable"); + return [persona("local")]; + }); + + const result = await resolvePersonaList(listPersonas, "/wt", "/repo"); + + expect(result).toEqual([persona("local")]); + expect(listPersonas).toHaveBeenCalledTimes(2); + }); + + it("keeps repo personas when the worktree root listing fails", async () => { + const listPersonas = vi.fn(async (root: string) => { + if (root === "/wt") throw new Error("worktree unavailable"); + return [persona("repo")]; + }); + + const result = await resolvePersonaList(listPersonas, "/wt", "/repo"); + + expect(result).toEqual([persona("repo")]); + expect(listPersonas).toHaveBeenCalledTimes(2); + }); + + it("returns an empty list when neither root is available", async () => { + const listPersonas = vi.fn(async () => []); + + const result = await resolvePersonaList(listPersonas, null, null); + + expect(result).toEqual([]); + expect(listPersonas).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/server/test/persona-loader.test.ts b/apps/server/test/persona-loader.test.ts index f28b2dcc..81c4b21b 100644 --- a/apps/server/test/persona-loader.test.ts +++ b/apps/server/test/persona-loader.test.ts @@ -8,6 +8,8 @@ import { INLINE_DIFF_THRESHOLD_BYTES, loadPersonaBySlug, loadPersonas, + loadPersonasFromRoots, + mergePersonasWithWorktreePrecedence, parseFrontmatter, } from "../src/personas/loader.js"; import type { PersonaDefinition } from "../src/personas/loader.js"; @@ -444,6 +446,98 @@ feedbackFormat: checklist }); }); +describe("mergePersonasWithWorktreePrecedence", () => { + const persona = (slug: string, name = slug): PersonaDefinition => ({ + slug, + name, + description: "", + feedbackFormat: "findings", + body: "", + }); + + it("includes repo personas that are absent from the worktree", () => { + const merged = mergePersonasWithWorktreePrecedence({ + worktreePersonas: [persona("worktree-only")], + repoPersonas: [persona("repo-only")], + }); + + expect(merged.map((p) => p.slug)).toEqual(["worktree-only", "repo-only"]); + }); + + it("uses the worktree persona when both roots define the same slug", () => { + const merged = mergePersonasWithWorktreePrecedence({ + worktreePersonas: [persona("review", "Worktree Review")], + repoPersonas: [persona("review", "Repo Review"), persona("release")], + }); + + expect(merged).toEqual([ + persona("review", "Worktree Review"), + persona("release"), + ]); + }); +}); + +describe("loadPersonasFromRoots", () => { + const tmpBase = `/tmp/dispatch-persona-roots-test-${process.pid}`; + const worktreeRoot = path.join(tmpBase, "worktree"); + const repoRoot = path.join(tmpBase, "repo"); + + beforeAll(() => { + mkdirSync(path.join(worktreeRoot, ".dispatch", "personas"), { + recursive: true, + }); + mkdirSync(path.join(repoRoot, ".dispatch", "personas"), { + recursive: true, + }); + writeFileSync( + path.join(worktreeRoot, ".dispatch", "personas", "security.md"), + `--- +name: Worktree Security +--- + +# Worktree security` + ); + writeFileSync( + path.join(repoRoot, ".dispatch", "personas", "security.md"), + `--- +name: Repo Security +--- + +# Repo security` + ); + writeFileSync( + path.join(repoRoot, ".dispatch", "personas", "release.md"), + `--- +name: Release +--- + +# Release` + ); + }); + + afterAll(() => { + rmSync(tmpBase, { recursive: true, force: true }); + }); + + it("loads both roots and lets the worktree override duplicate slugs", async () => { + const personas = await loadPersonasFromRoots({ worktreeRoot, repoRoot }); + + expect(personas.map((p) => [p.slug, p.name])).toEqual([ + ["security", "Worktree Security"], + ["release", "Release"], + ]); + }); + + it("does not read the repo twice when both roots are the same", async () => { + const personas = await loadPersonasFromRoots({ + worktreeRoot: repoRoot, + repoRoot, + }); + + expect(personas.map((p) => p.slug)).toEqual(["release", "security"]); + }); +}); + describe("loadPersonaBySlug", () => { const tmpRoot = `/tmp/dispatch-persona-slug-test-${process.pid}`; const personasDir = path.join(tmpRoot, ".dispatch", "personas"); diff --git a/apps/server/test/persona-reviews-routes.test.ts b/apps/server/test/persona-reviews-routes.test.ts index 60fdf33a..f236cc56 100644 --- a/apps/server/test/persona-reviews-routes.test.ts +++ b/apps/server/test/persona-reviews-routes.test.ts @@ -13,7 +13,7 @@ import { registerPersonaReviewRoutes } from "../src/routes/persona-reviews.js"; import { CLI_AGENT_TYPES } from "../src/agent-type-settings.js"; vi.mock("../src/personas/loader.js", () => ({ - loadPersonas: vi.fn(async () => []), + loadPersonasFromRoots: vi.fn(async () => []), })); vi.mock("../src/shared/git/git-context.js", () => ({ @@ -119,8 +119,8 @@ describe("GET /api/v1/personas", () => { }); it("returns personas array for a valid cwd", async () => { - const { loadPersonas } = await import("../src/personas/loader.js"); - vi.mocked(loadPersonas).mockResolvedValueOnce([ + const { loadPersonasFromRoots } = await import("../src/personas/loader.js"); + vi.mocked(loadPersonasFromRoots).mockResolvedValueOnce([ { slug: "security-review", name: "Security Review" }, ] as never); const res = await app.inject({ @@ -132,13 +132,11 @@ describe("GET /api/v1/personas", () => { expect(res.json().personas[0].slug).toBe("security-review"); }); - it("falls back to repo root when worktree returns empty", async () => { - const { loadPersonas } = await import("../src/personas/loader.js"); - vi.mocked(loadPersonas) - .mockResolvedValueOnce([]) - .mockResolvedValueOnce([ - { slug: "from-repo", name: "From Repo" }, - ] as never); + it("loads personas from the resolved worktree and repo roots", async () => { + const { loadPersonasFromRoots } = await import("../src/personas/loader.js"); + vi.mocked(loadPersonasFromRoots).mockResolvedValueOnce([ + { slug: "from-repo", name: "From Repo" }, + ] as never); const res = await app.inject({ method: "GET", url: "/api/v1/personas?cwd=/tmp", @@ -146,12 +144,17 @@ describe("GET /api/v1/personas", () => { expect(res.statusCode).toBe(200); expect(res.json().personas).toHaveLength(1); expect(res.json().personas[0].slug).toBe("from-repo"); - expect(loadPersonas).toHaveBeenCalledTimes(2); + expect(loadPersonasFromRoots).toHaveBeenCalledWith({ + worktreeRoot: "/tmp", + repoRoot: "/tmp", + }); }); it("returns empty array on error", async () => { - const { loadPersonas } = await import("../src/personas/loader.js"); - vi.mocked(loadPersonas).mockRejectedValueOnce(new Error("not a git repo")); + const { loadPersonasFromRoots } = await import("../src/personas/loader.js"); + vi.mocked(loadPersonasFromRoots).mockRejectedValueOnce( + new Error("not a git repo") + ); const res = await app.inject({ method: "GET", url: "/api/v1/personas?cwd=/nonexistent",