From 18b6c788d5d4dfc23bc1a7d78cf115ab35b7b1a1 Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Tue, 25 Aug 2026 13:51:54 -0700 Subject: [PATCH] feat(server): de-duplicate imported company names (#12145) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Importing a company package as a new company takes the company name from the package manifest > - Repeat imports of the same package therefore create several identically named companies, distinguishable only by issue prefix > - Users cannot tell which import they are looking at, which feeds the "my import disappeared" loop of importing again > - This pull request suffixes manifest-derived names with " (2)", " (3)", … on collision, while honoring explicitly typed names verbatim > - The benefit is that every imported company has a recognizable name ## Linked Issues or Issue Description **What existing behavior does this improve?** Naming of companies created by the company package import. **Subsystem affected** Server — company import (`server/src/services/company-portability.ts`). **Current behavior** The new-company branch uses `newCompanyName ?? manifest name ?? "Imported Company"` with no de-duplication. Only the issue prefix is unique. Three imports of the same package yield three companies with the same name. **Proposed behavior** When the name comes from the manifest (no explicit `newCompanyName`), the import checks existing company names case-insensitively and appends the first free " (N)" suffix. Explicit names remain honored verbatim. Name exhaustion (thousands of collisions) falls back to the base name rather than failing the import, since names carry no uniqueness invariant. **Breaking changes** None. Only the default name of newly imported companies changes, and only on collision. ## What Changed - New exported pure helper `dedupeImportedCompanyName(baseName, existingNames)`. - The new-company branch resolves the name through it when no explicit name was provided, reading existing names via `companyService.list()`. ## Verification - `cd server && npx vitest run src/__tests__/company-portability.test.ts` — 87 tests pass (new: pure helper cases and two `importBundle` tests for the suffixed manifest name and the honored explicit name). - `cd server && npx vitest run src/__tests__/company-portability-routes.test.ts src/__tests__/company-portability-import-batching.test.ts` — 44 passed, 1 skipped (pre-existing skip). - `cd server && pnpm run typecheck` — clean. ## Risks - Low risk. The check-then-create has a theoretical race with a concurrent import, but names have no unique constraint — the worst case is today's behavior (a duplicate name). Issue-prefix uniqueness is untouched. ## Model Used - Claude Fable 5 (`claude-fable-5`, Anthropic) with extended thinking and tool use, via Claude Code. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --- .../src/__tests__/company-portability.test.ts | 111 +++++++++++++++++- server/src/services/company-portability.ts | 37 +++++- 2 files changed, 143 insertions(+), 5 deletions(-) diff --git a/server/src/__tests__/company-portability.test.ts b/server/src/__tests__/company-portability.test.ts index 49edb4c881..c919bcb554 100644 --- a/server/src/__tests__/company-portability.test.ts +++ b/server/src/__tests__/company-portability.test.ts @@ -9,6 +9,10 @@ import type { CompanyPortabilityFileEntry } from "@paperclipai/shared"; const companySvc = { getById: vi.fn(), + // Async-empty default (not a bare vi.fn()): every new-company import reads + // the existing names for de-duplication, including describes that never + // touch this mock. + list: vi.fn(async () => []), create: vi.fn(), update: vi.fn(), }; @@ -154,7 +158,7 @@ vi.mock("../routes/org-chart-svg.js", () => ({ renderOrgChartPng: vi.fn(async () => Buffer.from("png")), })); -const { companyPortabilityService, parseGitHubSourceUrl, renderYamlBlock, renderFrontmatter } = await import("../services/company-portability.js"); +const { companyPortabilityService, dedupeImportedCompanyName, parseGitHubSourceUrl, renderYamlBlock, renderFrontmatter } = await import("../services/company-portability.js"); function asTextFile(entry: CompanyPortabilityFileEntry | undefined) { expect(typeof entry).toBe("string"); @@ -184,6 +188,7 @@ describe("company portability", () => { presentation: null, metadata: null, }); + companySvc.list.mockResolvedValue([]); companySvc.getById.mockResolvedValue({ id: "company-1", name: "Paperclip", @@ -2609,6 +2614,93 @@ describe("company portability", () => { ]); }); + it("suffixes a manifest-derived company name that collides with an existing company", async () => { + const portability = companyPortabilityService({} as any); + + companySvc.list.mockResolvedValue([ + { name: "Imported Paperclip" }, + // Case-insensitive: an existing "(2)" in any casing blocks that suffix. + { name: "imported paperclip (2)" }, + ]); + companySvc.create.mockResolvedValue({ + id: "company-imported", + name: "Imported Paperclip (3)", + }); + accessSvc.ensureMembership.mockResolvedValue(undefined); + + const files = { + "COMPANY.md": ["---", 'schema: "agentcompanies/v1"', 'name: "Imported Paperclip"', "---", ""].join("\n"), + }; + + await portability.importBundle({ + source: { type: "inline", rootPath: "paperclip-demo", files }, + include: { company: true, agents: false, projects: false, issues: false }, + // No newCompanyName: the manifest name is used and must be de-duplicated. + target: { mode: "new_company" }, + collisionStrategy: "rename", + }, "user-1"); + + expect(companySvc.create).toHaveBeenCalledWith(expect.objectContaining({ + name: "Imported Paperclip (3)", + })); + }); + + it("skips name de-duplication for agent-safe imports so collisions stay unobservable", async () => { + const portability = companyPortabilityService({} as any); + + companySvc.list.mockResolvedValue([{ name: "Imported Paperclip" }]); + companySvc.create.mockResolvedValue({ + id: "company-imported", + name: "Imported Paperclip", + }); + accessSvc.listActiveUserMemberships.mockResolvedValue([{ userId: "user-1" }]); + accessSvc.copyActiveUserMemberships.mockResolvedValue([]); + + const files = { + "COMPANY.md": ["---", 'schema: "agentcompanies/v1"', 'name: "Imported Paperclip"', "---", ""].join("\n"), + }; + + await portability.importBundle({ + source: { type: "inline", rootPath: "paperclip-demo", files }, + include: { company: true, agents: false, projects: false, issues: false }, + target: { mode: "new_company" }, + collisionStrategy: "rename", + }, "user-1", { mode: "agent_safe", sourceCompanyId: "company-1" }); + + // The instance-wide name list must never be consulted for a + // company-scoped agent, and no suffix may reflect a collision back. + expect(companySvc.list).not.toHaveBeenCalled(); + expect(companySvc.create).toHaveBeenCalledWith(expect.objectContaining({ + name: "Imported Paperclip", + })); + }); + + it("honors an explicitly typed company name even when it collides", async () => { + const portability = companyPortabilityService({} as any); + + companySvc.list.mockResolvedValue([{ name: "Imported Paperclip" }]); + companySvc.create.mockResolvedValue({ + id: "company-imported", + name: "Imported Paperclip", + }); + accessSvc.ensureMembership.mockResolvedValue(undefined); + + const files = { + "COMPANY.md": ["---", 'schema: "agentcompanies/v1"', 'name: "Imported Paperclip"', "---", ""].join("\n"), + }; + + await portability.importBundle({ + source: { type: "inline", rootPath: "paperclip-demo", files }, + include: { company: true, agents: false, projects: false, issues: false }, + target: { mode: "new_company", newCompanyName: "Imported Paperclip" }, + collisionStrategy: "rename", + }, "user-1"); + + expect(companySvc.create).toHaveBeenCalledWith(expect.objectContaining({ + name: "Imported Paperclip", + })); + }); + it("pauses imported agents and routines when pauseAutomations is requested", async () => { const portability = companyPortabilityService({} as any); @@ -5717,3 +5809,20 @@ describe("company portability", () => { expect(preview.plan.issuePlans).toHaveLength(0); }); }); + +describe("dedupeImportedCompanyName", () => { + it("returns the base name when nothing collides", () => { + expect(dedupeImportedCompanyName("Paperclip", ["Other Co"])).toBe("Paperclip"); + expect(dedupeImportedCompanyName("Paperclip", [])).toBe("Paperclip"); + }); + + it("suffixes past every taken candidate, case-insensitively", () => { + expect(dedupeImportedCompanyName("Paperclip", ["paperclip"])).toBe("Paperclip (2)"); + expect(dedupeImportedCompanyName("Paperclip", ["Paperclip", "Paperclip (2)"])).toBe("Paperclip (3)"); + expect(dedupeImportedCompanyName("Paperclip", ["PAPERCLIP", "paperclip (2)"])).toBe("Paperclip (3)"); + }); + + it("ignores surrounding whitespace in existing names", () => { + expect(dedupeImportedCompanyName("Paperclip", [" Paperclip "])).toBe("Paperclip (2)"); + }); +}); diff --git a/server/src/services/company-portability.ts b/server/src/services/company-portability.ts index a6df4e9ee1..85b61c9e46 100644 --- a/server/src/services/company-portability.ts +++ b/server/src/services/company-portability.ts @@ -229,6 +229,25 @@ function assertInlineSourceComplete(source: CompanyPortabilityImport["source"]) } } +/** + * Suffix a manifest-derived company name with " (2)", " (3)", … when it + * collides case-insensitively with an existing company, so repeat imports of + * the same package do not produce several identically named companies that + * only differ by issue prefix. Explicit user-typed names bypass this — they + * are the caller's deliberate choice. + */ +export function dedupeImportedCompanyName(baseName: string, existingNames: string[]): string { + const normalized = new Set(existingNames.map((name) => name.trim().toLowerCase())); + if (!normalized.has(baseName.trim().toLowerCase())) return baseName; + for (let suffix = 2; suffix < 10_000; suffix += 1) { + const candidate = `${baseName} (${suffix})`; + if (!normalized.has(candidate.toLowerCase())) return candidate; + } + // Pathological: thousands of identically named companies. Give up on the + // suffix rather than fail the import — names carry no uniqueness invariant. + return baseName; +} + function resolveSkillConflictStrategy(mode: ImportMode, collisionStrategy: CompanyPortabilityCollisionStrategy) { if (mode === "board_full") return collisionStrategy; return collisionStrategy === "skip" ? "skip" as const : "rename" as const; @@ -5241,11 +5260,21 @@ export function companyPortabilityService(db: Db, storage?: StorageService) { throw unprocessable("Safe new-company import requires at least one active user membership on the source company."); } } + const requestedCompanyName = asString(input.target.newCompanyName); + const manifestCompanyName = + sourceManifest.company?.name ?? sourceManifest.source?.companyName ?? "Imported Company"; + // De-duplicate only for board-driven imports. The lookup reads every + // company name in the instance, and reflecting a collision back through + // the numeric suffix would let a company-scoped agent (agent_safe mode) + // probe for the existence of company names outside its own company. const companyName = - asString(input.target.newCompanyName) ?? - sourceManifest.company?.name ?? - sourceManifest.source?.companyName ?? - "Imported Company"; + requestedCompanyName ?? + (mode === "agent_safe" + ? manifestCompanyName + : dedupeImportedCompanyName( + manifestCompanyName, + (await companies.list()).map((company) => company.name), + )); const created = await companies.create({ name: companyName, description: include.company ? (sourceManifest.company?.description ?? null) : null,