PAP-10287 address teams catalog review feedback

Co-Authored-By: Paperclip <noreply@paperclip.ing>

cli/src/__tests__/teams.test.ts

@@ -502,6 +502,36 @@ describe("teams CLI commands", () => {
     expect(errorSpy).not.toHaveBeenCalled();
   });
 
+  it("does not request board approval for unrelated forbidden install errors", async () => {
+    fetchMock.mockResolvedValueOnce(jsonResponse({ error: "Agent key cannot access another company" }, 403));
+    vi.spyOn(process, "exit").mockImplementation(((code?: string | number | null) => {
+      throw new Error(`exit:${code ?? 0}`);
+    }) as typeof process.exit);
+
+    await expect(runCommand([
+      "teams",
+      "install",
+      "product-engineering",
+      "--company-id",
+      "company-1",
+      "--request-approval-on-forbidden",
+      "--api-base",
+      "http://paperclip.test",
+      "--api-key",
+      "token",
+    ])).rejects.toThrow("exit:1");
+
+    expect(fetchMock).toHaveBeenCalledTimes(1);
+    expect(fetchMock).toHaveBeenCalledWith(
+      "http://paperclip.test/api/companies/company-1/teams/catalog/ref/install?ref=product-engineering",
+      expect.objectContaining({
+        method: "POST",
+        body: JSON.stringify({}),
+      }),
+    );
+    expect(String(errorSpy.mock.calls[0]?.[0])).toContain("Agent key cannot access another company");
+  });
+
   it("surfaces server blocks for unsafe local-path catalog team sources", async () => {
     fetchMock.mockResolvedValueOnce(jsonResponse({
       error: 'Local path skill source "../unsafe" is development-only and is not allowed for catalog team install.',

cli/src/commands/client/company.ts

@@ -1750,11 +1750,11 @@ async function resolveCurrentCompanyId(ctx: { companyId?: string; api: { get<T>(
 }
 
 function isBoardAccessRequiredError(error: unknown): error is ApiRequestError {
-  return error instanceof ApiRequestError && error.status === 403 && error.message.includes("Board access required");
+  return error instanceof ApiRequestError && error.status === 403 && error.message.toLowerCase().includes("board access required");
 }
 
 function isInstanceAdminRequiredError(error: unknown): error is ApiRequestError {
-  return error instanceof ApiRequestError && error.status === 403 && error.message.includes("Instance admin");
+  return error instanceof ApiRequestError && error.status === 403 && error.message.toLowerCase().includes("instance admin");
 }
 
 function addCompanyJsonPost(parent: Command, name: string, description: string, pathSuffix: string): void {

cli/src/commands/client/teams.ts

@@ -361,12 +361,16 @@ function buildTeamInstallOptions(opts: TeamInstallOptions): CatalogTeamInstallOp
   return buildTeamOptions(opts);
 }
 
+const INSTALL_APPROVAL_FALLBACK_MESSAGES = [
+  "missing permission: agents:create",
+  "missing permission: can create agents",
+];
+
 function shouldRequestInstallApproval(error: unknown, opts: TeamInstallOptions): error is ApiRequestError {
-  return Boolean(
-    (opts.requestApprovalOnForbidden || isPaperclipTaskRun()) &&
-      error instanceof ApiRequestError &&
-      error.status === 403,
-  );
+  if (!(opts.requestApprovalOnForbidden || isPaperclipTaskRun())) return false;
+  if (!(error instanceof ApiRequestError) || error.status !== 403) return false;
+  const message = error.message.toLowerCase();
+  return INSTALL_APPROVAL_FALLBACK_MESSAGES.some((expected) => message.includes(expected));
 }
 
 function isPaperclipTaskRun(): boolean {

server/src/__tests__/company-portability.test.ts

@@ -64,6 +64,7 @@ const assetSvc = {
 
 const secretSvc = {
   create: vi.fn(async () => ({ id: "secret-created" })),
+  remove: vi.fn(async () => true),
   normalizeAdapterConfigForPersistence: vi.fn(async (_companyId: string, config: Record<string, unknown>) => config),
   normalizeEnvBindingsForPersistence: vi.fn(async (_companyId: string, env: Record<string, unknown>) => env),
   syncEnvBindingsForTarget: vi.fn(async () => []),
@@ -133,6 +134,7 @@ describe("company portability", () => {
   beforeEach(() => {
     vi.clearAllMocks();
     secretSvc.create.mockResolvedValue({ id: "secret-created" });
+    secretSvc.remove.mockResolvedValue(true);
     secretSvc.normalizeAdapterConfigForPersistence.mockImplementation(async (_companyId, config) => config);
     secretSvc.normalizeEnvBindingsForPersistence.mockImplementation(async (_companyId, env) => env);
     secretSvc.syncEnvBindingsForTarget.mockResolvedValue([]);
@@ -1501,6 +1503,69 @@ describe("company portability", () => {
     );
   });
 
+  it("removes import secrets created before a later import failure", async () => {
+    const portability = companyPortabilityService({} as any);
+    agentSvc.list.mockResolvedValue([]);
+    secretSvc.create.mockResolvedValueOnce({ id: "secret-created-for-failed-import" });
+    agentSvc.create.mockRejectedValueOnce(new Error("agent create failed"));
+
+    await expect(portability.importBundle({
+      source: {
+        type: "inline",
+        files: {
+          "COMPANY.md": [
+            "---",
+            "name: Import",
+            "includes:",
+            "  - agents/coder/AGENTS.md",
+            "---",
+            "",
+          ].join("\n"),
+          "agents/coder/AGENTS.md": [
+            "---",
+            "name: Coder",
+            "slug: coder",
+            "kind: agent",
+            "---",
+            "",
+            "# Coder",
+            "",
+          ].join("\n"),
+          ".paperclip.yaml": [
+            "schema: paperclip/v1",
+            "agents:",
+            "  coder:",
+            "    adapter:",
+            "      type: codex_local",
+            "      config: {}",
+            "    inputs:",
+            "      env:",
+            "        OPENAI_API_KEY:",
+            "          kind: secret",
+            "          requirement: required",
+            "",
+          ].join("\n"),
+        },
+      },
+      include: {
+        company: false,
+        agents: true,
+        projects: false,
+        issues: false,
+      },
+      target: {
+        mode: "existing_company",
+        companyId: "company-1",
+      },
+      collisionStrategy: "rename",
+      secretValues: {
+        "agent:coder:OPENAI_API_KEY": "sk-imported",
+      },
+    }, "user-1")).rejects.toThrow("agent create failed");
+
+    expect(secretSvc.remove).toHaveBeenCalledWith("secret-created-for-failed-import");
+  });
+
   it("reparents imported roots to pre-existing target managers before resolving imported hierarchy", async () => {
     const portability = companyPortabilityService({} as any);
     agentSvc.list.mockResolvedValue([

server/src/__tests__/teams-catalog-service.test.ts

@@ -186,6 +186,46 @@ describe("teamsCatalogService", () => {
     );
   });
 
+  it("forces catalog previews to exclude company metadata even when requested", async () => {
+    const svc = teamsCatalogService({} as any);
+
+    await svc.previewCatalogTeamImport("company-1", "content-machine", {
+      include: { company: true, agents: false },
+    });
+
+    expect(mockCompanyPortabilityService.previewImport).toHaveBeenCalledWith(
+      expect.objectContaining({
+        include: expect.objectContaining({
+          company: false,
+          agents: false,
+        }),
+      }),
+      { mode: "agent_safe", sourceCompanyId: "company-1" },
+    );
+  });
+
+  it("preflights imports before installing catalog skills", async () => {
+    mockCompanyPortabilityService.previewImport.mockResolvedValueOnce({
+      include: { company: false, agents: true, projects: true, issues: true, skills: true },
+      targetCompanyId: "company-1",
+      targetCompanyName: "Paperclip",
+      collisionStrategy: "rename",
+      selectedAgentSlugs: ["ceo"],
+      plan: { companyAction: "none", agentPlans: [], projectPlans: [], issuePlans: [] },
+      manifest: { agents: [], skills: [], projects: [], issues: [], envInputs: [], includes: { company: false, agents: true, projects: true, issues: true, skills: true }, company: null, schemaVersion: 1, generatedAt: new Date().toISOString(), source: null, sidebar: null },
+      files: {},
+      envInputs: [],
+      warnings: [],
+      errors: ["Safe import does not allow process adapter type."],
+    });
+    const svc = teamsCatalogService({} as any);
+
+    await expect(svc.installCatalogTeam("company-1", "core-exec-team")).rejects.toMatchObject({ status: 422 });
+
+    expect(mockCompanySkillService.installFromCatalog).not.toHaveBeenCalled();
+    expect(mockCompanyPortabilityService.importBundle).not.toHaveBeenCalled();
+  });
+
   it("injects safe claude_local adapter defaults for every bundled agent when no overrides are supplied", async () => {
     const svc = teamsCatalogService({} as any);
 

server/src/routes/teams-catalog.ts

@@ -69,7 +69,7 @@ export function teamsCatalogRoutes(db: Db) {
       category: firstQueryString(req.query.category),
       q: firstQueryString(req.query.q),
     });
-    res.json(listCatalogTeams(query));
+    res.json(await listCatalogTeams(query));
   });
 
   router.get("/teams/catalog/:catalogId/files", async (req, res) => {
@@ -82,7 +82,7 @@ export function teamsCatalogRoutes(db: Db) {
   router.get("/teams/catalog/:catalogId", async (req, res) => {
     assertAuthenticated(req);
     const catalogRef = firstQueryString(req.query.ref) ?? (req.params.catalogId as string);
-    res.json(getCatalogTeamOrThrow(catalogRef));
+    res.json(await getCatalogTeamOrThrow(catalogRef));
   });
 
   router.get("/companies/:companyId/teams/catalog/installed", async (req, res) => {

server/src/services/company-portability.ts

@@ -3052,6 +3052,7 @@ export function companyPortabilityService(db: Db, storage?: StorageService) {
     envInputs: CompanyPortabilityEnvInput[],
     secretValues: Record<string, string> | null | undefined,
     actorUserId: string | null | undefined,
+    createdSecretIds: string[] = [],
   ) {
     if (envInputs.length === 0) return;
     const missingRequired = envInputs.filter((input) => {
@@ -3088,6 +3089,7 @@ export function companyPortabilityService(db: Db, storage?: StorageService) {
         },
         { userId: actorUserId ?? null, agentId: null },
       );
+      createdSecretIds.push(secret.id);
       writeManifestEnvBinding(manifest, input, {
         type: "secret_ref",
         secretId: secret.id,
@@ -4385,13 +4387,16 @@ export function companyPortabilityService(db: Db, storage?: StorageService) {
       }
       return true;
     });
-    await materializeImportEnvInputValues(
-      targetCompany.id,
-      sourceManifest,
-      importEnvInputs,
-      input.secretValues,
-      actorUserId,
-    );
+    const createdImportSecretIds: string[] = [];
+    try {
+      await materializeImportEnvInputValues(
+        targetCompany.id,
+        sourceManifest,
+        importEnvInputs,
+        input.secretValues,
+        actorUserId,
+        createdImportSecretIds,
+      );
 
     if (include.company) {
       const logoPath = sourceManifest.company?.logoPath ?? null;
@@ -4950,17 +4955,23 @@ export function companyPortabilityService(db: Db, storage?: StorageService) {
       }
     }
 
-    return {
-      company: {
-        id: targetCompany.id,
-        name: targetCompany.name,
-        action: companyAction,
-      },
-      agents: resultAgents,
-      projects: resultProjects,
-      envInputs: sourceManifest.envInputs ?? [],
-      warnings,
-    };
+      return {
+        company: {
+          id: targetCompany.id,
+          name: targetCompany.name,
+          action: companyAction,
+        },
+        agents: resultAgents,
+        projects: resultProjects,
+        envInputs: sourceManifest.envInputs ?? [],
+        warnings,
+      };
+    } catch (error) {
+      for (const secretId of createdImportSecretIds) {
+        await secrets.remove(secretId).catch(() => undefined);
+      }
+      throw error;
+    }
   }
 
   return {

server/src/services/teams-catalog.ts

@@ -1,4 +1,3 @@
-import { existsSync, readFileSync, statSync } from "node:fs";
 import { promises as fs } from "node:fs";
 import path from "node:path";
 import { fileURLToPath } from "node:url";
@@ -131,50 +130,66 @@ export interface CatalogTeamFileDetail {
 }
 
 const serviceDir = path.dirname(fileURLToPath(import.meta.url));
-const repoRoot = path.resolve(serviceDir, "../../..");
-const catalogPackageRoot = path.join(repoRoot, "packages/teams-catalog");
-const catalogManifestPath = path.join(catalogPackageRoot, "generated/catalog.json");
+const catalogPackageRootCandidates = buildCatalogPackageRootCandidates();
+let catalogPackageRoot = catalogPackageRootCandidates[0]!;
+let catalogManifestPath = path.join(catalogPackageRoot, "generated/catalog.json");
 let cachedCatalogManifest: {
   manifest: CatalogManifestFile;
+  manifestPath: string;
   mtimeMs: number;
   size: number;
 } | null = null;
 
-function loadCatalogManifest(): CatalogManifestFile {
-  if (!existsSync(catalogManifestPath)) {
-    throw new Error(
-      `Teams catalog manifest not found at ${catalogManifestPath}. Run pnpm --filter @paperclipai/teams-catalog build:manifest.`,
-    );
-  }
-  return JSON.parse(readFileSync(catalogManifestPath, "utf8")) as CatalogManifestFile;
+function buildCatalogPackageRootCandidates() {
+  const configuredRoot = process.env.PAPERCLIP_TEAMS_CATALOG_DIR?.trim();
+  const candidates = [
+    ...(configuredRoot ? [path.resolve(configuredRoot)] : []),
+    path.resolve(process.cwd(), "packages/teams-catalog"),
+    path.resolve(serviceDir, "../../..", "packages/teams-catalog"),
+  ];
+  return Array.from(new Set(candidates));
 }
 
-function getCatalogManifest() {
-  if (!existsSync(catalogManifestPath)) {
-    throw new Error(
-      `Teams catalog manifest not found at ${catalogManifestPath}. Run pnpm --filter @paperclipai/teams-catalog build:manifest.`,
-    );
+async function statCatalogManifest() {
+  for (const candidateRoot of catalogPackageRootCandidates) {
+    const candidateManifestPath = path.join(candidateRoot, "generated/catalog.json");
+    try {
+      const stats = await fs.stat(candidateManifestPath);
+      catalogPackageRoot = candidateRoot;
+      catalogManifestPath = candidateManifestPath;
+      return { stats, manifestPath: candidateManifestPath };
+    } catch {
+      // Try the next known runtime layout before reporting all checked paths.
+    }
   }
-  const stats = statSync(catalogManifestPath);
+  throw new Error(
+    `Teams catalog manifest not found. Checked: ${catalogPackageRootCandidates.map((root) => path.join(root, "generated/catalog.json")).join(", ")}. Run pnpm --filter @paperclipai/teams-catalog build:manifest.`,
+  );
+}
+
+async function getCatalogManifest() {
+  const { stats, manifestPath } = await statCatalogManifest();
   if (
     cachedCatalogManifest
+    && cachedCatalogManifest.manifestPath === manifestPath
     && cachedCatalogManifest.mtimeMs === stats.mtimeMs
     && cachedCatalogManifest.size === stats.size
   ) {
     return cachedCatalogManifest.manifest;
   }
 
-  const manifest = loadCatalogManifest();
+  const manifest = JSON.parse(await fs.readFile(manifestPath, "utf8")) as CatalogManifestFile;
   cachedCatalogManifest = {
     manifest,
+    manifestPath,
     mtimeMs: stats.mtimeMs,
     size: stats.size,
   };
   return manifest;
 }
 
-function getCatalogTeams() {
-  const manifest = getCatalogManifest();
+async function getCatalogTeams() {
+  const manifest = await getCatalogManifest();
   return manifest.teams.map((team) => ({
     ...team,
     packageName: manifest.packageName,
@@ -196,9 +211,9 @@ function searchText(team: CatalogTeam) {
   ].join("\n").toLowerCase();
 }
 
-export function listCatalogTeams(query: CatalogTeamListQuery = {}): CatalogTeam[] {
+export async function listCatalogTeams(query: CatalogTeamListQuery = {}): Promise<CatalogTeam[]> {
   const normalizedQuery = query.q?.trim().toLowerCase() ?? "";
-  return getCatalogTeams()
+  return (await getCatalogTeams())
     .filter((team) => !query.kind || team.kind === query.kind)
     .filter((team) => !query.category || team.category === query.category)
     .filter((team) => !normalizedQuery || searchText(team).includes(normalizedQuery))
@@ -242,10 +257,10 @@ export function readCatalogTeamProvenance(
   };
 }
 
-export function resolveCatalogTeamReference(reference: string): { team: CatalogTeam | null; ambiguous: boolean } {
+export async function resolveCatalogTeamReference(reference: string): Promise<{ team: CatalogTeam | null; ambiguous: boolean }> {
   const trimmed = reference.trim();
   if (!trimmed) return { team: null, ambiguous: false };
-  const teams = getCatalogTeams();
+  const teams = await getCatalogTeams();
 
   const exact = teams.find((team) => team.id === trimmed || team.key === trimmed);
   if (exact) return { team: exact, ambiguous: false };
@@ -256,8 +271,8 @@ export function resolveCatalogTeamReference(reference: string): { team: CatalogT
   return { team: null, ambiguous: false };
 }
 
-export function getCatalogTeamOrThrow(reference: string): CatalogTeam {
-  const result = resolveCatalogTeamReference(reference);
+export async function getCatalogTeamOrThrow(reference: string): Promise<CatalogTeam> {
+  const result = await resolveCatalogTeamReference(reference);
   if (result.ambiguous) {
     throw conflict(`Catalog team slug "${reference}" is ambiguous. Use an id or key.`);
   }
@@ -310,7 +325,7 @@ export async function readCatalogTeamFile(
   reference: string,
   relativePath = "TEAM.md",
 ): Promise<CatalogTeamFileDetail> {
-  const team = getCatalogTeamOrThrow(reference);
+  const team = await getCatalogTeamOrThrow(reference);
   const resolved = resolveCatalogTeamFile(team, relativePath);
 
   if (resolved.fileEntry.kind === "asset") {
@@ -414,8 +429,8 @@ function renderSyntheticCompanyMarkdown(team: CatalogTeam) {
   return lines.join("\n");
 }
 
-function catalogProvenance(team: CatalogTeam) {
-  const manifest = getCatalogManifest();
+async function catalogProvenance(team: CatalogTeam) {
+  const manifest = await getCatalogManifest();
   return {
     catalogId: team.id,
     catalogKey: team.key,
@@ -428,8 +443,8 @@ function catalogProvenance(team: CatalogTeam) {
   };
 }
 
-function renderCatalogProvenanceYaml(team: CatalogTeam, targetManager: CatalogTargetManagerReference | null) {
-  const provenance = catalogProvenance(team);
+async function renderCatalogProvenanceYaml(team: CatalogTeam, targetManager: CatalogTargetManagerReference | null) {
+  const provenance = await catalogProvenance(team);
   const agentSlugs = Array.from(new Set(team.agentSlugs)).sort();
   const projectSlugs = Array.from(new Set(team.projectSlugs)).sort();
   const taskSlugs = team.files
@@ -798,15 +813,16 @@ function buildPortabilityInput(
   source: CompanyPortabilitySource,
   options: CatalogTeamImportOptions,
 ): CompanyPortabilityPreview {
+  const requestedInclude = options.include ?? {};
   return {
     source,
     include: {
-      company: false,
       agents: true,
       projects: true,
       issues: true,
       skills: true,
-      ...(options.include ?? {}),
+      ...requestedInclude,
+      company: false,
     },
     target: {
       mode: "existing_company",
@@ -853,7 +869,7 @@ export function teamsCatalogService(db: Db) {
     catalogRef: string,
     options: CatalogTeamImportOptions = {},
   ): Promise<CatalogTeamPreparedSource> {
-    const team = getCatalogTeamOrThrow(catalogRef);
+    const team = await getCatalogTeamOrThrow(catalogRef);
     const warnings: string[] = [];
     const errors: string[] = [];
 
@@ -877,7 +893,7 @@ export function teamsCatalogService(db: Db) {
       typeof files[".paperclip.yaml"] === "string"
         ? parseYamlFrontmatter(files[".paperclip.yaml"])
         : {};
-    const generatedExtension = parseYamlFrontmatter(renderCatalogProvenanceYaml(team, targetManager));
+    const generatedExtension = parseYamlFrontmatter(await renderCatalogProvenanceYaml(team, targetManager));
     files[".paperclip.yaml"] = renderYamlFile(mergePlainRecords(existingExtension, generatedExtension));
     rewriteAgentCatalogSkillRefs(team, files);
 
@@ -979,10 +995,6 @@ export function teamsCatalogService(db: Db) {
       throw unprocessable(`Catalog team source preparation failed: ${prepared.errors.join("; ")}`);
     }
 
-    const warnings = [
-      ...prepared.warnings,
-      ...await prepareSkillInstalls(companyId, prepared),
-    ];
     const importInput: CompanyPortabilityImport = {
       ...buildPortabilityInput(companyId, prepared.source, options),
       adapterOverrides: withSafeCatalogAdapterDefaults(
@@ -991,6 +1003,19 @@ export function teamsCatalogService(db: Db) {
       ),
       secretValues: options.secretValues,
     };
+    const importPreview = await portability.previewImport(importInput, {
+      mode: "agent_safe",
+      sourceCompanyId: companyId,
+    });
+    if (importPreview.errors.length > 0) {
+      throw unprocessable(`Catalog team import preview has errors: ${importPreview.errors.join("; ")}`);
+    }
+    const warnings = [
+      ...prepared.warnings,
+      ...importPreview.warnings,
+      `Catalog agents without explicit overrides default to ${DEFAULT_SAFE_CATALOG_ADAPTER_TYPE}. Pass adapterOverrides to use a different supported adapter.`,
+      ...await prepareSkillInstalls(companyId, prepared),
+    ];
     const result = await portability.importBundle(
       importInput,
       options.actor?.userId ?? (options.actor?.actorType === "user" ? options.actor.actorId : null),
@@ -1022,7 +1047,7 @@ export function teamsCatalogService(db: Db) {
    */
   async function listInstalledCatalogTeams(companyId: string): Promise<InstalledCatalogTeam[]> {
     const companyAgents = await agents.list(companyId);
-    const currentTeams = getCatalogTeams();
+    const currentTeams = await getCatalogTeams();
     const currentById = new Map(currentTeams.map((team) => [team.id, team]));
 
     type Aggregate = {